fuzz: Abort if system time is called without mock time being set #31549

pull marcofleon wants to merge 2 commits into bitcoin:master from marcofleon:2024/12/fuzz-set-mocktime changing 17 files +65 −5
  1. marcofleon commented at 5:59 pm on December 20, 2024: contributor

    This PR expands the CheckGlobals utility that was introduced in #31486 and should help with fuzz stability (https://github.com/bitcoin/bitcoin/issues/29018).

    System time shouldn’t be used when running a fuzz test, as it is likely to introduce instability (non-determinism). This PR identifies and fixes the targets that were calling system time without setting mock time at the start of an iteration.

    RemovingSetMockTime() from any one of these targets should result in a crash and a message describing the issue.

  2. DrahtBot commented at 6:00 pm on December 20, 2024: 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/31549.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK dergoegge, brunoerg, achow101
    Concept ACK Prabhat1308
    Stale ACK i-am-yuvi, maflcko

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31022 (test: Add mockable steady clock, tests for PCP and NATPMP implementations by laanwj)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Tests on Dec 20, 2024
  4. marcofleon force-pushed on Dec 20, 2024
  5. DrahtBot added the label CI failed on Dec 20, 2024
  6. DrahtBot commented at 6:35 pm on December 20, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34723484665

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  7. marcofleon force-pushed on Dec 20, 2024
  8. DrahtBot removed the label CI failed on Dec 20, 2024
  9. brunoerg commented at 9:32 pm on December 20, 2024: contributor
    Concept ACK
  10. Prabhat1308 commented at 4:19 pm on December 24, 2024: none
    Concept ACK .
  11. i-am-yuvi commented at 3:44 pm on January 2, 2025: none

    I am unable to crash the fuzz test when setmocktime() is removed from one of the targets.

    clang version:

    0Homebrew clang version 18.1.7
1Target: arm64-apple-darwin24.2.0
2Thread model: posix
3InstalledDir: /opt/homebrew/opt/llvm/bin

    clang++ version:

    0Homebrew clang version 18.1.7
1Target: arm64-apple-darwin24.2.0
2Thread model: posix
3InstalledDir: /opt/homebrew/opt/llvm/bin

    and I’ve used this command mentioned in docs for macos:

    0$ cmake --preset=libfuzzer \
1   -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
2   -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
3   -DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries

    Is there anything I am doing wrong??

  12. in src/util/time.cpp:27 in 8cf5c4e91c outdated 
    19@@ -20,9 +20,11 @@
20 void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); }
21 
22 static std::atomic<std::chrono::seconds> g_mock_time{}; //!< For testing
23+std::atomic<bool> g_used_system_time{false};
24 
25 NodeClock::time_point NodeClock::now() noexcept
26 {
27+    g_used_system_time = true;
    maflcko commented at 3:58 pm on January 2, 2025:
    Shouldn’t this be g_used_system_time |= bool(mocktime.count())? This would allow to remove the GetMockTime().count() == 0 check as well.
    marcofleon commented at 8:10 pm on January 2, 2025:

    Thanks, yeah that makes more sense. This actually caught system time usage in p2p_headers_presync that wasn’t crashing before. Because the mocktime.count() check is now in the function itself rather than at the end of the fuzz iteration.

    Using the bitwise OR assignment didn’t compile for me, but I believe what I just pushed should work.

  13. marcofleon commented at 8:03 pm on January 2, 2025: contributor

    I am unable to crash the fuzz test when setmocktime() is removed from one of the targets. @i-am-yuvi which target was it?

    It could be that running the test from scratch doesn’t quickly reach code where now() is called. Try running the target on a corpus of inputs:

    0FUZZ=load_external_block_file ./mocktimefuzzbuild/src/test/fuzz/fuzz ../qa-assets/fuzz_corpora/load_external_block_file -runs=1

    You can find the corpora here: https://github.com/bitcoin-core/qa-assets

    I think it’s also possible that it’s machine specifc. The command above for load_external_block_file crashes on my Linux machine but not on my mac, for example.

  14. marcofleon force-pushed on Jan 2, 2025
  15. i-am-yuvi commented at 12:53 pm on January 4, 2025: none

    I am unable to crash the fuzz test when setmocktime() is removed from one of the targets.

    @i-am-yuvi which target was it?

    It could be that running the test from scratch doesn’t quickly reach code where now() is called. Try running the target on a corpus of inputs:

    0FUZZ=load_external_block_file ./mocktimefuzzbuild/src/test/fuzz/fuzz ../qa-assets/fuzz_corpora/load_external_block_file -runs=1

    You can find the corpora here: https://github.com/bitcoin-core/qa-assets

    I think it’s also possible that it’s machine specifc. The command above for load_external_block_file crashes on my Linux machine but not on my mac, for example.

    Thanks, worked(crashed) on Linux with Corpus. I haven’t tested on Mac due to storage issue, it would be interesting to see if that would crash on macos!!

  16. i-am-yuvi approved
  17. i-am-yuvi commented at 1:19 pm on January 4, 2025: none

    Tested ACK fdc5224d205c69ebcb2d2b7bcd95386ef89181ca on x86_64

    • removing setmocktime () from any one of the targets would cause a crash.
    • Not tested on macos
  18. maflcko commented at 11:59 am on January 6, 2025: member

    I think you forgot to set it in the init function? Otherwise, the global state may be non-deterministic:

     0index e4e4723c74..3f3bfbeeaa 100644
 1--- a/src/test/fuzz/fuzz.cpp
 2+++ b/src/test/fuzz/fuzz.cpp
 3@@ -115,6 +115,7 @@ void initialize()
 4     // - GetStrongRandBytes(), which is used for the creation of private key material.
 5     // - Creating a BasicTestingSetup or derived class will switch to a random seed.
 6     SeedRandomStateForTest(SeedRand::ZEROS);
 7+    //here
 8 
 9     // Terminate immediately if a fuzzing harness ever tries to create a socket.
10     // Individual tests can override this by pointing CreateSock to a mocked alternative.
  19. maflcko commented at 12:50 pm on January 6, 2025: member

    Tested that no superfluous ones were added via:

     0diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
 1index e4e4723c74..eda5ff4cc6 100644
 2--- a/src/test/fuzz/fuzz.cpp
 3+++ b/src/test/fuzz/fuzz.cpp
 4@@ -78,11 +78,13 @@ void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target,
 5 
 6 static std::string_view g_fuzz_target;
 7 static const TypeTestOneInput* g_test_one_input{nullptr};
 8+bool g_ever_used_g_mocktime{false};
 9 
10 inline void test_one_input(FuzzBufferType buffer)
11 {
12     CheckGlobals check{};
13     (*Assert(g_test_one_input))(buffer);
14+    g_ever_used_g_mocktime|= g_used_mocktime;
15 }
16 
17 const std::function<std::string()> G_TEST_GET_FULL_NAME{[]{
18@@ -274,6 +276,9 @@ int main(int argc, char** argv)
19     }
20     const auto end_time{Now<SteadySeconds>()};
21     std::cout << g_fuzz_target << ": succeeded against " << tested << " files in " << count_seconds(end_time - start_time) << "s." << std::endl;
22+    if (tested && !g_ever_used_g_mocktime && g_set_mocktime) {
23+        Assert(false); // remove unused SetMockTime()?
24+    }
25 #endif
26     return 0;
27 }
28diff --git a/src/test/fuzz/util/check_globals.cpp b/src/test/fuzz/util/check_globals.cpp
29index f91a965afc..76d62fa3ca 100644
30--- a/src/test/fuzz/util/check_globals.cpp
31+++ b/src/test/fuzz/util/check_globals.cpp
32@@ -17,6 +17,8 @@ struct CheckGlobalsImpl {
33     {
34         g_used_g_prng = false;
35         g_seeded_g_prng_zero = false;
36+        g_set_mocktime= false;
37+        g_used_mocktime=false;
38         g_used_system_time = false;
39         SetMockTime(0s);
40     }
41diff --git a/src/test/fuzz/util/check_globals.h b/src/test/fuzz/util/check_globals.h
42index 12d39c2daf..798a9ba506 100644
43--- a/src/test/fuzz/util/check_globals.h
44+++ b/src/test/fuzz/util/check_globals.h
45@@ -10,6 +10,8 @@
46 #include <optional>
47 #include <string>
48 
49+extern std::atomic<bool> g_set_mocktime;
50+extern std::atomic<bool> g_used_mocktime;
51 extern std::atomic<bool> g_used_system_time;
52 
53 struct CheckGlobalsImpl;
54diff --git a/src/util/time.cpp b/src/util/time.cpp
55index 00f0f47392..f8f1eb5504 100644
56--- a/src/util/time.cpp
57+++ b/src/util/time.cpp
58@@ -20,6 +20,8 @@
59 void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); }
60 
61 static std::atomic<std::chrono::seconds> g_mock_time{}; //!< For testing
62+std::atomic<bool> g_set_mocktime{false};
63+std::atomic<bool> g_used_mocktime{false};
64 std::atomic<bool> g_used_system_time{false};
65 
66 NodeClock::time_point NodeClock::now() noexcept
67@@ -27,6 +29,8 @@ NodeClock::time_point NodeClock::now() noexcept
68     const auto mocktime{g_mock_time.load(std::memory_order_relaxed)};
69     if (!mocktime.count()) {
70         g_used_system_time = true;
71+    }else{
72+    g_used_mocktime=true;
73     }
74     const auto ret{
75         mocktime.count() ?
76@@ -40,6 +44,7 @@ void SetMockTime(int64_t nMockTimeIn) { SetMockTime(std::chrono::seconds{nMockTi
77 void SetMockTime(std::chrono::seconds mock_time_in)
78 {
79     Assert(mock_time_in >= 0s);
80+    g_set_mocktime=true;
81     g_mock_time.store(mock_time_in, std::memory_order_relaxed);
82 }
83
  20. maflcko commented at 12:51 pm on January 6, 2025: member

    ACK 28e374f0a7dc1bda1c71e14eb896b2d9d2f23b48 🍭

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 28e374f0a7dc1bda1c71e14eb896b2d9d2f23b48 🍭
3k8AjcAoUwz8rU6OdmDfzs1fRRDn4rJcQL6cG4GfGNM5b8gdMKm2r8/okjdf72tEmzwVbTt5qEHM12yxpjaAPCw==
  21. DrahtBot requested review from Prabhat1308 on Jan 6, 2025
  22. DrahtBot requested review from i-am-yuvi on Jan 6, 2025
  23. DrahtBot requested review from brunoerg on Jan 6, 2025
  24. fuzz: Add SetMockTime() to necessary targets ff21870e20
  25. fuzz: Abort when calling system time without setting mock time a96b84cb1b
  26. marcofleon force-pushed on Jan 6, 2025
  27. marcofleon commented at 3:54 pm on January 6, 2025: contributor

    Thanks for the review @i-am-yuvi!

    I haven’t tested on Mac due to storage issue, it would be interesting to see if that would crash on macos!!

    So I learned that it’s likely this https://github.com/bitcoin/bitcoin/blob/41a2ce9b7d7354c633c104bab5653f1f60eb2068/src/test/fuzz/util.cpp#L278-L289 that is preventing the load_external_block_file target from crashing on a mac.

  28. marcofleon commented at 3:58 pm on January 6, 2025: contributor

    I think you forgot to set it in the init function? Otherwise, the global state may be non-deterministic

    Good catch, thank you. Added in ff21870e20b2391b684cc50fdd6879805055d6a1

  29. dergoegge approved
  30. dergoegge commented at 10:47 am on January 7, 2025: member
    Code review ACK a96b84cb1b76e65a639e62f0224f534f89858c18
  31. DrahtBot requested review from maflcko on Jan 7, 2025
  32. brunoerg commented at 11:29 am on January 7, 2025: contributor
    crACK a96b84cb1b76e65a639e62f0224f534f89858c18
  33. achow101 commented at 0:25 am on January 10, 2025: member
    ACK a96b84cb1b76e65a639e62f0224f534f89858c18
  34. achow101 merged this on Jan 10, 2025
  35. achow101 closed this on Jan 10, 2025
  36. Jassor909 approved
  37. maflcko commented at 8:52 am on January 10, 2025: member
    post-merge reACK a96b84cb1b76e65a639e62f0224f534f89858c18 (only change is adding missing mocktime in init)


marcofleon DrahtBot brunoerg Prabhat1308 i-am-yuvi maflcko dergoegge achow101 Jassor909


maflcko i-am-yuvi Prabhat1308

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-01-21 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me