Skip to content

Commit

Permalink
Fix UB in windows::start_read
Browse files Browse the repository at this point in the history
The primary issue is that `mem::transmute`
from `isize` to `Box<_>` (without first
casting to `*mut _`) is undefined behavior.

On Rust v1.78.0+, this ends up crashing with
`STATUS_ILLEGAL_INSTRUCTION` when
`ReadDirectoryChangesW` fails and this
branch is hit in release mode.

While this could be fixed by simply adding
`as *mut ReadDirectoryRequest`, this cleans
up the overall unsafe logic to be more
readable and idiomatic Rust.
  • Loading branch information
encounter authored and 0xpr03 committed Jun 26, 2024
1 parent 023dae1 commit abc8a65
Showing 1 changed file with 10 additions and 11 deletions.
21 changes: 10 additions & 11 deletions notify/src/windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
use crate::{bounded, unbounded, BoundSender, Config, Receiver, Sender};
use crate::{event::*, WatcherKind};
use crate::{Error, EventHandler, RecursiveMode, Result, Watcher};
use std::alloc;
use std::collections::HashMap;
use std::env;
use std::ffi::OsString;
use std::mem;
use std::os::raw::c_void;
use std::os::windows::ffi::{OsStrExt, OsStringExt};
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -255,7 +255,7 @@ fn stop_watch(ws: &WatchState, meta_tx: &Sender<MetaEvent>) {
}

fn start_read(rd: &ReadData, event_handler: Arc<Mutex<dyn EventHandler>>, handle: HANDLE) {
let mut request = Box::new(ReadDirectoryRequest {
let request = Box::new(ReadDirectoryRequest {
event_handler,
handle,
buffer: [0u8; BUF_SIZE as usize],
Expand All @@ -270,31 +270,30 @@ fn start_read(rd: &ReadData, event_handler: Arc<Mutex<dyn EventHandler>>, handle
| FILE_NOTIFY_CHANGE_CREATION
| FILE_NOTIFY_CHANGE_SECURITY;

let monitor_subdir = if (&request.data.file).is_none() && request.data.is_recursive {
let monitor_subdir = if request.data.file.is_none() && request.data.is_recursive {
1
} else {
0
};

unsafe {
let mut overlapped = std::mem::ManuallyDrop::new(Box::new(mem::zeroed::<OVERLAPPED>()));
let overlapped = alloc::alloc_zeroed(alloc::Layout::new::<OVERLAPPED>()) as *mut OVERLAPPED;
// 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);
overlapped.hEvent = request_p as isize;
let request = Box::leak(request);
(*overlapped).hEvent = request as *mut _ as _;

// 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,
request.buffer.as_mut_ptr() as *mut c_void,
BUF_SIZE,
monitor_subdir,
flags,
&mut 0u32 as *mut u32, // not used for async reqs
(&mut **overlapped) as *mut OVERLAPPED,
overlapped,
Some(handle_event),
);

Expand All @@ -303,8 +302,8 @@ fn start_read(rd: &ReadData, event_handler: Arc<Mutex<dyn EventHandler>>, handle
// Because of the error, ownership of the `overlapped` alloc was not passed
// over to `ReadDirectoryChangesW`.
// So we can claim ownership back.
let _overlapped_alloc = std::mem::ManuallyDrop::into_inner(overlapped);
let request: Box<ReadDirectoryRequest> = Box::from_raw(request_p);
let _overlapped = Box::from_raw(overlapped);
let request = Box::from_raw(request);
ReleaseSemaphore(request.data.complete_sem, 1, ptr::null_mut());
}
}
Expand Down

0 comments on commit abc8a65

Please sign in to comment.