RFC: add freeze operation

[honzasp]

View all comments

Introduce an operation similar to the LLVM freeze instruction, which converts uninitialized values into initialized but arbitrary values:

impl<T> MaybeUninit<T> {
    const fn freeze(self) -> MaybeUninit<T>;
}

The biggest disadvantage of adding the freeze operation is that a Rust program can leak a secret that was previously stored in the uninitialized memory without triggering UB.

Important

Since RFCs involve many conversations at once that can be difficult to follow, please use review comment threads on the text changes instead of direct comments on the RFC.

If you don't have a particular section of the RFC to comment on, you can click on the "Comment on this file" button on the top-right corner of the diff, to the right of the "Viewed" checkbox. This will create a separate thread even if others have commented on the file too.

Rendered

[Lokathor]

Hello. Yes, I'd merge this PR for sure.

View changes since the review

[RalfJung]

This sounds very good overall, thanks a lot :)
Cc @rust-lang/opsem

View changes since this review

[RalfJung]

Cc @nikic -- do you think it makes sense to add a flag to LLVM's memcpy that indicates a freeze? Ideally we'd not have to hand-roll our own freezing memcpy in Rust, that seems a bit silly. We want to use the regular highly-optimized memcpy primitive, just in a way that LLVM considers to be freezing all undef/poison.

[RalfJung]

I don't find this very convincing. We don't usually name things after the LLVM IR operation they compile to.

I think it should be called frozen.

[hanna-kruppe]

In general, we don't have to make things UB to consider them wrong and try to prevent them. We also have the concept of erroneous behavior (EB), where the program is considered buggy and can be aborted if e.g. run under Miri or a sanitizer, but otherwise has well-defined behavior (or at least not UB).

Could we have EB for at least some subset of Rust programs that leak uninitialized memory? It makes no sense to consider every use of freeze that sees an uninit byte to be EB, if we want that then we shouldn't add freeze to begin with. But tools like Valgrind and MemorySanitizer can diagnose specific uses of uninitialized memory that are likely bugs, e.g., branching on a condition or dereferencing a pointer derived from uninitialized memory.

Is there a way to specify EB that blesses (roughly) the kind of checks those tools perform? That seems like it would provide a decent compromise (it rules out use case 1 but still allows other use cases). Unfortunately I don't see an easy way to do it:

• A simple operational semantics would be a naive "taint tracking" model where bytes that were uninit and got frozen are still recorded as being "tainted" by uninit-ness, and this is propagated though essentially every operation on values, and certain operations on tainted values/memory (e.g., branching or outputting) are EB. However, this disallows use case 2 where the offending bits are masked out, and likely other use cases as well.
• To be smarter about when the "taint" of uninit-ness can be safely considered defused, one could try to do "possible values of non-det choice" reasoning like LLVM's undef (e.g., undef & 1 is either 0 or 1 and (undef & 1) >> 1 is always 0). However, this seems very hard to reason about and and possibly makes some desirable compiler optimizations illegal (w.r.t. not introducing EB).
• A more teleological definition would be that there is EB if the observable behavior of the program depends on the non-deterministic choices made by freeze operations. However, this is impossible to implement, and allows some programs that sanitizers will flag as using uninitialized memory.

View changes since the review

[RalfJung]

"program leaks secrets" is not something that you can define as a property of an AM execution, so I don't think we can have Miri detect this or call it EB. (Formally it's a hyperproperty, you need to define a notion of "public"/"secret" data and then compare two runs of the program to determine that a secret was leaked.)

A simple operational semantics would be a naive "taint tracking" model where bytes that were uninit and got frozen are still recorded as being "tainted" by uninit-ness,

I think you are inventing provenance for integers. Please, let's not.

[hanna-kruppe]

I'm well aware that "leaks secrets" can't be operationalized at AM level, and of other challenges. I'm wondering whether there is some property of an AM execution that we can define, which is somewhat related to improper use of uninitialized memory, and useful as EB: doesn't rule out any important use cases, but diagnoses some obviously buggy programs. Tools like Valgrind and MemorySanitizer are useful and already don't complain about some of the things that freeze would allow doing. It would be a shame if we had to essentially turn them off completely around any use of freeze.

Maybe the fact that freeze is opt-in is good enough to still catch all the same bugs in practice. But it's not obvious to me.

[RalfJung]

It would be a shame if we had to essentially turn them off completely around any use of freeze.

I can't think of anything better. Any way of distinguishing the result of freeze from a normal integer amounts to essentially a form of provenance on integers, and that's too big a hammer for this IMO.

[programmerjake]

imo whatever we pick should support stuff like Atomic::<(u8, u16)>::compare_exchange which needs to be able to freeze the padding bytes (probably using MaybeUninit<[u8; 4]>) and have them turn into normal bytes that don't report errors because you had to compare them in your cmpxchg loop.

[RalfJung]

Please let's not scope creep. Atomic on types with padding has a bunch of extra complications.

[programmerjake]

I'm not saying Rust should implement Atomic for types with padding, but that with freeze a user library could.

[RalfJung]

Ah I see. Yeah that may be possible, but one has to be careful in from_mut (it probably has to freeze padding).

[RalfJung]

Looks like all the first-round feedback was handled. Let's nominate this for t-lang.

[honzasp]

To be honest, I am now even less convinced that adding freeze is a good idea. If we look at the use cases from the RFC:

1. memcpy() serialization: doing this is almost always a very bad idea, because it can leak content of uninitialized memory, including secrets.
2. Masked reads: as @SkiFire13 pointed out, this can already be solved by overwriting the uninitialized padding bytes with zeros, and LLVM seems to be able to optimize this out to the exact same code as with freeze + mask.
3. C bitfields: this is an edge case that's only relevant if we access C bitfields and compile with LTO, and according to @RalfJung we can't guarantee that the freeze() function will do the right thing anyway. (And even though C bitfields may cause UB on the LLVM level, this doesn't seem to cause any problems in practice.)
4. Sparse set and other clever data structures: this is a fairly niche application, and it can be implemented safely with inline assembly or FFI.

On the other hand, we lose the property that safe Rust can never leak values of uninitialized memory without invoking UB or using inline assembly or FFI. This is not a theoretical concern, this opens a whole class of security issues.

(It might look a bit weird that I'm arguing against an RFC that I wrote, but my main aim was to resolve this question: either decide to add freeze, or decide that this operation will never be added to the language.)

[programmerjake]

another use-case for freeze: allow optimizing Simd operations -- when LLVM gains llvm.speculative.load and Rust gains a function that uses that, it can be used to load a Simd even if it might go beyond the end of the input buffer, freezing that allows you to do your simd operations without needing masking until the end, potentially running faster that way.

[RalfJung]

Possible motivations, some were already mentioned:

• SIMD where some lanes are "garbage"
• atomics on arbitrary types of small size, including types that have padding and even unions
• More general version of the previous point: Soundly deal with padding of data of arbitrary type where we cannot just manually reset the padding to 0. (E.g., implement a memcmp that's never UB, even if it may produce odd behavior for some data.)
• Data structures such as the sparse set
• As a "story" for in-memory freeze via inline asm

I feel like we have seen more use cases over the years. Cc @chorman0773 @rust-lang/opsem

None of these are Earth-shattering on their own but it adds up.

[jplatte]

Regarding use cases, would this allow implementing seqlock w/o inline assembly? I vaguely remember that it has similar problems as the mentioned Atomic<T> use case, but maybe not the exact same.

[RalfJung]

Regarding use cases, would this allow implementing seqlock w/o inline assembly? I vaguely remember that it has similar problems as the mentioned Atomic<T> use case, but maybe not the exact same.

I don't think so. The RFC to watch for that is #3301.

[programmerjake]

Regarding use cases, would this allow implementing seqlock w/o inline assembly? I vaguely remember that it has similar problems as the mentioned Atomic<T> use case, but maybe not the exact same.

I don't think so.

if you want a slower seqlock that works on arbitrary types including padding, but not including anything containing pointers, you can use freeze to do that without needing inline assembly. A demo doing that (though it uses inline assembly to implement freeze since freeze isn't implemented yet): https://rust.godbolt.org/z/zd6jTec3e

you'll want atomic bytewise memcpy for full speed seqlocks, since the compiler can do much larger loads and stores with that.

[RalfJung]

Ah right, you can do "atomic bytewise memcpy at home". There's also code somewhere that uses a derive for that, to do such a copy for a specific type. So yeah that would all work with just this RFC There are even variants of that that don't need this RFC I think.

[RalfJung]

Cc @thomcc as another freeze supporter, maybe you can help gather more usecases. :)