Fixing Potential Double Free Issue

[kuzeyardabulut]

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

	let mut overlapped: Box<OVERLAPPED> = Box::new(mem::zeroed());
	// When using callback based async requests, we are allowed to use the hEvent member
	// for our own purposes
	let req_buf = request.buffer.as_mut_ptr() as *mut c_void;
	let request_p = Box::into_raw(request) as isize;
	overlapped.hEvent = request_p;
	// This is using an asynchronous call with a completion routine for receiving notifications
	// An I/O completion port would probably be more performant
	let ret = ReadDirectoryChangesW(
	handle,
	req_buf,
	BUF_SIZE,
	monitor_subdir,
	flags,
	&mut 0u32 as *mut u32, // not used for async reqs
	&mut *overlapped as *mut OVERLAPPED,
	Some(handle_event),
	);
	if ret == 0 {
	// error reading. retransmute request memory to allow drop.
	// allow overlapped to drop by omitting forget()
	let request: Box<ReadDirectoryRequest> = mem::transmute(request_p);
	ReleaseSemaphore(request.data.complete_sem, 1, ptr::null_mut());
	} else {
	// read ok. forget overlapped to let the completion routine handle memory
	mem::forget(overlapped);

If a panic!() occurs between the Box::new() function and std::mem::forget, a double free vulnerability emerges.

Related Issue

[0xpr03]

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

[0xpr03]

Also please don't name your commit "Update windows.rs"

[kuzeyardabulut]

I must have overlooked it, I fixed it.

[Nilstrieb]

Looks like it's still wrong?

[kuzeyardabulut]

Looks like it's still wrong?

How can i fix it ?

[Nemo157]

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.

[kuzeyardabulut]

Thanks!

[adamreichold]

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.

[danielhenrymantilla]

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());
}

[0xpr03]

Thanks to the other reviewers.