Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
stevepryde opened this issue Dec 20, 2023 · 6 comments · Fixed by #143
Closed

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

stevepryde opened this issue Dec 20, 2023 · 6 comments · Fixed by #143
Assignees
Labels
C-bug Category: Something isn't working

Comments

@stevepryde
Copy link

stevepryde commented Dec 20, 2023

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
Copy link
Author

Additional details:

  • Mac M1 (macOS 14.2.1)
  • Rust 1.74.1 (also tried on 1.74.0 and 1.73.0, same result)

@stevepryde
Copy link
Author

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

@abusch
Copy link

abusch commented Dec 20, 2023

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:

  • replacing the Other variant with Other(u32) works fine. So does Other (empty variant). But Other(Box<u32>) fails
  • removing .wrap_err() (i.e returning Err(MyError::ClientConstruction.into()) in trigger()) makes it work

@ten3roberts
Copy link
Contributor

Thank you for opening the issue.

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

@ten3roberts
Copy link
Contributor

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

@ten3roberts ten3roberts self-assigned this Dec 29, 2023
@ten3roberts ten3roberts added the C-bug Category: Something isn't working label Dec 29, 2023
ten3roberts added a commit that referenced this issue Dec 29, 2023
The context chain downcast called converted to the wrong inner type
which caused the wrong drop impl to be called.

The tests did not catch this because they had compatible drop
implementations due to matching type layout. Solved by swizzling the
fields of the chain test types to force incompatible layouts
thenorili pushed a commit that referenced this issue Dec 30, 2023
The context chain downcast called converted to the wrong inner type
which caused the wrong drop impl to be called.

The tests did not catch this because they had compatible drop
implementations due to matching type layout. Solved by swizzling the
fields of the chain test types to force incompatible layouts

Resolves: #141
@stevepryde
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants