-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Fix the definition of sigevent #2813
Changes from all commits
3c21a9f
c77fdd6
84057ce
7064630
c72e45f
62d6477
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1393,18 +1393,6 @@ s_no_extra_traits! { | |
__reserved: [::c_long; 4] | ||
} | ||
|
||
pub struct sigevent { | ||
pub sigev_notify: ::c_int, | ||
pub sigev_signo: ::c_int, | ||
pub sigev_value: ::sigval, | ||
//The rest of the structure is actually a union. We expose only | ||
//sigev_notify_thread_id because it's the most useful union member. | ||
pub sigev_notify_thread_id: ::lwpid_t, | ||
#[cfg(target_pointer_width = "64")] | ||
__unused1: ::c_int, | ||
__unused2: [::c_long; 7] | ||
} | ||
|
||
pub struct ptsstat { | ||
#[cfg(any(freebsd12, freebsd13, freebsd14))] | ||
pub dev: u64, | ||
|
@@ -1637,6 +1625,95 @@ s_no_extra_traits! { | |
} | ||
} | ||
|
||
#[cfg(libc_union)] | ||
s! { | ||
pub struct __c_anonymous_sigev_thread { | ||
pub _function: *mut ::c_void, // Actually a function pointer | ||
pub _attribute: *mut ::pthread_attr_t, | ||
} | ||
|
||
// When sigevent was first added to libc, Rust still didn't support unions. | ||
// So the definition only included one of the union's member. This | ||
// structure exists for backwards-compatibility with consumers that still | ||
// try to access that one member. | ||
#[doc(hidden)] | ||
#[deprecated( | ||
since = "0.2.147", | ||
note = "Use sigevent instead" | ||
)] | ||
pub struct sigevent_0_2_126 { | ||
pub sigev_notify: ::c_int, | ||
pub sigev_signo: ::c_int, | ||
pub sigev_value: ::sigval, | ||
pub sigev_notify_thread_id: ::lwpid_t, | ||
#[cfg(target_pointer_width = "64")] | ||
__unused1: ::c_int, | ||
__unused2: [::c_long; 7] | ||
} | ||
} | ||
|
||
#[cfg(libc_union)] | ||
s_no_extra_traits! { | ||
// Can't correctly impl Debug for unions | ||
#[allow(missing_debug_implementations)] | ||
pub union __c_anonymous_sigev_un { | ||
pub _threadid: ::__lwpid_t, | ||
pub _sigev_thread: __c_anonymous_sigev_thread, | ||
pub _kevent_flags: ::c_ushort, | ||
__spare__: [::c_long; 8], | ||
} | ||
|
||
pub struct sigevent { | ||
pub sigev_notify: ::c_int, | ||
pub sigev_signo: ::c_int, | ||
pub sigev_value: ::sigval, | ||
pub _sigev_un: __c_anonymous_sigev_un, | ||
/// Exists just to prevent the struct from being safely constructed, | ||
/// because the Debug, Hash, PartialImpl, and | ||
/// Deref<Target=sigevent_0_2_126> trait impls might read uninitialized | ||
/// fields of _sigev_un. This field may be removed once those trait | ||
/// impls are. | ||
_private: () | ||
} | ||
} | ||
|
||
#[cfg(not(libc_union))] | ||
s_no_extra_traits! { | ||
pub struct sigevent { | ||
pub sigev_notify: ::c_int, | ||
pub sigev_signo: ::c_int, | ||
pub sigev_value: ::sigval, | ||
pub _unused0: ::lwpid_t, | ||
#[cfg(target_pointer_width = "64")] | ||
__unused1: ::c_int, | ||
__unused2: [::c_long; 7], | ||
/// Exists just to prevent the struct from being safely constructed, | ||
/// because the Debug, Hash, PartialImpl, and | ||
/// Deref<Target=sigevent_0_2_126> trait impls might read uninitialized | ||
/// fields of _sigev_un. This field may be removed once those trait | ||
/// impls are. | ||
_private: () | ||
} | ||
} | ||
|
||
#[allow(deprecated)] | ||
#[cfg(libc_union)] | ||
impl ::core::ops::Deref for sigevent { | ||
type Target = sigevent_0_2_126; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
unsafe { &*(self as *const Self as *const sigevent_0_2_126) } | ||
} | ||
} | ||
|
||
#[allow(deprecated)] | ||
#[cfg(libc_union)] | ||
impl ::core::ops::DerefMut for sigevent { | ||
fn deref_mut(&mut self) -> &mut Self::Target { | ||
unsafe { &mut *(self as *mut Self as *mut sigevent_0_2_126) } | ||
} | ||
} | ||
|
||
cfg_if! { | ||
if #[cfg(feature = "extra_traits")] { | ||
impl PartialEq for utmpx { | ||
|
@@ -1826,33 +1903,78 @@ cfg_if! { | |
} | ||
} | ||
|
||
#[cfg(libc_union)] | ||
impl PartialEq for sigevent { | ||
fn eq(&self, other: &sigevent) -> bool { | ||
self.sigev_notify == other.sigev_notify | ||
&& self.sigev_signo == other.sigev_signo | ||
&& self.sigev_value == other.sigev_value | ||
// sigev_notify indicates which union fields are valid | ||
&& match self.sigev_notify { | ||
::SIGEV_NONE => true, | ||
::SIGEV_SIGNAL => true, | ||
::SIGEV_THREAD => unsafe { | ||
self._sigev_un._sigev_thread | ||
== other._sigev_un._sigev_thread | ||
}, | ||
::SIGEV_KEVENT => unsafe { | ||
self._sigev_un._kevent_flags | ||
== other._sigev_un._kevent_flags | ||
}, | ||
::SIGEV_THREAD_ID => unsafe { | ||
self._sigev_un._threadid | ||
== other._sigev_un._threadid | ||
}, | ||
Comment on lines
+1916
to
+1927
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All these unsafe blocks are reading and comparing potentially uninitialized data from the union. fn main() {
let sigevent = libc::sigevent {
sigev_notify: libc::SIGEV_THREAD,
sigev_signo: 0,
sigev_value: libc::sigval {
sival_ptr: 0 as *mut _,
},
_sigev_un: libc::__c_anonymous_sigev_un {
_kevent_flags: 0,
},
};
let _eq = sigevent == sigevent;
} error: Undefined Behavior: type validation failed: encountered uninitialized raw pointer
--> /git/libc/src/unix/bsd/freebsdlike/freebsd/mod.rs:233:1
|
233 | / s! {
234 | | pub struct aiocb {
235 | | pub aio_fildes: ::c_int,
236 | | pub aio_offset: ::off_t,
... |
1013 | | }
1014 | | }
| |_^ type validation failed: encountered uninitialized raw pointer
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: inside `<libc::__c_anonymous_sigev_thread as std::cmp::PartialEq>::eq` at /git/libc/src/unix/bsd/freebsdlike/freebsd/mod.rs:876:9
= note: inside `<libc::sigevent as std::cmp::PartialEq>::eq` at /git/libc/src/unix/bsd/freebsdlike/freebsd/mod.rs:1445:29
note: inside `main` at src/main.rs:12:15
--> src/main.rs:12:15
|
12 | let _eq = sigevent == sigevent;
| ^^^^^^^^^^^^^^^^^^^^
= note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I think we should remove most trait impls for any struct that contains a union (see #2816). But how to do it without breaking existing code? |
||
_ => false | ||
} | ||
} | ||
} | ||
#[cfg(not(libc_union))] | ||
impl PartialEq for sigevent { | ||
fn eq(&self, other: &sigevent) -> bool { | ||
self.sigev_notify == other.sigev_notify | ||
&& self.sigev_signo == other.sigev_signo | ||
&& self.sigev_value == other.sigev_value | ||
&& self.sigev_notify_thread_id | ||
== other.sigev_notify_thread_id | ||
} | ||
} | ||
impl Eq for sigevent {} | ||
impl ::fmt::Debug for sigevent { | ||
fn fmt(&self, f: &mut ::fmt::Formatter) -> ::fmt::Result { | ||
f.debug_struct("sigevent") | ||
.field("sigev_notify", &self.sigev_notify) | ||
.field("sigev_signo", &self.sigev_signo) | ||
.field("sigev_value", &self.sigev_value) | ||
.field("sigev_notify_thread_id", | ||
&self.sigev_notify_thread_id) | ||
.finish() | ||
let mut ds = f.debug_struct("sigevent"); | ||
ds.field("sigev_notify", &self.sigev_notify); | ||
ds.field("sigev_signo", &self.sigev_signo); | ||
ds.field("sigev_value", &self.sigev_value); | ||
#[cfg(libc_union)] | ||
// The sigev_notify field indicates which union fields are valid | ||
unsafe { | ||
match self.sigev_notify { | ||
::SIGEV_THREAD => ds.field("_sigev_thread", | ||
&self._sigev_un._sigev_thread), | ||
::SIGEV_KEVENT => ds.field("_kevent_flags", | ||
&self._sigev_un._kevent_flags), | ||
::SIGEV_THREAD_ID => ds.field("_threadid", | ||
&self._sigev_un._threadid), | ||
Comment on lines
+1951
to
+1956
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. fn main() {
let sigevent = libc::sigevent {
sigev_notify: libc::SIGEV_THREAD,
sigev_signo: 0,
sigev_value: libc::sigval {
sival_ptr: 0 as *mut _,
},
_sigev_un: libc::__c_anonymous_sigev_un {
_kevent_flags: 0,
},
};
println!("{:?}", sigevent);
} |
||
_ => &mut ds | ||
} | ||
}; | ||
ds.finish() | ||
} | ||
} | ||
impl ::hash::Hash for sigevent { | ||
fn hash<H: ::hash::Hasher>(&self, state: &mut H) { | ||
self.sigev_notify.hash(state); | ||
self.sigev_signo.hash(state); | ||
self.sigev_value.hash(state); | ||
self.sigev_notify_thread_id.hash(state); | ||
#[cfg(libc_union)] | ||
// The sigev_notify field indicates which union fields are valid | ||
unsafe { | ||
match self.sigev_notify { | ||
::SIGEV_THREAD => self._sigev_un._sigev_thread.hash(state), | ||
::SIGEV_KEVENT => self._sigev_un._kevent_flags.hash(state), | ||
::SIGEV_THREAD_ID => self._sigev_un._threadid.hash(state), | ||
Comment on lines
+1972
to
+1974
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. I think either these impls need to be removed because |
||
_ => () | ||
}; | ||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Deref impl exposes UB to safe code on freebsd.
Tested in miri using rust-lang/miri#2221:
@Amanieu I thought this exact unsoundness issue came up in a recent T-libs-api meeting. Did we end up deciding that this is "okay enough" as a tradeoff? I didn't see it discussed explicitly here on the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that field will never be uninitialized, because no program should ever use both
_sigev_un
andsigev_notify_thread_id
. The latter only exists to provide backwards-compatibility with the pre-union code. So anybody accessing that field should already have initialized the entire structure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that what you said is what programs should do. If the libc crate ensures that programs do as you described, then that's great. If the programmer needs to ensure they do as you described or else they get memory unsafety (which is what is currently implemented in this PR) then we need to make them write
unsafe
somewhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the old, non-union version of
sigevent
is already in the wild. And since it isn'tunsafe
, there simply isn't any way for libc to fix the definition ofsigevent
, be backwards-compatible, and beunsafe
. The deref compromise suggested by @Amanieu is IMHO the best compromise.FWIW there is exactly one currently-maintained crate on crates.io that uses the old
sigev_notify_thread_id
field, and I'm a maintainer of that crate, so I can fix any resulting bugs. It's my intention to release a new version of Nix as soon as libc releases a new version with this PR. However, the backwards-compatibility is still useful for users of older Nix versions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this PR, libc::sigevent cannot be constructed in safe code because it has private padding fields. After this PR it can be constructed as shown above, which is a soundness bug. Preserving the property that it cannot be constructed in safe code is not a breaking change. The safety requirement on constructing a sigevent can be that you never expose it in a way that would mismatch the sigev_notify with the active variant of _sigev_un.
You can use #[non_exhaustive] if libc uses that these days, or just a zero sized private field with appropriate comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we could do that. But it would also have the side effect of making
sigevent
not safely constructable, just to support the backwards-compatibility use-case. That seems like an unnecessary handicap going forward. Would you be satisfied if we added#[deprecated]
to thesigevent_0_2_126
struct and itssigev_notify_thread_id
field? That's probably something we ought to do anyway.