Remove new field from ucontext_t for compatibility with earlier glibc versions #1411
Conversation
Per discussion in rust-lang#1410, this is necessary to avoid struct size mismatches between Rust and C on systems with glibc < 2.28.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| // ucontext_t added a new field as of glibc 2.28; our struct definition is | ||
| // conservative and omits the field, but that means the size doesn't match for newer | ||
| // glibcs (see https://github.com/rust-lang/libc/issues/1410) | ||
| "ucontext_t" if gnu => true, |
There was a problem hiding this comment.
Could you add the following FIXME here?
FIXME: remove once Ubuntu 20.04 LTS is released, somewhere in 2020.
There was a problem hiding this comment.
Done, though I'm a little concerned about the timeline given that 18.04 LTS is supported until 2023. Is there a stated policy for how long this crate supports a given glibc version?
There was a problem hiding this comment.
Not really. We tend to support a "sufficiently" old version, adding features of newer glibc versions on top. The glibc version supported differs per target, it's all a huge mess.
There was a problem hiding this comment.
Hah, fair enough. It would be nice if we could trigger a load-time failure for too-old versions of glibc, as the original bug that triggered my report was pretty difficult to track down. I've seen programs fail to dynamically load before, asking for a particular glibc version, but I don't know how we could introduce that behavior. Either way, something for another PR probably.
| @@ -285,7 +285,6 @@ s_no_extra_traits! { | |||
| pub uc_mcontext: mcontext_t, | |||
| pub uc_sigmask: ::sigset_t, | |||
| __private: [u8; 512], | |||
| __ssp: [::c_ulonglong; 4], | |||
There was a problem hiding this comment.
Instead of removing the field, can you comment it out, and add a comment about why it is commented out ? For example:
// FIXME: the shadow stack field requires glibc >= 2.28.
// Re-add once we drop compatibility with glibc versions older than 2.28.
// __ssp: [::c_ulonglong; 4],
|
@bors: r+ |
|
📌 Commit 339fe22 has been approved by |
|
💔 Test failed - checks-travis |
|
@bors: r+ |
|
📌 Commit e94fffc has been approved by |
|
💔 Test failed - checks-travis |
|
@bors: r+ |
|
📌 Commit f6e48fc has been approved by |
|
☀️ Test successful - checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-travis, status-appveyor |
Per discussion in #1410 with @gnzlbg, this is necessary to avoid struct size mismatches between Rust and C on systems with glibc < 2.28.