search

fitzgen / mach

A rust interface to the Mach 3.0 kernel that underlies OSX.
https://docs.rs/mach
Other
81 stars 28 forks source link

Fix undefined-behavior in nightly Rust #32

Closed gnzlbg closed 6 years ago

gnzlbg commented 6 years ago

MacOSX uses #pragma pack 4 to pack all of its structs on x86_64 but mach did not.

This resulted in the Rust structs of mach having a different layout than the C structs which is undefined behavior.

This PR fixes this using repr(packed) for nightly Rust builds. It adds a new opt-in feature to the crate called unstable that enables the repr_packed feature and it conditionally applies repr(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: https://github.com/rust-lang/libc/pull/972

gnzlbg commented 6 years ago

@fitzgen @dcuddeback I am a bit wary of using unstable features in mach, but I've tried to compartmentalize here so that we don't break people using stable Rust.

This allows us to test that repr(packed) does indeed solve mach problems, and to allow those on nightly to use this. Once repr(packed) is stabilized we might want to still keep it behind a feature gate to avoid forcing people to update, and once we unconditionally enable it we can just leave the unstable feature as is but doing nothing to avoid breaking other people's script that might be passing that explicitly.

gnzlbg commented 6 years ago

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 master, and I would prefer to do a release after that, but I'd rather bump the version to mach 0.2 since there have been a couple of breaking changes on master.

AFAICT all the changes on master have fixed one form or another of undefined behavior (structs that were not repr(C) but should have been, incorrect types on extern C function signatures, etc.), so they could qualify for a new 0.1.x release, but I'd rather not risk breaking anybody and go straight to 0.2.

Thoughts?

fitzgen commented 6 years ago

I think it is fine to go straight to a 0.2 series.

fitzgen commented 6 years ago

I thought that stable repr(packed) was riding the release trains now -- I guess that isn't the case?

gnzlbg commented 6 years ago

Ok! repr(packed) is stable since 1.0, but repr(packed(N)) is not :/

gnzlbg commented 6 years ago

So https://github.com/rust-lang/libc/pull/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.

fitzgen commented 6 years ago

Sounds good!

fitzgen commented 6 years ago

Probably want to update the table in the README with the new rust version too.

gnzlbg commented 6 years ago

@fitzgen done #33