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.

Fix

In Rust, std::mem::forget does not actually free the memory, instead it simply allows the memory to leak. This can lead to double free when the data object goes out of scope and its destructor is called automatically. The modification here uses std::mem::ManuallyDrop to wrap data. This ensures that data will not be automatically dropped when it goes out of scope, thus avoiding a double free scenario. With ManuallyDrop, we explicitly state that the data variable should not be dropped, thus avoiding any potential double free issues.

[0xpr03]

Hi, thanks for reporting.
Adding ManuallyDrop doesn't seem to be wrong. However I can't find any way this could panic - at least not in a way that it creates a problem. The only thing that could panic is ReadDirectoryChangesW itself, which would mean the C-API of windows is panicking, as it's a direct wrapper.

[kuzeyardabulut]

Thanks for your answer. For now, I guess there is no possible panic!() here, but it may be important to fix this error for the progress of the project.

I will create a PR. Please let me know if there is any mistake.