This adds one more check to abort when global PRNG is used before SeedRand::ZEROS in fuzz tests. This is achieved by carving out the two remaining uses. First, g_rng_temp_path_init, and second the random fallback for RANDOM_CTX_SEED, which isn’t used in fuzz tests anyway.
Requested in #31521 (comment)
Can be tested by reverting fadd568931a2d21e0f80e1efaf2281f5164fa20e and observing an abort when running the utxo_total_supply fuzz target.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31548.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | marcofleon, hodlinator |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
107@@ -108,12 +108,12 @@ void ResetCoverageCounters() {}
108 #endif
109
110
111-void initialize()
112+inline void initialize()
inline rather than static?
static seems a better fit. Thx, fixed.
ACK fa7267022e8992f3db8eb7c8e81563c29294c19b
Good that you also replaced the defunct comment in fuzz.cpp with a useful one.
Could extend this to also catch too early uses of randomness when they occur in GetRandBytes, but g_rng_temp_path_init makes that a bit cumbersome.
Removing g_used_g_prng = false in g_rng_temp_path_init - triggered expected assert.
Reverted fadd568931a2d21e0f80e1efaf2281f5164fa20e as suggested - again triggering expected assert.
Could extend this to also catch too early uses of randomness when they occur in
GetRandBytes, butg_rng_temp_path_initmakes that a bit cumbersome.
That is what this pull is trying to do. Did I miss something?
That is what this pull is trying to do. Did I miss something?
Was thinking something like:
0void GetRandBytes(Span<unsigned char> bytes) noexcept
1{
2 g_used_g_prng = true;
3+ if constexpr (G_FUZZING) {
4+ Assert(g_seeded_g_prng_zero); // Need to seed randomness first
5+ }
6 ProcRand(bytes.data(), bytes.size(), RNGLevel::FAST, /*always_use_real_rng=*/false);
7}
So one gets that assert as quickly as possible.
But as I said, g_rng_temp_path_init would make that check a bit more complicated.
ACK fa3c787b62af6abaac35a8f0d785becdb8871cc0
iiuc this PR adds a check to make sure that no randomness is used before SeedRandomStateForTest(SeedRand::ZEROS) occurs (except in a couple special cases such as g_rng_temp_path_init), vs what check_globals does which is just checking if seeding happened at any point in the fuzz test.
Tested by moving SeedRandomStateForTest(SeedRand::ZEROS) to after test setup in utxo_total_supply and saw the assertion failure.
0/src/test/util/random.cpp:45 SeedRandomStateForTest: Assertion `!g_used_g_prng' failed.
re-ACK fa3c787b62af6abaac35a8f0d785becdb8871cc0
Switched 2 inline functions to static which more clearly communicates the purpose being translation-unit-locality.
Labels
Tests