Fix undefined-behavior in nightly Rust #32
Conversation
|
@fitzgen @dcuddeback I am a bit wary of using unstable features in This allows us to test that |
|
So @fitzgen @dcuddeback after figuring out the minimum Rust version required for this change (Rust 1.13.0) that does not seem that bad - we currently support Rust 1.11.0 though. I think this can be merged as is to AFAICT all the changes on master have fixed one form or another of undefined behavior (structs that were not Thoughts? |
| @@ -45,7 +45,7 @@ fi | |||
|
|
|||
| # Runs ctest to verify mach's ABI against the system libraries: | |||
| if [[ -z "$NOCTEST" ]]; then | |||
| if [[ $TRAVIS_RUST_VERSION == "beta" ]] || [[ $TRAVIS_RUST_VERSION == "nightly" ]]; then | |||
| if [[ $TRAVIS_RUST_VERSION == "nightly" ]]; then | |||
There was a problem hiding this comment.
The ctest now only pass on nightly because they require repr(packed) to pass. Before they used to pass on beta as well because we just ignored the undefined behavior on packed structs. I think it is more important for CI to catch these issues unconditionally than to just check beta.
| @@ -5,7 +5,7 @@ authors = ["gnzlbg <[email protected]>"] | |||
| build = "build.rs" | |||
|
|
|||
| [dependencies] | |||
| mach = { path = ".." } | |||
| mach = { path = "..", features = ["unstable"] } | |||
There was a problem hiding this comment.
ctest does not white-list structs with wrong layouts anymore, so without repr(packed) it would fail. This should catch on CI that newer types introduced in the future have at least the proper layout on nightly.
| @@ -2,6 +2,7 @@ | |||
| #![allow(non_upper_case_globals)] | |||
|
|
|||
| #![cfg_attr(not(feature = "use_std"), no_std)] | |||
| #![cfg_attr(feature = "unstable", feature(repr_packed))] | |||
There was a problem hiding this comment.
packed is opt-in, users compiling with nightly need to explicitly opt-into it, this allows the library to keep working as is on stable Rust.
|
I think it is fine to go straight to a |
|
I thought that stable repr(packed) was riding the release trains now -- I guess that isn't the case? |
|
Ok! |
|
So rust-lang/libc#972 was merged in libc, bumping the minimum supported libc Rust version to 1.13.0 as well. This PR does the same, syncing mach with libc. I've added a new commit bumping the crate version to 0.2, and just published it. |
|
Sounds good! |
|
Probably want to update the table in the README with the new rust version too. |
MacOSX uses
#pragma pack 4to pack all of its structs onx86_64butmachdid not.This resulted in the Rust structs of
machhaving a different layout than the Cstructswhich is undefined behavior.This PR fixes this using
repr(packed)for nightly Rust builds. It adds a new opt-in feature to the crate calledunstablethat enables therepr_packedfeature and it conditionally appliesrepr(packed(N))to all structs with incorrect layout.Note: the stable Rust build bots currently fail because Rust 1.11.0 is too old: it does not have attribute literals. I've made a similar PR to libc where the exact same thing happened. I'll wait till that is merged and will bump our stable Rust version to continue to match that of libc: rust-lang/libc#972