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

Padding Bytes Guarantees? #174

Closed
jswrenn opened this issue Jul 24, 2019 · 33 comments · Fixed by #195
Closed

Padding Bytes Guarantees? #174

jswrenn opened this issue Jul 24, 2019 · 33 comments · Fixed by #195
Labels
A-padding Topic: Related to padding C-open-question Category: An open question that we should revisit

Comments

@jswrenn
Copy link
Member

jswrenn commented Jul 24, 2019

The context of this issue is determining when it is safe for a struct to implement FromZeros (à la zerocopy::FromBytes), a marker trait implemented for a type T iff any sequence of initialized zeroed bytes of length size_of::<T>() is a valid instance of T. (For such types, mem::zeroed is safe!)

Per the reference:

The representation of a type can change the padding between fields, but does not change the layout of the fields themselves.

Therefore: for a struct T to be FromZeros, the fields of T must also be FromZeros.

I'm trying to determine whether this requirement alone is sufficient.

My understanding is that the padding bytes between fields is expressly undefined (i.e., it can be anything, including uninitialized). Will Rust ever rely on padding having a particular value?

If so, for a struct T to be FromZeros, it must have no padding, as a 0u8 might not correspond to a valid padding byte for T. This poses a very severe limitation on what structs could be FromZeros (and, by extension, FromBytes).

@Lokathor
Copy link
Contributor

I also use a similar unsafe marker to make safe zeroing https://docs.rs/lokacore/0.1.0/lokacore/trait.Zeroable.html, so I suppose this applies to me as well.

@RalfJung
Copy link
Member

RalfJung commented Jul 24, 2019

Indeed, this should be properly guaranteed somewhere, at some point.

My current understanding is that, for structs, your condition is sufficient: in our current implementation, padding bytes can and do change as the struct gets copied, so rustc cannot rely on padding having a particular value.

@jswrenn
Copy link
Member Author

jswrenn commented Jul 25, 2019

in our current implementation

Part of why I'm cautious is that I can totally imagine optimizations that are enabled by assuming a particular value. For instance, if you assume all padding of a type T has a particular value, you can speed up equality checks between two instances of T by loading them in their entirety into registers and checking for equality (rather than checking field-by-field).

Making this guarantee will rule out performing that optimization. (That said, I think the benefits of that optimization pale in comparison to the sort of optimizations enabled by zerocopy::FromBytes!)

@RalfJung
Copy link
Member

RalfJung commented Jul 25, 2019

@jswrenn absolutely agreed. That's why I said current implementation. (I added emphasis on that now.)

@comex
Copy link

comex commented Jul 31, 2019

Assuming anything about padding bytes would make it impossible to use MaybeUninit to initialize structs field-by-field, even once raw-pointer-to-field is fixed.

@RalfJung
Copy link
Member

See oconnor663/shared_child.rs#17: it would also make it impossible to initialize a struct with mem::zeroed (unless the padding assumption is "all 0").

So indeed I find it hard to imagine that we'd ever rely on padding having a particular value, or being preserved, or anything like that. If you want to be really sure, I'd expect this to be a rather uncontroversial RFC; the main issue is wording this precisely enough.

@jswrenn
Copy link
Member Author

jswrenn commented Jul 31, 2019

I drafted an RFC around the time I filed this issue, but put it on hold for typelayout. The field-by-field initialization case is especially compelling. I'll make a PR in the next day or so. :)

@oconnor663
Copy link

And the key issue with oconnor663/shared_child.rs#17 is that I think a lot of structs in libc can only be initialized with something like mem::zeroed, as far as I can tell, because they have both private fields and no public constructors.

@jswrenn
Copy link
Member Author

jswrenn commented Jul 31, 2019

@oconnor663 I've added a section about this to my RFC's motivation section and filed an issue on libc documenting the problem: rust-lang/libc#1451

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 31, 2019

Assuming anything different from C about padding bytes for repr(C) types would make them unusable in C FFI:

let mut x: repr_c_type = c_ffi_fn();
c_ffi_fn2(&mut x);

When passing a repr_c_type value by value from C to Rust the padding bytes might contain anything that C allows them to contain, including undef. When passing a pointer to repr_c_type to C, C can dereference the pointer and write to it, writing anything that C allows to the padding bytes. It can also read from the pointer, which means that padding bytes in the Rust side must have a value that C supports.

AFAICT, this sub-section of the nomicon (https://doc.rust-lang.org/nomicon/ffi.html#interoperability-with-foreign-code) guarantees that repr(C) types, including their padding, are C compatible.

We could tune what values padding type supports as long as doing so does not break the C FFI use case.

@jswrenn
Copy link
Member Author

jswrenn commented Jul 31, 2019

@gnzlbg I'm convinced that the C FFI guarantees imply that Rust must never assume that padding bytes for #[repr(C)] datatypes have a particular value (and that a new RFC is not necessary for this). AFAIK, types in libc are repr(C), so zeroed initialization is safe.

However, there is still no such guarantee for repr(Rust) types, right?

@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2019

@gnzlbg raised the point that mem::zeroed does not actually guarantee that the returned value has 0s in its padding bytes -- indeed, padding bytes are not preserved when returned from a function, and that includes mem::zeroed.

This still leaves the question of whether it is okay to transmute an all-0 integer slice -- or any integer slice -- to a struct involving padding (assuming the fields themselves are okay with whatever the integer values at their offsets are). Everyone seems to strongly agree that the answer is "yes, when creating a value of struct type you can have any data in the padding". I think the only open question is whether this needs an RFC, and where this should be documented.

Is that an accurate summary of the current status?


As pointed out before, for repr(C) struct this likely does not need an RFC because we should follow C rules here. That leaves our own Rust-specific reprs. We basically have two options:

  • Permit the compiler to make assumptions about padding. This would have the big advantage of letting us use padding for niches. The big disadvantage is that we have to preserve those bytes on copy then, so we'd have to change the way things compile to LLVM, which could become very tricky.
  • Allow any value for padding, and do not preserve padding on copies. This would have the big advantage of being more familiar, and matching our current implementation.

However, even if we end up wanting to use padding as niches, I strongly feel we should make sure that "0" is not considered a niche, to keep mem::zeroed working for such types.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 2, 2019

I don't remember exactly what the issues where with using the padding bytes of a repr(Rust) T for niches (cc @eddyb ), but I don't see why typed copies of T cannot set those bytes to whatever value the Rust implementation needs.

@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2019

I don't see why typed copies of T cannot set those bytes to whatever value the Rust implementation needs.

Consider Option<(u8, u16)> storing the "option" flag in the padding of the tuple. When storing another tuple into this option, we have to make sure the existing padding is preserved -- so I actually got it wrong above, to use padding for niches, the compiler would have to ensure that we never copy padding.

@hanna-kruppe
Copy link

so I actually got it wrong above, to use padding for niches, the compiler would have to ensure that we never copy padding.

Worse, it's not just the compiler, as users can and do write unsafe code that copies all size_of::<T>() bytes with memcpy/ptr::copy*/etc. so we have a terrible backwards compatibility hazard there, similar to the stride!=size proposals.

@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2019

Yeah I think I agree.

So probably the only open question here is whether it is worth having an RFC to set this in stone, or if we can just submit PRs to appropriate sections of Reference and Nomicon.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 2, 2019

Consider Option<(u8, u16)> storing the "option" flag in the padding of the tuple. When storing another tuple into this option, we have to make sure the existing padding is preserved

So given:

let mut x: Option<(u8, u16)> = Some(y);
let mut ref: &mut (u8, u16) = x.as_mut().unwrap();
*ref = (42, 42);

The write through *ref would need to never copy padding to preserve the discriminant of x. I think this would be achievable.

Worse, it's not just the compiler, as users can and do write unsafe code that copies all size_of::() bytes with memcpy/ptr::copy*/etc. so we have a terrible backwards compatibility hazard there, similar to the stride!=size proposals.

We can't do anything about untyped memcpys, and I agree that users can and do write this code. I don't know whether users that are writing this code today are relying on unspecified or undefined behavior or not. Even if they are, I don't know how much code this would break, potentially a lot of code.

Do we have an example of something users are already doing here that we just cannot break ? I imagine that something like abomonation might be using untyped memcpys for fast serialization, but abomonation is quite clear that it might not be a sound abstraction. Does serde do something like this as well?

@hanna-kruppe
Copy link

hanna-kruppe commented Aug 2, 2019

"Untyped" memcpys are foundational, and for good reasons. Even just in std, just off the top of my head, mem::swap (and everything built on it) boils down to one or multiple of them and core data structures such as Vec and VecDeque use them heavily. It would be preposterous to suddenly start wagging our fingers at users doing the same -- extraordinarily reasonable -- thing. Furthermore, our codegen generates memcpy calls everywhere, even for typed copies, and we'd lose all memcpy optimizations and a sizable chunk of performance for copies of big types if we stopped doing that. Banning memcpy and its variants is simply infeasible for a systems language.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 2, 2019

@rkruppe

Even just in std, just off the top of my head, mem::swap (and everything built on it) boils down to one or multiple of them and core data structures such as Vec and VecDeque use them heavily.

Why does mem::swap(&mut T, &mut T) perform untyped memcpys ? AFAICT this is a bug that would unnecessarily copy padding when swapping #[repr(align(128))] struct S(u8);. For example, C++ std::swap does perform typed copies.

How would the untyped memcpys inside Vec be broken if we were to make this change ? AFAICT, those untyped memcpys would still be correct for both Vec<(u8,u16)> and for Vec<Option<(u8, u16)>> because they would copy padding, and hence, the discriminant in the case of the Option.

@hanna-kruppe
Copy link

It's less that e.g. Vec specifically would be broken, and more that the message "you can use memcpy(-like functions) for Rust types, don't worry" is deeply, deeply ingrained in code and in peoples' understanding of Ruts. Using padding for niches means that this is no longer true, and that's pretty much end of story. There might be circumstances where one can be sure nothing important is in the padding, but how would we even formalize this, let alone make people understand this & adhere to it everywhere? Who audits every use of ptr::copy etc.? I can't seriously view this thought experiment as anything less than effectively trying to abolish untyped copies entirely.

At least with the stride/size distinction, you can still use memcpy and friends as long as you calculate the right size to not clobber "trailing padding" (which may in fact not be padding but another field of a surrounding struct). That's difficult/impossible in today's Rust, but for entirely self-imposed, historical reasons. However, if internal padding must not be clobbered, then memcpy (indeed, any function optimized for large bulk copies without holes) is unusable, and congratulations, you just made systems programming even more hellish. As @RalfJung put it recently in another thread about padding:

It adds UB in very non-trivial raw-byte-level memory-manipulating code. I am already extremely worried about the kind of rules Stacked Borrows will impose on such code, but there at least we have a very strong motivation. But what you are proposing here goes way beyond that, it IMO makes it a lot harder to think about what happens when working with raw pointers in Rust, and the gain is nowhere near what our aliasing rules are giving us.

PS:

Why does mem::swap(&mut T, &mut T) perform untyped memcpys

Trailing padding aside, for large types it uses an algorithm that swaps in fixed-size chunks. This caps the size of the temporary buffer needed. Taking care to preserve internal padding would greatly complicate that code and slow it down, and even if you did that, it can't be phrased as a typed copy without changing the space usage for the worse.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 2, 2019

Furthermore, our codegen generates memcpy calls everywhere, even for typed copies, and we'd lose all memcpy optimizations and a sizable chunk of performance for copies of big types if we stopped doing that. Banning memcpy and its variants is simply infeasible for a systems language.

This is a good point.

It is not completely clear to me that this is a net negative. For example, we currently emit a memcpy for 6 bytes for Option<(u8, u16)> and this change would make that Option 4-bytes wide and still allow us to emit a single memcpy for it.

How would the untyped memcpys inside Vec be broken if we were to make this change ?

This is an answer to myself, but @rkruppe is right in that these are broken. For example, consider Option<[(u8, u16); N]>::Some(&mut x: &mut [(u8, u16); N]), if we want to exploit the padding byte for an enum layout optimization, this means that we can't copy the whole array when writing to a &mut [(u8, u16); N] using a single memcpy, because that would overwrite the discriminant of the Option. The copy can happen through a &mut [(u8, u16)] as well, which can be used to overwrite some of the element of any slice, including Vec. So while Vec itself might be able to use untyped memcpys internally, code interfacing with a Vec via a [T] would need to use typed copys and that would be horrible.

@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2019

For example, consider Option<[(u8, u16); N]>::Some(&mut x: &mut [(u8, u16); N]), if we want to exploit the padding byte for an enum layout optimization, this means that we can't copy the whole array when writing to a &mut [(u8, u16); N] using a single memcpy, because that would overwrite the discriminant of the Option.

Oh my, yes. You just convinced me even more that we should not do this.^^

@RalfJung RalfJung added C-open-question Category: An open question that we should revisit A-uninitialized Topic: Related to uninitialized memory A-padding Topic: Related to padding and removed A-uninitialized Topic: Related to uninitialized memory labels Aug 14, 2019
@manuthambi
Copy link

manuthambi commented Mar 15, 2020

Not sure I understood the reasoning here. If Rust guarantees that padding bytes are zero. Then we should be able to memcpy them everywhere including across &mut pointers, compare them fast as @jswrenn mentioned. We can also use them for enum layout optimization as long as Some version has the discriminant as 0.

As mentioned here, Rust already uses non-zero discriminant for None because
size_of::<Option<(u16, u8, bool)>>() == 4

I guess we still have to somehow deal with initializing a struct through MaybeUninit. But that seems solvable by having some compiler intrinsic which fills in the padding bytes?

@RustyYato
Copy link

To go off of @manuthambi, we could introduce a type like this:

#[repr(u8)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
enum ZeroPadding {
    Zero = 0,
}

The only valid value is zero, so it is still safe to call mem::zeroed(), but no other value is safe. You can replace one of the padding bytes with this to allow niches, and nothing else. (This way you don't need to wonder why there is a random bool in the type).

@RalfJung
Copy link
Member

RalfJung commented Mar 15, 2020

If Rust guarantees that padding bytes are zero

But how would we even guarantee that? LLVM doesn't give us a way to do this, as far as I know.

But also, I fail to see the connection to the original question here. That question was "is it allowed to set padding bytes to 0". The answer is "in current implementations, yes" and "most likely this will also be part of an eventual spec".

@manuthambi
Copy link

manuthambi commented Mar 15, 2020

But how would we even guarantee that?

I am not familiar with rustc-llvm interface, but along the lines of what @KrishnaSannasi said, can't we just not tell LLVM that it is padding? Just mark it as a valid byte with a 0 value.

I am worried that if we declare padding bytes can be anything, then unsafe code will be written assuming that, and we will close off these optimizations forever.

@Diggsey
Copy link

Diggsey commented Mar 15, 2020

Regarding MaybeUninit and unions: the compiler would have to write the padding bytes to zero in a way such that when every field in a struct is assigned, all padding is also initialized.

This could be done by either picking one field to "own" the padding bytes, so when that field is written, all of the padding is also set to zero, or each field could "own" the padding which follows it, so when a field is written the compiler must also write zeros to the following padding,

This works, but only if #[repr(Rust)] is never stabilized: if the layout of types were ever stabilized then it would be possible to initialize one "variant" of a union and then use that as the basis for partially initializing another variant. In that case, the compiler would never be able to guarantee that all the padding bytes are zero, and we can't put that burden on the user unless we made some kind of intrinsic for clearing padding bytes.

@manuthambi
Copy link

The language in the unsafe code guidelines here allowing copying Pad to result in bytes with any value is going to be problematic.

@RalfJung
Copy link
Member

can't we just not tell LLVM that it is padding? Just mark it as a valid byte with a 0 value.

At least for repr(C), no, we cannot. That could affect how data is passed by-value across function boundaries. Also we have to be able to work with arbitrary padding in data when interfacing with C.

For repr(Rust), maybe. This still remains entirely off-topic for this thread.

@RalfJung
Copy link
Member

Okay I think I understood what you were going for in these comments. I think you were suggesting we change the way Rust types are laid out, and introduce a "different kind of padding". As I said that's unrelated to UCG, which is tasked with describing the current behavior and suggesting rules (that could become normative through an RFC) for how that behavior is permitted to change in the future.

I opened a rustc issue for the suggestion: rust-lang/rust#70230

@manuthambi
Copy link

Exactly! Thanks for opening the new issue :)

@QuineDot
Copy link

So probably the only open question here is whether it is worth having an RFC to set this in stone, or if we can just submit PRs to appropriate sections of Reference and Nomicon.

FYI, memcpy of [T: Copy], and by implication of T: Copy, were blessed by RFC (discussion) and stabilized in 2016.

@RalfJung
Copy link
Member

Yeah but that doesn't say anything about whether more "lossy" ways of copying would also work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-padding Topic: Related to padding C-open-question Category: An open question that we should revisit
Projects
None yet
Development

Successfully merging a pull request may close this issue.