fix(core,assembly): replace unsound ptr::read in panic recovery#2934
fix(core,assembly): replace unsound ptr::read in panic recovery#2934giwaov wants to merge 1 commit into0xMiden:nextfrom
Conversation
5a393bc to
2fd0051
Compare
|
@amathxbt Please cease spam comments. |
|
This PR contains unsigned commits. All commits must be cryptographically signed (GPG or SSH). Unsigned commits:
For instructions on setting up commit signing and re-signing existing commits, see: |
663e84c to
6e319ed
Compare
huitseeker
left a comment
There was a problem hiding this comment.
This just needs a small wording change in the PR description & Changelog, otherwise LGTM.
CHANGELOG.md
Outdated
| - Rejected non-syscall references to exported kernel procedures in the linker ([#2902](https://github.com/0xMiden/miden-vm/issues/2902)). | ||
| #### Bug Fixes | ||
|
|
||
| - Removed unsound `ptr::read` in IO panic recovery that could cause a double-drop ([#2934](https://github.com/0xMiden/miden-vm/pull/2934)). |
There was a problem hiding this comment.
The fix here is smaller than the current wording. This patch removes UB in the downcast::<std::io::Error>() arm by replacing ptr::read with *err, but it does not make normal write failures return io::Error today. See above for details.
I think the PR title and this changelog line should say that plainly, like remove UB in panic recovery or replace unsound ptr::read in panic recovery, so readers do not think the full I/O recovery path is fixed.
| Err(err) => std::panic::resume_unwind(err), | ||
| } | ||
| .map_err(|p| match p.downcast::<std::io::Error>() { | ||
| Ok(err) => *err, |
There was a problem hiding this comment.
The reason this still does not guarantee io::Error on normal write failure is that the writer below is not panicking with io::Error today. ByteWriter for std::io::Write uses write_all(...).expect("write failed"), so the usual panic payload is the formatted message, not std::io::Error, and this branch is skipped. I do not think that wider behavior needs to be fixed in this PR. Reworking that would mean changing how write failures are surfaced, while this patch is already valuable as the smaller safety fix: it removes the UB if the std::io::Error branch is ever reached.
Replace unsafe ptr::read with safe *err unbox in the downcast::<std::io::Error>() arm of catch_unwind panic recovery. The ptr::read performed a bitwise copy out of the Box while the Box was still dropped at end of scope, causing potential UB via double-drop. This patch removes the unsafe block entirely. Closes 0xMiden#2814
6e319ed to
f110921
Compare
|
Updated the PR title, commit message, and CHANGELOG wording per your feedback. Now says 'replace unsound ptr::read in panic recovery' to clarify this is the smaller UB fix, not a full I/O recovery path change. |
Description
\Program::write_to_file\ and \Library::write_to_file\ use \catch_unwind\ to convert panics from \ByteWriter\ into \io::Error. On the downcast success branch, the code uses:
\
ust
unsafe { core::ptr::read(&*err) }
\\
This is unsound: \ptr::read\ performs a bitwise copy of the \std::io::Error\ out of the \Box, but the original \Boxstd::io::Error\ is then dropped at end of scope, causing a double-drop (undefined behaviour).
Fix
Replace with *err, which safely moves the value out of the \Box\ via \DerefMove/unbox semantics no \unsafe\ needed.
Changes
Both changes remove \unsafe\ blocks entirely from these functions.
Closes #2814