mirror of
https://github.com/bitcoin/bitcoin.git
synced 2026-09-12 05:32:22 +02:00
Merge bitcoin/bitcoin#35478: fuzz: reset the mockable steady clock between iterations
19b32a2e18fuzz: reset the mockable steady clock between iterations (Hao Xu) Pull request description: Fix the issue mentioned by https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-4422112607 And this is my investigation on it: https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-4639472489 `CheckGlobalsImpl`'s constructor runs at the start of every fuzz iteration and already resets the global RNG flags and the mockable `NodeClock` (`SetMockTime(0s)`), but it never reset the mockable steady clock. A value written to `g_mock_steady_time` by one input therefore leaks into the next iteration. The most common source is `FuzzedSock`'s constructor, which calls `SetMockTime(INITIAL_MOCK_TIME)` (through `ElapseTime(0s)`) and never clears it: once any input constructs a `FuzzedSock`, the steady clock stays mocked for every subsequent iteration in the same process. This is one of the global-state leaks tracked in #29018. ### Fix Reset `MockableSteadyClock` symmetrically with `NodeClock`: ```diff g_used_system_time = false; SetMockTime(0s); +MockableSteadyClock::ClearMockTime(); ``` Besides removing the leak, this puts the steady clock under the same discipline as the system clock: a target that reads `MockableSteadyClock::now()` without first mocking it (via `FuzzedSock`, `SteadyClockContext`, …) is now caught by the existing `g_used_system_time` check at the end of the iteration, instead of silently reusing a value left over from a previous input. Clearing in `~FuzzedSock()` would be wrong: several `FuzzedSock`s can be alive simultaneously (e.g. `process_messages` adds 1–3 peers), so clearing in one destructor would corrupt the mock observed by the others. Resetting at the iteration boundary keeps it decoupled from socket lifetimes. ### Testing Verified with the global-state-detector approach from #29018 (snapshotting/diffing the writable globals around each iteration): - **Before:** a single empty input to `process_message` reports `g_mock_steady_time` changing `00 → 01` (`0` → `INITIAL_MOCK_TIME`). - **After:** that report is gone; the only remaining diffs are the benign one-time initialization of `ConsumeTime`'s function-local statics. `p2p_headers_presync` (uses `SteadyClockContext`) and `pcp_request_port_map` (uses `FuzzedSock`) still run to `succeeded` without aborting, confirming existing steady-clock readers are unaffected. This leak is invisible to coverage-based checks such as `deterministic-fuzz-coverage`, because `g_mock_steady_time` is only consumed through coarse time comparisons (e.g. the 250 ms presync rate-limiter): a changed value doesn't change the executed branches, so only a memory-diffing detector can see it. ACKs for top commit: maflcko: lgtm ACK19b32a2e18marcofleon: Nice catch, ACK19b32a2e18Tree-SHA512: b875795addb2914eae489adc703438483f8e464b9a210bd5d76189f13266dae5843c8749590d59e78bf171f19aa7cee21ca678cd311843d8a88cbe9831f20b6a
This commit is contained in:
@@ -19,6 +19,7 @@ struct CheckGlobalsImpl {
|
||||
g_seeded_g_prng_zero = false;
|
||||
g_used_system_time = false;
|
||||
SetMockTime(0s);
|
||||
MockableSteadyClock::ClearMockTime();
|
||||
}
|
||||
~CheckGlobalsImpl()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user