Skip to content

Fix unsound unsafe impl Send + Sync for Display (issue #2) #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
moneppo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix unsound unsafe impl Send + Sync for Display (issue #2) #4

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use byteorder::{ByteOrder, LittleEndian};
use bytes::{BufMut, BytesMut};
use hidapi::{HidApi, HidDevice, HidError, HidResult};
use pyo3::{exceptions::PyTypeError, prelude::*};
use std::sync::Mutex;

trait ResultExt<T> {
fn to_py_err(self) -> PyResult<T>;
Expand Down Expand Up @@ -131,15 +132,19 @@ impl Rect {
}
}

/// Wrapper that marks HidDevice as Send.
///
/// Safety: The C hidapi library serializes concurrent access to a device handle
/// internally, so it is safe to send an HidDevice to another thread.
Comment on lines +137 to +138
Copy link
Copy Markdown
Contributor

@cceckman cceckman May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see this in the C code or the docs, but I'm less of an expert there; do you have a link to where this gets implemented / how it's accomplished?

If it's internally synchronized, it should implement Sync as well. But given the hidapi crate doesn't implement Sync for HidDevice, I'm suspicious that it doesn't actually have internal synchronization.

(Even if it does implement Sync, we might want a Mutex on our accessor to synchronize the Glider-level transactions.)

struct SendableDevice(HidDevice);
unsafe impl Send for SendableDevice {}
Copy link
Copy Markdown
Contributor

@cceckman cceckman May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like HidDevice should already be Send.

The inner device has its own unsafe impl:

https://github.com/ruabmbua/hidapi-rs/blob/7c8358fd275b394df588cc7ad513693b46d4d908/src/hidapi.rs#L160

and it's a restriction on HidDeviceBackend:

https://github.com/ruabmbua/hidapi-rs/blob/7c8358fd275b394df588cc7ad513693b46d4d908/src/lib.rs#L121

So I think HidDevice should already be Send. If we remove this unsafe impl, does everything still compile?

In general, the unsafe code should only be in the crate that does the unsafe->safe bridge, FFI and whatnot. (In this case,hidapi.) It tells us what guarantees are provided.


/// Core structure defining the display and possible interactions.
#[pyclass(frozen)]
pub struct Display {
device: HidDevice,
device: Mutex<SendableDevice>,
Copy link
Copy Markdown
Contributor

@cceckman cceckman May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
device: Mutex<SendableDevice>,
// The Glider API has a request/response protocol. We want to make sure we capture the response for a given request, so we lock the device for each transaction.
device: Mutex<SendableDevice>,

}

unsafe impl Send for Display {}
unsafe impl Sync for Display {}

#[pymethods]
impl Display {
/// Connects to the display and returns a `Display` struct for control.
Expand All @@ -151,7 +156,7 @@ impl Display {
let api = HidApi::new_without_enumerate().to_py_err()?;
let device = api.open(VENDOR_ID, PRODUCT_ID).to_py_err()?;

Ok(Self { device })
Ok(Self { device: Mutex::new(SendableDevice(device)) })
}

/// Sets the mode for a region of the display. Note that this will always
Expand All @@ -166,10 +171,11 @@ impl Display {
buf.put_i16_le(area.x1);
buf.put_i16_le(area.y1);
buf.put_u16(crc16::State::<crc16::XMODEM>::calculate(&buf));
self.device.write(&buf).to_py_err()?;
let device = self.device.lock().unwrap();
device.0.write(&buf).to_py_err()?;

let mut response: [u8; 32] = [0; 32];
self.device.read_timeout(&mut response, 200).to_py_err()?;
device.0.read_timeout(&mut response, 200).to_py_err()?;
match LittleEndian::read_u16(&response) {
0x00 => Err(PyTypeError::new_err("invalid command")),
0x01 => Err(PyTypeError::new_err("checksum incorrect")),
Expand All @@ -193,10 +199,11 @@ impl Display {

let chksum = crc16::State::<crc16::XMODEM>::calculate(&buf);
buf.put_u16(chksum);
self.device.write(&buf).to_py_err()?;
let device = self.device.lock().unwrap();
device.0.write(&buf).to_py_err()?;

let mut response: [u8; 16] = [0; 16];
self.device.read_timeout(&mut response, 200).to_py_err()?;
device.0.read_timeout(&mut response, 200).to_py_err()?;
match LittleEndian::read_u16(&response) {
0x00 => Err(PyTypeError::new_err("invalid command")),
0x01 => Err(PyTypeError::new_err("checksum incorrect")),
Expand Down