eyre-rs / eyre

A trait object based error handling type for easy idiomatic error handling and reporting in Rust applications
Apache License 2.0
1.35k stars 63 forks source link

Segfault in `Report::downcast()` introduced in 0.6.9 #141

Closed stevepryde closed 8 months ago

stevepryde commented 8 months ago

I recently had a test segfault after updating to the latest eyre (0.6.11). The same test passes on 0.6.8 but segfaults on 0.6.9 or later.

error: test failed, to rerun pass `--bin ...`

Caused by:
  process didn't exit successfully: ... (signal: 11, SIGSEGV: invalid memory reference)

Here is a minimal reproduction:

use eyre::Context;

fn main() {
    let err = trigger().unwrap_err();
    let _ = err.downcast::<MyError>();
}

#[derive(Debug, thiserror::Error)]
enum MyError {
    #[error("error building http client")]
    ClientConstruction,
    #[error(transparent)]
    SsmError(#[from] aws_sdk_ssm::Error),
}

fn trigger() -> eyre::Result<()> {
    do_thing().wrap_err("failed")?;
    Ok(())
}

fn do_thing() -> Result<(), MyError> {
    Err(MyError::ClientConstruction)
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_me() {
        let err = trigger().unwrap_err();
        let _ = err.downcast::<MyError>();
    }
}

Cargo.toml deps

[dependencies]
eyre = "0.6.9" # or later
thiserror = "1"
aws-sdk-ssm = "1"

I included the test because cargo test gives slightly more info.

Something about the second error variant is related, even though that variant isn't constructed. I used aws_sdk_ssm::Error in the error variant because that's how I hit the issue.

The issue seems to be in src/error.rs, in context_drop_rest(), which was changed in 0.6.9. Any reason why this code differs from anyhow ?

stevepryde commented 8 months ago

Additional details:

stevepryde commented 8 months ago

I get the same segfault also on latest Manjaro Linux (x86-64), using Rust 1.74.1.

abusch commented 8 months ago

I've managed to simplify the reproduction case to remove dependencies (thiserror and aws-sdk-ssm are off the hook):

#![allow(dead_code)]
use std::{error::Error, fmt::Display};

use eyre::Context;

fn main() {
    let err = trigger().unwrap_err();
    let _ = err.downcast::<MyError>();
    println!("it worked!");
}

#[derive(Debug)]
enum MyError {
    ClientConstruction,
    Other(String),
}

impl Display for MyError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "MyError error")
    }
}
impl Error for MyError {}

fn trigger() -> eyre::Result<()> {
    Err(MyError::ClientConstruction).wrap_err("failed")
}

The failure I get is slightly different (on a Mac M1 Sonoma 14.2) :

eyre_test(48377,0x1e194d000) malloc: *** error for object 0x1025c23ff: pointer being freed was not allocated
eyre_test(48377,0x1e194d000) malloc: *** set a breakpoint in malloc_error_break to debug

[Process exited 255]

Some things I've noticed:

ten3roberts commented 8 months ago

Thank you for opening the issue.

I will look into it and see what may be causing it

ten3roberts commented 8 months ago

I believe it is that the inner error is incorrectly freed even though it was downcasted, we still need to free the wrapped error.

The content of the Other variant is likely because it gives the enum a Drop impl if any fields needs drop. Without it, the box deallocation will take shortcuts when freeing and not drop the contents, but just free

stevepryde commented 7 months ago

Any reason why this fix hasn't been released yet (and affected versions yanked)? It's a known soundness issue in a popular crate.