Skip to content

fix: eliminate unsound ptr::read double-drop UB in write_to_file (Program + Library)#2840

Open
amathxbt wants to merge 3 commits into0xMiden:nextfrom
amathxbt:fix-ub-double-drop-write-to-file
Open

fix: eliminate unsound ptr::read double-drop UB in write_to_file (Program + Library)#2840
amathxbt wants to merge 3 commits into0xMiden:nextfrom
amathxbt:fix-ub-double-drop-write-to-file

Conversation

@amathxbt
Copy link

Summary

Fixes #2814

Eliminates latent undefined behaviour (double-drop) in both Program::write_to_file and Library::write_to_file by replacing an unsound unsafe { core::ptr::read(&*err) } with the safe equivalent *err.


The Bug — Unsound ptr::read Causes Double-Drop

Both functions wrap file I/O in std::panic::catch_unwind to recover panics from the non-fallible ByteWriter API. In the panic handler:

// BEFORE — unsound in both core/src/program/mod.rs:161 and
//          crates/assembly-syntax/src/library/mod.rs:427
Ok(err) => unsafe { core::ptr::read(&*err) },

Why This Is Undefined Behaviour

p.downcast::<std::io::Error>() returns Ok(Box<io::Error>). The code:

  1. Dereferences the Box<io::Error> to get a &io::Error
  2. ptr::read performs a bitwise copy of the io::Error out of the box — without consuming the box
  3. The Box<io::Error> is then dropped at end of scope, freeing the memory that was already bitwise-copied out

This is a double-drop: the io::Error's destructor runs twice on the same memory — once via the value returned by ptr::read, and once when the Box is dropped. That is explicitly undefined behaviour in Rust.

From the docs: "Like forget, ptr::read does not run the destructor of self... The caller is responsible for managing the lifetime of the value."

The misleading // SAFETY comments claim it is "guaranteed safe" — but no such guarantee exists. The original author likely intended a safe move but reached for ptr::read unnecessarily.

Why It Hasn't Triggered Yet

The panic payload in the current WriteAdapter implementation is a formatted String message, not a boxed io::Error. So downcast::<io::Error>() currently returns Err, and the Ok(err) arm is never reached. However:

  • This is a silent latent hazard that activates the moment WriteAdapter is refactored to panic with an actual io::Error
  • The // SAFETY comment actively misleads future maintainers into thinking the unsafe block is correct
  • Tools like Miri and cargo-careful will flag this as UB today

The Fix — Safe Deref-Move

// AFTER — both files
Ok(err) => *err,

*err on a Box<T> in a value position performs a deref-move: it consumes the Box, moves the inner io::Error out, and the box's allocation is freed exactly once. This is entirely safe, requires no unsafe, and is idiomatic Rust.

Files Changed

File Location Change
core/src/program/mod.rs Line 161 unsafe { core::ptr::read(&*err) }*err
crates/assembly-syntax/src/library/mod.rs Line 427 unsafe { core::ptr::read(&*err) }*err

Pre-PR Checklist

…to_file

Replaces `unsafe { core::ptr::read(&*err) }` with `*err` to move the
error out of the Box instead of bitwise-copying it. The original code
caused a double-drop (undefined behaviour): ptr::read copies the
Box<io::Error> contents without consuming the box, then the box is
dropped at end of scope, freeing already-moved memory.

The fix uses safe Rust's move semantics via deref-move (`*err`), which
consumes the Box and moves the inner io::Error out, with no double-drop.

Fixes 0xMiden#2814
…ary::write_to_file

Same soundness fix as core/src/program/mod.rs: replace
`unsafe { core::ptr::read(&*err) }` with `*err` to safely move
the io::Error out of its Box rather than bitwise-copying it.

Fixes 0xMiden#2814
match p.downcast::<std::io::Error>() {
// SAFETY: It is guaranteed to be safe to read Box<std::io::Error>
Ok(err) => unsafe { core::ptr::read(&*err) },
Ok(err) => *err,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a small regression test that forces this downcast::<std::io::Error>() arm in both Program::write_to_file and Library::write_to_file, so this UB pattern is less likely to come back later?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Latent undefined behaviour in IO panic recovery (ptr::read causes double-drop)

2 participants