Skip to content
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 undefined-behavior on MacOSX structs in stdbuilds #972

Merged
merged 3 commits into from
Apr 17, 2018

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Apr 15, 2018

Some MacOSX structs have an incorrect layout that results in undefined behavior. This is because on x86_64 the MacOSX kernel headers define these using #pragma pack 4.

This PR fixes their layout using repr(packed(4)) . Since it is only available on nightly, it is only enabled for stdbuilds .

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 15, 2018

@alexcrichton this would require updating the stable Rust version used in CI. Is there a policy about that?

@alexcrichton
Copy link
Member

What would it require updating to?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 15, 2018

I think attr_literals but I'll bisect the first version that works.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 15, 2018

@alexcrichton we would need to update from 1.0.0 to 1.13.0 meaning newer libc versions wouldn't compile with older compilers anymore. This is pretty much a backwards incompatible change, might need to change libc from 0.2 to 0.3 :/

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 15, 2018

An alternative could be tuning repr(packed) syntax to something that is compatible with 1.0, maybe instead of repr(packed(N)) we could add repr(packed1), repr(packed4), etc.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 15, 2018

@alexcrichton I can also put these two structs into two different files, and conditionally include one or the other depending on the unstable cargo feature.

@alexcrichton
Copy link
Member

Nah Rust 1.13.0 is ancient at this point (aka pre-stable-Serde) so I doubt anything before that is still in use. If you'd like to raise the minimum version in CI to 1.13.0 I think we'll be good to go!

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 16, 2018

@alexcrichton done !

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 17, 2018

📌 Commit 77837a0 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Apr 17, 2018

⌛ Testing commit 77837a0 with merge 69769fb...

bors added a commit that referenced this pull request Apr 17, 2018
Fix undefined-behavior on MacOSX structs in stdbuilds

Some MacOSX structs have an incorrect layout that results in undefined behavior. This is because on `x86_64` the MacOSX kernel headers define these using `#pragma pack 4`.

This PR fixes their layout using `repr(packed(4))` . Since it is only available on nightly, it is only enabled for stdbuilds .
@bors
Copy link
Contributor

bors commented Apr 17, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 69769fb to master...

@bors bors merged commit 77837a0 into rust-lang:master Apr 17, 2018
lambda-fairy added a commit to lambda-fairy/rust-errno that referenced this pull request Jul 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants