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

Zero out padding in custom Default trait implementations #2051

Merged
merged 2 commits into from May 11, 2021

Conversation

danobi
Copy link
Contributor

@danobi danobi commented May 8, 2021

Previously, we were using std::mem::zeroed() which unfortunately does
not necessarily zero out padding. It'd be better if the padding is
zeroed out because some libraries are sensitive to non-zero'd out bytes,
especially when forward/backward compatability is involved.

This commit ensures all bytes are zeroed out in custom Default trait
implementations.

This closes #2050 .

Note that I kept these commits separate to help with review. They should
probably be squashed before merge so tests are still bisectable.

@highfive
Copy link

highfive commented May 8, 2021

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks good. There are some libclang-version-specific tests that need updating (CI is red because of that), but with the bit below addressed and those updated this looks great.

src/codegen/mod.rs Outdated Show resolved Hide resolved
src/codegen/mod.rs Show resolved Hide resolved
Previously, we were using `std::mem::zeroed()` which unfortunately does
not necessarily zero out padding. It'd be better if the padding is
zeroed out because some libraries are sensitive to non-zero'd out bytes,
especially when forward/backward compatability is involved.

This commit ensures all bytes are zeroed out in custom Default trait
implementations.
@danobi
Copy link
Contributor Author

danobi commented May 8, 2021

Waiting for the CI to run again so I can just copy paste the diffs from other clang versions

@danobi
Copy link
Contributor Author

danobi commented May 8, 2021

I think I fixed up the fixtures. Another CI run should confirm

@danobi
Copy link
Contributor Author

danobi commented May 10, 2021

Bump for CI

@danobi
Copy link
Contributor Author

danobi commented May 10, 2021

Getting close! Bumping again

@emilio
Copy link
Contributor

emilio commented May 10, 2021

I really hate that github policy of having to explicitly approve CI runs. I did plan to fix this up and merge it whenever I got a chance, but if you finish it up that's even better of course, thanks! :)

@emilio
Copy link
Contributor

emilio commented May 10, 2021

(I should also figure out a way to make runs not stop at the first failure...)

@danobi
Copy link
Contributor Author

danobi commented May 10, 2021

If you want to merge as-is and fixup later, that'd work for me too.

@emilio
Copy link
Contributor

emilio commented May 10, 2021

No, I meant pulling your changes, fix up as needed myself, and push with the fixes. I may be able to do it tomorrow if you don't get it green by then, otherwise later this week.

@danobi
Copy link
Contributor Author

danobi commented May 10, 2021

Woohoo! Got all of them

@emilio emilio merged commit e6684dc into rust-lang:master May 11, 2021
@emilio
Copy link
Contributor

emilio commented May 11, 2021

Sweet, thanks!

LoganBarnett pushed a commit to LoganBarnett/rust-bindgen that referenced this pull request Dec 2, 2023
)

* Zero out padding in custom Default trait implementations

Previously, we were using `std::mem::zeroed()` which unfortunately does
not necessarily zero out padding. It'd be better if the padding is
zeroed out because some libraries are sensitive to non-zero'd out bytes,
especially when forward/backward compatability is involved.

This commit ensures all bytes are zeroed out in custom Default trait
implementations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Padding not generated
3 participants