rust-lang / libc

Raw bindings to platform APIs for Rust
https://docs.rs/libc
Apache License 2.0
2.09k stars 1.04k forks source link

Incorrect `pthread_t` definition on Darwin (should be a raw pointer, not an `uintptr_t`) #2903

Open thomcc opened 2 years ago

thomcc commented 2 years ago

This is a bug, but not one we can probably fix without breaking too much (it would break std's reexport of pthread_t). Aside from breaking code that treats it directly as an integer, it would mean pthread_t would suddenly become !Send and !Sync, and I wouldn't be surprised if that's a problem for some.

Anyway, on Darwin-based OSes, pthread_t should be a pointer, but is instead an integer. I noticed this when trying to pass ptr::null_mut() into a function that Apple documents as having special behavior when passed a NULL for pthread_t.

However, it's worth noting that the structure it points to is not opaque (at least not fully), and low level code on Darwin could potentially need to access it (at the moment this is likely not possible to do correctly on stable Rust, due to the lack of stable "C-unwind"). Specifically it has a list of functions that may be manipulated by code that wants to have cleanup functions run on thread cancellation. This list is directly manipulated by the pthread_cleanup_push and pthread_cleanup_pop function-style C macros, but this would be done directly by Rust code, which cannot use such macros. This is pretty niche, but is a case that's slightly relevant to https://github.com/rust-lang/rust/issues/95496.

The correct definition on Darwin would look more like:

s! {
    pub struct __darwin_pthread_handler_rec {
        pub __routine: Option<unsafe extern "C" fn(arg1: *mut ::c_void)>,
        pub __arg: *mut ::c_void,
        pub __next: *mut __darwin_pthread_handler_rec,
    }
    pub struct _opaque_pthread_t {
        pub __sig: ::c_long,
        pub __cleanup_stack: *mut __darwin_pthread_handler_rec,
        pub __opaque: [::c_char; __PTHREAD_SIZE__],
    }
}

pub type __darwin_pthread_t = *mut _opaque_pthread_t;

pub type pthread_t = __darwin_pthread_t;

I suppose it's fine for code that cares about this to maintain its own bindings, though.

workingjubilee commented 1 year ago

Hmm. Making this code incompatible in a new version of libc is actually probably fine? This was part of the reason that these modules were deprecated and the libc crate was made to replace it. Old code that uses a prior version of libc will not be broken if we do this on the 0.3 bump. It will become incompatible with the deprecated std::os::raw definition, but... what else is new? That's been a problem for all FFI typedefs, and I don't believe std's ancient mistake is warranted as a blocking reason for fixing the bindings in the crate that was specifically made so that Rust could fix those bindings going forward.