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

The behavior of returning a value of a type containing an explicit padding field from C into Rust might be undefined #1453

Open
gnzlbg opened this issue Aug 2, 2019 · 4 comments
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. E-medium E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. I-unsound 💥 A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness
Milestone

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 2, 2019

libc uses integers and aggregates of integers for explicit padding byte fields. When these fields are returned from C, they might be uninitialized. We currently do not have any wording in the spec saying that an uninitialized integer is not instant undefined behavior, and there are proposals that define them to both be ok, and to actually be instant undefined behavior.

These padding fields should use MaybeUninit<INT> instead.

The right way to actually fix this issue is to actually remove these padding fields. They only exist because repr(align(N)), repr(packed(N)), unions, etc. did not exist when these types were added. For toolchains that actually support MaybeUninit, we can actually do better and don't use any kind of padding fields at all. For toolchains that do not support repr(align(N)), repr(packed(N)), unions, etc. MaybeUninit does not exist and cannot be actually implemented in Rust, so there is nothing we can do.

For C types using FAM, there is no way to avoid undefined behavior in Rust today, so we have to use explicit padding fields.

Removing these fields or changing their type are major breaking changes when these fields are public. Most of these fields are private, but some of them aren't.

Rust currently does not exploit this potential undefined behavior for anything, and for most targets, it cannot actually exploit it since the C library is dynamically linked or linked without cross-lang-lto. If we start using cross-lang-lto, e.g., when statically-linking musl, this could become an issue in the future.

@gnzlbg gnzlbg changed the title Padding fields passed to C should be MaybeUninit The behavior of returning a value of a type containing an explicit padding field from C into Rust might be undefined Aug 2, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 2, 2019

cc @emilio - I think rust-bindgen probably has the same issues when it comes to automatically-generated explicit padding fields. The constraints of fixing that there are probably the same as the constraints of fixing that here: where MaybeUninit is available, one can probably do better and not emit padding fields in the first place, and where MaybeUninit is not available, there might not be a way to avoid the UB anyways (e.g. depending on how old the Rust version being targeted is). For FAMs, I don't think rust-bindgen can do anything either.

@emilio
Copy link

emilio commented Aug 2, 2019

Yes, that's right. I've opened rust-lang/rust-bindgen#1606 to track this.

@Demi-Marie
Copy link

I don’t believe that this is likely to cause problems in practice: C code in system libraries is not visible to the Rust compiler, so rustc has no way to know that these fields are uninitialized. bindgen has a bigger problem because it may be used with LTO.

@glandium
Copy link
Contributor

C code in system libraries is not visible to the Rust compiler, so rustc has no way to know that these fields are uninitialized.

Actually, that's not true with linker-plugin-lto.

@tgross35 tgross35 added the I-unsound 💥 A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Aug 29, 2024
@tgross35 tgross35 added this to the 1.0 milestone Aug 29, 2024
@tgross35 tgross35 added the E-medium E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Aug 29, 2024
@tgross35 tgross35 added the E-help-wanted Call for participation: Help is requested to fix this issue. label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. E-medium E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. I-unsound 💥 A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness
Projects
None yet
Development

No branches or pull requests

5 participants