random_seed is UB

[nico-abram]

Here:

temporary/src/lib.rs

Lines 143 to 146 in 6821517

	fn random_seed(_: &Path, _: &str) -> [u64; 2] {
	use std::mem::uninitialized as rand;
	unsafe { [rand::<u64>() ^ 0x12345678, rand::<u64>() ^ 0x87654321] }
	}

According to the std::mem::uninitialized docs this is UB: https://doc.rust-lang.org/std/mem/fn.uninitialized.html

As the assume_init documentation explains, the Rust compiler assumes that values are properly initialized. As a consequence, calling e.g. mem::uninitialized::() causes immediate undefined behavior for returning a bool that is not definitely either true or false. Worse, truly uninitialized memory like what gets returned here is special in that the compiler knows that it does not have a fixed value. This makes it undefined behavior to have uninitialized data in a variable even if that variable has an integer type. (Notice that the rules around uninitialized integers are not finalized yet, but until they are, it is advisable to avoid them.)

This crate could possibly give you an easy way to get a real entropy source, if desireable: https://github.com/rust-random/getrandom

Otherwise, you could just use hardcoded values like [0 ^0x12345678, 0 ^ 0x87654321]

If you still want a hacky inbetween, maybe consider using a function address and relying on ASLR being enabled (Like random_seed as usize as u64).

Also notice that sometimes the compiler just replaces uninitialized() with a fixed value (Like 0), so it would be no better than a fixed value entropy-wise despite being UB.

EDIT: Maybe it could also be replaced with inline asm to read the stack, but that would require a different asm block for each ISA and not a real source of entropy.

[IvanUkhov]

Hello, thank you for the issue! For my understanding, is the concern that there is a usage of unsafe code? I left it that way so that, in some nonoptimized contexts, it would behave less predictably, but it is clear that random_seed is not that random, and that it would probably be some constant returned. For this create, this is not critical. In addition, I wanted to eventually make use of the two arguments passed into random_seed to allow the caller to have some influence.

[5225225]

For my understanding, is the concern that there is a usage of unsafe code?

The issue is not unsafe code, the issue is incorrect code.

It's undefined behaviour to use a variable that has not been initialised (there's... some subtle points around what exactly "use" is, but doing a bitwise xor, or returning it from a function is definitely use).

If the uninit variable was being used as a "i will initialise this later" slot, then you could use MaybeUninit. But it's not, it's being used as a value and you're trying to get entropy from there. That will never be allowed, and the compiler is allowed to assume you don't do that (hence it being optimised out to a [0, 0] return in release mode)

Honestly, I'd add a dependency on getrandom and let that do the actual hard part of securely getting good entropy for you.

This crate acts completely predictably with optimised mode (as I noted in my advisory for this crate: rustsec/advisory-db#1196), and some libraries do run their tests in release mode for performance gains.

I'm not terribly concerned about this crate because it's generally used only for tests, so at least this isn't being run "in the wild". Still, I'd make it use getrandom and ideally yank all versions of the crate that use this method of getting entropy.

[IvanUkhov]

Thank you for the response. Can you please give an example of how it might lead to problems in this case?

[5225225]

Well, if you run tests in release, your names are always the same. So you cannot run 2 tests that rely on unique names in release.

And in general you're relying on the compiler to happen to not notice what you're doing. (These might even be turned into unconditional panics in the future, but work towards doing that is ongoing and somewhat slow, because people write stuff that happens to work most of the time.)

[IvanUkhov]

Sorry, maybe it was not clear from before. This is the expected behavior. Of course, it will return the same name. This is partially the reason there is a retry loop:

https://github.com/stainless-steel/temporary/blob/master/src/lib.rs#L59

Eventually it will run out of retries, but the probability of this happening is low, especially given we are talking about testing. One can push it even further by updating random_seed how I alluded to above.

[5225225]

I guess, but in that case, why not just not have the entropy in the first place?

If you have the retries, and tests run in release will never get any entropy, that's equal to using tmp.00001 and incrementing the counter until you find a free directory.

---

Edit: either not have entropy or collect it properly (getrandom)

[nico-abram]

For my understanding, is the concern that there is a usage of unsafe code?

No, there is nothing wrong with using unsafe when you need it. The concern is specifically that the unsafe code is Undefined Behaviour (a.k.a wrong/incorrect). To quote the reference (https://doc.rust-lang.org/reference/behavior-considered-undefined.html):

Rust code is incorrect if it exhibits any of the behaviors in the following list. This includes code within unsafe blocks and unsafe functions. unsafe only means that avoiding undefined behavior is on the programmer; it does not change anything about the fact that Rust programs must never cause undefined behavior.

It also explicitly calls out this case:

An integer (i*/u*), floating point value (f*), or raw pointer obtained from uninitialized memory, or uninitialized memory in a str.

Like 522 mentioned, this is not guaranteed to work. The compiler is free to remove this function, or turn it into an undefined instruction like ud2, or turn it into an unconditional panic. For an example of the compiler turning this kind of thing into an unconditional panic for some types (Specifically types that do not permit zero-initialization) see rust-lang/rust#66059
This example panics at runtime with the message "attempted to leave type core::num::nonzero::NonZeroUsize uninitialized, which is invalid":

fn main() {
    unsafe {
        let _x: core::num::NonZeroUsize = std::mem::uninitialized();
    }
}

I will repeat a part of the quote from the docs in my first comment:

(Notice that the rules around uninitialized integers are not finalized yet, but until they are, it is advisable to avoid them.)

Which means this could be relaxed in the future but it is currently considered UB. As far as I know the last discussion about that question happened here (rust-lang/unsafe-code-guidelines#71). Regardless, it's a good idea to fix this code under the rules from the current documentation instead of assuming it will eventually be considered valid.

[5225225]

(Specifically types that do not permit zero-initialization)

It's not just that, char also panics. I think the rules are "if it has no niches we let it pass for now, but we currently don't check arrays because that breaks too much stuff".

Regardless, doing operations on uninit values will never be not UB. You'd need freeze for that.

[IvanUkhov]

I have updated the function to have no undefined behavior. Thank you for your input!

[5225225]

cool, thank you very much!

I updated the advisory to list the patched version, so anyone on older versions can upgrade / get their dependencies to upgrade.