random: Initialize variables in hardware RNG functions #31863

1. 
   eval-exec commented at 9:08 am on February 14, 2025: contributor

   See: #31826 (review) , So this PR want to prevent potential uninitialized value issues and improve code clarity.
2. 
   DrahtBot commented at 9:09 am on February 14, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31863.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	sipa, achow101

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. 
   eval-exec commented at 10:27 am on February 14, 2025: contributor

   Hi @theuni, I’d appreciate it if you could review this PR.
4. in src/random.cpp:219 in ffac2733c9 outdated

   214@@ -215,8 +215,8 @@ void ReportHardwareRand()
   215  */
   216 uint64_t GetRNDR() noexcept
   217 {
   218-    uint8_t ok;
   219-    uint64_t r1;
   220+    uint8_t ok = 0;
   221+    uint64_t r1 = 0;
   

   ---

   ---

   

   maflcko commented at 11:07 am on February 14, 2025:

   Setting the sanity check value to a default of 0 is fine, because if it was never set, it will run in an infinite loop, just as-if the rng failed.

   However, setting the random value that is returned to a default value is problematic and controversial, because:

   • It silences sanitizers such as msan and valgrind that detect uninitialized reads
   • If we wanted to zero-init everything, we could just do #18892 (or a project-wide source change)
   • The initialization above was added in commit 71f183a49b714a28622277fa668d8f9f3dac0aae, however, it is not needed here, so adding it here as well seems odd. At least, I’d expect a rationale for it.

   ---

   

   eval-exec commented at 11:24 am on February 14, 2025:

   Agreed, seting ok = 0 is enough.

   ---

   

   theuni commented at 8:42 pm on February 14, 2025:

   Thanks @maflcko, I also agree.
5. random: Initialize variables in hardware RNG functions 99755e04ff
6. eval-exec force-pushed on Feb 14, 2025
7. 
   luke-jr commented at 7:57 pm on February 14, 2025: member

   improve code clarity.

   Initializing variables that do not need initializing is worse for code clarity.

   If you want to silence buggy static analysis, I don’t mind (~0), but it’s not a code clarity fix…

8. 
   sipa commented at 8:01 pm on February 14, 2025: member

   Eagerly awaiting C++26’s explicit [[indeterminate]] initialization.
9. theuni approved
10. 
    theuni commented at 8:44 pm on February 14, 2025: member

    utACk 99755e04ffadd5daad53c686f4f0feee2232eeb2
11. 
    sipa commented at 8:51 pm on February 14, 2025: member

    utACK 99755e04ffadd5daad53c686f4f0feee2232eeb2
12. 
    achow101 commented at 10:43 pm on February 14, 2025: member

    ACK 99755e04ffadd5daad53c686f4f0feee2232eeb2
13. achow101 merged this on Feb 14, 2025
14. achow101 closed this on Feb 14, 2025

    ---

15. 
    maflcko commented at 8:41 am on February 17, 2025: member

    Eagerly awaiting C++26’s explicit [[indeterminate]] initialization.

    Maybe I am missing something, but I don’t think [[indeterminate]] should be used here either. My understanding is that [[indeterminate]] in C++26 is simply a way to restore pre-C++-26 UB. However, if there really is UB in one of those functions, we’d want the C++26 erroneous behavior so that the implementation is encouraged to issue a diagnostic (and possibly abort) to notify about the incorrect program code.

    Though, given that no implementation exists for [[indeterminate]] (nor C++26 erroneous values), it is hard to say what the overhead would be for one over the other and in what cases the compiler can optimize and rule out the erroneous handling.

Contributors
eval-exec DrahtBot maflcko theuni luke-jr sipa achow101