Fixing Potential Double Free Issue#517
Conversation
notify/src/windows.rs
Outdated
| // allow overlapped to drop by dropping ManuallyDrop | ||
| let request: Box<ReadDirectoryRequest> = mem::transmute(request_p); | ||
|
|
||
| std::mem::ManuallyDrop::drop(&mut overlapped); |
There was a problem hiding this comment.
this changes the logic - we didn't drop it in case ret != 0, now we're dropping it right before a releasesemaphore call on ret == 0, that can't be right
There was a problem hiding this comment.
Also please don't name your commit "Update windows.rs"
There was a problem hiding this comment.
I must have overlooked it, I fixed it.
There was a problem hiding this comment.
Looks like it's still wrong?
How can i fix it ?
There was a problem hiding this comment.
The drop call should still be in the if ret == 0 path (and none in the else path), but after the ReleaseSemaphore call, so the value is dropped at the same place it would have been previously.
There was a problem hiding this comment.
I would like to add that if ManuallyDrop is used, then ManuallyDrop::into_inner should be used re-arm the Drop impl of Box<OVERLAPPED>.
Personally, I have to say that using the mem::forget does look like the superior solution in this case, from a clarity and therefore safety perspective. Especially since the original issue description
If a panic!() occurs between the Box::new() function and std::mem::forget, a double free vulnerability emerges.
is wrong: Conceptually, ownership of overlapped is passed to ReadDirectoryChangesW which is why we need to disarm the Drop when that function indicates success. So a panic would have to occur between ReadDirectoryChangesW returning and mem::forget to open the code to a potential double free, but there is no code in between for ret != 0. A panic between Box::new and ReadDirectoryChangesW would not imply a double-free opportunity.
There was a problem hiding this comment.
The current code is indeed equivalent to the one pre-PR, and robust to some future panic paths that maybe could be introduced between ReadDirectoryChangesW succeeding (and thus claiming ownership of the OVERLAPPED heap allocation), and the forget (to relinquish our ownership of the allocation) happening ✅
I have nonetheless suggested a slight variation which uses ManuallyDrop::into_inner, based on what @adamreichold suggested, since it is a less error-prone API than drop, and maps better to what is conceptually happening.
Finally, in this case, since we are "just" dealing with a heap allocation, rather than ManuallyDrop<Box<…>>, I'd personally simply use *mut …:
const WIN_FALSE: BOOL = 0;
let overlapped_p = Box::into_raw(Box::new(mem::zeroed::<OVERLAPPED>()));
…
let success = ReadDirectoryChangesW(…);
if success == WIN_FALSE {
// error reading. retransmute request memory to allow drop.
// Because of the error, ownership of the `overlapped` alloc was not passed
// over to `ReadDirectoryChangesW`.
// So we can claim ownership back.
let _overlapped_alloc = Box::from_raw(overlapped);
request = Box::from_raw(request_p);
ReleaseSemaphore(request.data.complete_sem, 1, ptr::null_mut());
}Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>
Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>
|
Thanks to the other reviewers. |
Hi,
I found a memory-safety/soundness issue in this crate while scanning Rust code for potential vulnerabilities. This PR contains a fix for the issue.
Issue Description
notify/notify/src/windows.rs
Lines 281 to 310 in 5f40b83
If a panic!() occurs between the
Box::new()function andstd::mem::forget, a double free vulnerability emerges.Related Issue