HidDevice::drop is not thread safe on Windows

[YgorSouza]

I came across this problem in my application and managed to write a minimal reproduce for it:

Click to expand

#![forbid(unsafe_code)]
use std::{sync::Mutex, time::Duration};

use hidapi::HidDevice;

fn main() {
    let hidapi = hidapi::HidApi::new().unwrap();
    let devices = Mutex::new(Vec::<HidDevice>::new());
    std::thread::scope(|s| {
        s.spawn(|| {
            let mut buffer = [0; 2000];
            for _ in 0..10_000 {
                for device in devices.lock().unwrap().iter() {
                    _ = device.read(&mut buffer);
                }
                std::thread::sleep(Duration::from_millis(1));
            }
        });
        for _ in 0..2000 {
            {
                let mut devices = devices.lock().unwrap();
                devices.clear();
                devices.extend(hidapi.device_list().map(|d| {
                    let d = d.open_device(&hidapi).unwrap();
                    d.set_blocking_mode(false).unwrap();
                    d
                }));
                assert!(!devices.is_empty());
            }
            std::thread::sleep(Duration::from_millis(1));
        }
    });
}

This simply consists of two threads, where one repeatedly opens and closes all connected devices, and the other tries to read all devices that are open. It only uses safe Rust, and only depends on hidapi 2.6.0, and when I run it with some custom HID devices connected, it crashes almost every time with either (exit code: 0xc0000374, STATUS_HEAP_CORRUPTION) or (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION). This happens with or without the windows-native feature.

I believe this is due to the fact that the Drop implementation calls CancelIo, which only cancels I/O operations started on the calling thread. I think it should use CancelIoEx instead.
With the windows-native feature enabled and the following patch, it did not crash during my 30+ tests:

Click to expand

diff --git a/src/windows_native/mod.rs b/src/windows_native/mod.rs
index cffbca2..0726fc4 100644
--- a/src/windows_native/mod.rs
+++ b/src/windows_native/mod.rs
@@ -46,7 +46,7 @@ use windows_sys::Win32::Storage::FileSystem::{
     OPEN_EXISTING,
 };
 use windows_sys::Win32::System::Threading::ResetEvent;
-use windows_sys::Win32::System::IO::{CancelIo, DeviceIoControl};
+use windows_sys::Win32::System::IO::{CancelIo, CancelIoEx, DeviceIoControl};

 const STRING_BUF_LEN: usize = 128;

@@ -335,7 +335,7 @@ impl HidDeviceBackendWindows for HidDevice {
 impl Drop for HidDevice {
     fn drop(&mut self) {
         unsafe {
-            CancelIo(self.device_handle.as_raw());
+            CancelIoEx(self.device_handle.as_raw(), null());
         }
     }
 }

This has been a topic of discussion in the original C library for many years (see for example libusb/hidapi#133 and signal11/hidapi#48), but I believe it is the correct solution at least for the Rust crate. I wasn't able to test if the other call to CancelIo needs to be changed to CancelIoEx as well, since my application only reads and writes from one thread.

For completeness, although I don't think it matters, I ran these tests on Windows 11 Pro, Version 23H2.

[ruabmbua]

Hm this is a really weird issue. I don`t like the windows API design at all to be honest (having to cancel operations). Also I am very unfamiliar with the windows API in general. I think I am going to subscribe to the thread in the C library. Also maybe @sidit77 has any insights?

[sidit77]

Seems like a reasonable change. I think it's reasonable to match Microsoft in term of Windows version support. This means that the oldest supported version is 10.0.10240.

In general I just copied the C library for the IO code to avoid intoducing new bugs. It wasn't a conscious decision to use CancelIo over CancelIoEx.

I don't think the second call to CancelIo is problematic but I would replace it anyway because I don't really see a reason not to use CancelIoEx. You could even pass in the OVERLAPPED pointer.

The write function is also problematic, now that I think about it, as it doesn't cancel the write operation when it times out. I suspect the C library doesn't do it because it requires CancelIoEx if you don't want to mess up pending reads and potentially drop data.

[YgorSouza]

I opened a PR that should properly cancel both the read and the write on drop, if anyone wants to give it a try. Should this be reported to rustsec? Since it can potentially corrupt memory.