From uint64_t GetRdRand() noexcept :
In the case of ten RdRand failures in a row: wouldn’t r1 be read uninitialized on L150? (If so, then the same goes for the __i386__ case a couple of lines above.)
It should be noted that the odds of ten RdRand failures in a row are astronomically small, so I don’t think this is urgent at all. Would be nice to rule out the theoretical possibility though (if such a possibility exists).
Friendly ping @sipa :)
What would you want to do in this case? Terminate the program? Log a warning?
(it’s only used as additional entropy source, so maybe even “return 0 and ignore otherwise”)
We could change the signature to bool GetRdRand(uint64_t& out) and make it so that callers only do the hasher.Write(… data assumed to be from RdRand …) call if GetRdRand succeeds. In other words: act like we would have done if !g_rdseed_supported in case of GetRdRand failure.
Alternatively: as long as we keep on using it only as an additional entropy source I guess uint64_t r1 = 0; on L145 would be enough. (Obvious theoretical downside: if someone thinks GetRdRand() can be used as the only entropy source :))
if someone thinks GetRdRand() can be used as the only entropy source
There’s plenty of other reasons why that’d be a bad idea. E.g. hardware failures. But sure, it would have to be documented.
There’s plenty of other reasons why that’d be a bad idea.
Yes, obviously so :)
My point was that bool GetRdRand(uint64_t& out) makes it less likely for someone to make that mistake: it would be immediately obvious from the signature that the operation could fail.
Optional<...> for this.
RdRand connoisseur @gmaxwell - what do you think? :)
FWIW, clang++ -Wconditional-uninitialized seems to agree:
0random.cpp:150:12: warning: variable 'r1' may be uninitialized when used here [-Wconditional-uninitialized]
It is recommended that applications attempt 10 retries in a tight loop in the unlikely event that the RDRAND instruction does not return a random number. This number is based on a binomial probability argument: given the design margins of the DRNG, the odds of ten failures in a row are astronomically small and would in fact be an indication of a larger CPU issue.
maybe we should abort then?
But optional + a good log is also good IMHO
rdrand fails, the resulting register is overwritten (with zeroes), so I don’t think there can be an uninitialized value that comes out.
@sipa Thanks for clarifying! Good to hear. Closing this issue.
I guess it still might make sense to initialize uint64_t r1 to zero just to make it clear for Clang (and human readers) that r1 cannot be used uninitialized here:
0random.cpp:136:12: warning: variable 'r1' may be uninitialized when used here [-Wconditional-uninitialized]
1 return r1;
2 ^~
3random.cpp:131:16: note: initialize the variable 'r1' to silence this warning
4 uint64_t r1;
5 ^
6 = 0