Skip to content

Commit

Permalink
Fix invalid drop impl call in Report::downcast (#143)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ten3roberts committed Dec 30, 2023
1 parent 32d84dc commit 770ac3f
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 10 deletions.
4 changes: 2 additions & 2 deletions eyre/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -698,13 +698,13 @@ where
// ptr::read to take ownership of that value.
if TypeId::of::<D>() == target {
unsafe {
e.cast::<ErrorImpl<ContextError<ManuallyDrop<E>, E>>>()
e.cast::<ErrorImpl<ContextError<ManuallyDrop<D>, E>>>()
.into_box()
};
} else {
debug_assert_eq!(TypeId::of::<E>(), target);
unsafe {
e.cast::<ErrorImpl<ContextError<E, ManuallyDrop<E>>>>()
e.cast::<ErrorImpl<ContextError<D, ManuallyDrop<E>>>>()
.into_box()
};
}
Expand Down
5 changes: 4 additions & 1 deletion eyre/tests/drop/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ impl Flag {
#[derive(Debug)]
pub struct DetectDrop {
has_dropped: Flag,
label: &'static str,
}

impl DetectDrop {
pub fn new(has_dropped: &Flag) -> Self {
pub fn new(label: &'static str, has_dropped: &Flag) -> Self {
DetectDrop {
label,
has_dropped: Flag {
atomic: Arc::clone(&has_dropped.atomic),
},
Expand All @@ -46,6 +48,7 @@ impl Display for DetectDrop {

impl Drop for DetectDrop {
fn drop(&mut self) {
eprintln!("Dropping {}", self.label);
let already_dropped = self.has_dropped.atomic.swap(true, SeqCst);
assert!(!already_dropped);
}
Expand Down
10 changes: 6 additions & 4 deletions eyre/tests/test_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ fn test_inference() -> Result<()> {
macro_rules! context_type {
($name:ident) => {
#[derive(Debug)]
#[repr(C)]
struct $name {
message: &'static str,
_drop: DetectDrop,
message: &'static str,
}

impl Display for $name {
Expand All @@ -37,6 +38,7 @@ context_type!(MidLevel);

#[derive(Error, Debug)]
#[error("{message}")]
#[repr(C)]
struct LowLevel {
message: &'static str,
drop: DetectDrop,
Expand Down Expand Up @@ -67,22 +69,22 @@ fn make_chain() -> (Report, Dropped) {

let low = LowLevel {
message: "no such file or directory",
drop: DetectDrop::new(&dropped.low),
drop: DetectDrop::new("LowLevel", &dropped.low),
};

// impl Report for Result<T, E>
let mid = Err::<(), LowLevel>(low)
.wrap_err(MidLevel {
message: "failed to load config",
_drop: DetectDrop::new(&dropped.mid),
_drop: DetectDrop::new("MidLevel", &dropped.mid),
})
.unwrap_err();

// impl Report for Result<T, Error>
let high = Err::<(), Report>(mid)
.wrap_err(HighLevel {
message: "failed to start server",
_drop: DetectDrop::new(&dropped.high),
_drop: DetectDrop::new("HighLevel", &dropped.high),
})
.unwrap_err();

Expand Down
2 changes: 1 addition & 1 deletion eyre/tests/test_convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ fn test_convert() {
maybe_install_handler().unwrap();

let has_dropped = Flag::new();
let error: Report = Report::new(DetectDrop::new(&has_dropped));
let error: Report = Report::new(DetectDrop::new("TestConvert", &has_dropped));
let box_dyn = Box::<dyn StdError + Send + Sync>::from(error);
assert_eq!("oh no!", box_dyn.to_string());
drop(box_dyn);
Expand Down
2 changes: 1 addition & 1 deletion eyre/tests/test_downcast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ fn test_drop() {
maybe_install_handler().unwrap();

let has_dropped = Flag::new();
let error: Report = Report::new(DetectDrop::new(&has_dropped));
let error: Report = Report::new(DetectDrop::new("DetectDrop", &has_dropped));
drop(error.downcast::<DetectDrop>().unwrap());
assert!(has_dropped.get());
}
Expand Down
2 changes: 1 addition & 1 deletion eyre/tests/test_repr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,6 @@ fn test_drop() {
maybe_install_handler().unwrap();

let has_dropped = Flag::new();
drop(Report::new(DetectDrop::new(&has_dropped)));
drop(Report::new(DetectDrop::new("TestDrop", &has_dropped)));
assert!(has_dropped.get());
}

0 comments on commit 770ac3f

Please sign in to comment.