fuzz: Abort when using global PRNG without re-seed

maflcko commented at 12:38 pm on December 13, 2024: member

This is the first step toward improving fuzz stability and determinism (https://github.com/bitcoin/bitcoin/issues/29018).

A fuzz target using the global test-only PRNG will now abort if the seed is re-used across fuzz inputs.

Also, temporarily add SeedRandomStateForTest(SeedRand::ZEROS) to all affected fuzz targets. This may slow down the libfuzzer leak detector, but it will disable itself after some time, or it can be disabled explicitly with -detect_leaks=0.

In a follow-up, each affected fuzz target can be stripped of the global random use and a local RandomMixin (or similar) can be added instead.

(Can be tested by removing any one of the re-seed calls and observing a fuzz abort)

DrahtBot commented at 12:38 pm on December 13, 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/31486.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator, dergoegge, marcofleon

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:

#31481 (fuzz: Faster leak check, and SeedRand::ZEROS before every input by maflcko)

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.

DrahtBot added the label Tests on Dec 13, 2024

dergoegge commented at 1:10 pm on December 13, 2024: member

Concept ACK - Nice!

I kinda prefer this over #31481

maflcko force-pushed on Dec 13, 2024

fuzz: Add missing SeedRandomStateForTest(SeedRand::ZEROS) fa7809aeab

maflcko force-pushed on Dec 13, 2024

maflcko commented at 1:25 pm on December 13, 2024: member

(split into two commits to make review easier, no overall diff)

marcofleon commented at 3:17 pm on December 13, 2024: contributor

Removed the call for a few targets and the expected behavior (message and crash) happens for libfuzzer but not for afl++. With afl++, removing the call leads to a very obvious slowdown for some of the targets.

For example for tx_package_eval, starting from an empty corpus, removing SeedRandomStateForTest(SeedRand::ZEROS) causes the target to go from ~5000 exec/s to ~7 exec/sec.

marcofleon commented at 3:25 pm on December 13, 2024: contributor

Weird because test_one_input is called for AFL persistent mode so I don’t see why it wouldn’t also abort.

maflcko commented at 3:37 pm on December 13, 2024: member

removing SeedRandomStateForTest(SeedRand::ZEROS) causes the target to go from ~5000 exec/s to ~7 exec/sec.

Interesting, are you sure it is not the other way round? Did you try with addition_overflow (or another simple target) as well? Does it also happen with the fixed inputs? Otherwise, it may just be the fuzz engine walking a path, like in #30644 (comment) ? What options did you compile with? What are the steps to reproduce, starting from a fresh install of the OS?

in src/test/fuzz/fuzz.cpp:101 in fa5c0da4bb outdated

 96+                     "An alternative solution would be to avoid any use of globals.\n\n"
 97+
 98+                     "Without a solution, fuzz stability and determinism can lead \n"
 99+                     "to non-reproducible bugs or inefficient fuzzing.\n\n"
100+                  << std::endl;
101+        std::exit(EXIT_FAILURE);

dergoegge commented at 3:42 pm on December 13, 2024:

this needs to be a std::abort() (or similar), otherwise out of process fuzzers like afl will not catch this as a crash (by default at least)

dergoegge commented at 3:49 pm on December 13, 2024:

You may be able to confirm this with AFL_CRASH_EXITCODE='1' (i.e. tell afl++ to treat exit(1) as a crash)

maflcko commented at 3:55 pm on December 13, 2024:

thx, fixed!

dergoegge commented at 3:45 pm on December 13, 2024: member

the target to go from ~5000 exec/s to ~7 exec/sec

That is likely caused by persistent mode no longer working properly because of the std::exit, it basically degrades into non-persistent mode (i.e. re-forking every time it hits the std::exit).

maflcko force-pushed on Dec 13, 2024

in src/test/fuzz/fuzz.cpp:309 in fa5a1d524a outdated

304+                     "of processing the fuzz input.\n\n"
305+
306+                     "This is not needed and may slow down fuzzing. Please \n"
307+                     "remove it or add fuzz inputs that excersise it.\n\n"
308+                  << std::endl;
309+        std::exit(EXIT_FAILURE);

hodlinator commented at 9:09 pm on December 13, 2024:

Should this also be an abort(), or should the commit message be adjusted to say something else than “Abort”?

maflcko commented at 10:00 am on December 16, 2024:

Commit fa5a1d524aa5647ca3a5f61c664adccd1904849e seems a bit too fragile overall, because it could detect false-positives. So I went ahead and dropped it for now. It should be enough if someone cherry-picks and runs it every release (6mo), or so.

hodlinator commented at 2:24 pm on December 16, 2024:

Yeah, I guess if a fuzz target only executes code blocks using randomness depending on the fuzz-data, this could have been annoying for short fuzz-runs.

in src/test/util/random.h:32 in fa5a1d524a outdated

27@@ -27,6 +28,9 @@ enum class SeedRand {
28 /** Seed the global RNG state for testing and log the seed value. This affects all randomness, except GetStrongRandBytes(). */
29 void SeedRandomStateForTest(SeedRand seed);
30 
31+extern std::atomic<bool> seeded_g_prng_zero;
32+extern std::atomic<bool> used_g_prng;

hodlinator commented at 9:13 pm on December 13, 2024:

nit: Try to adhere to symbol naming convention from developer-notes.md?

0extern std::atomic<bool> g_seeded_prng_zero;
1extern std::atomic<bool> g_used_prng;

maflcko commented at 10:00 am on December 16, 2024:

thx, added g_.

hodlinator commented at 2:50 pm on December 16, 2024:

Should the _g_ in the middle of g_seeded_g_prng_zero and the other one still be kept?

maflcko commented at 3:37 pm on December 16, 2024:

Yes, because this is only about the global prng, not about any local ones.

hodlinator commented at 4:10 pm on December 16, 2024:

(Would prefer g_seeded_global_prng_zero in that case, even though I admit it is verbose).

in src/test/fuzz/fuzz.cpp:80 in fa5a1d524a outdated

76@@ -77,6 +77,32 @@ void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target,
77 
78 static std::string_view g_fuzz_target;
79 static const TypeTestOneInput* g_test_one_input{nullptr};
80+bool ever_used_g_prng{false};

hodlinator commented at 9:24 pm on December 13, 2024:

nit: Naming + static

0static bool g_ever_used_prng{false};

maflcko commented at 10:00 am on December 16, 2024:

dropped commit

in src/test/util/random.cpp:41 in fa5a1d524a outdated

37@@ -36,6 +38,7 @@ void SeedRandomStateForTest(SeedRand seedtype)
38         return GetRandHash();
39     }();
40 
41+    seeded_g_prng_zero = seedtype == SeedRand::ZEROS;

hodlinator commented at 9:30 pm on December 13, 2024:

Bug in that it can be reset if called with FIXED_SEED afterwards:

0    seeded_g_prng_zero |= seedtype == SeedRand::ZEROS;

Wonder if it would be useful to also add checks against FIXED_SEED being used?

maflcko commented at 10:00 am on December 16, 2024:

Wonder if it would be useful to also add checks against FIXED_SEED being used?

Yes, this is what the code here is already doing. And your suggestion would remove the check (in some cases), no?

hodlinator commented at 3:01 pm on December 16, 2024:

Current behavior with a test that calls both:

0SeedRandomStateForTest(SeedRand::ZEROS);
1SeedRandomStateForTest(SeedRand::FIXED_SEED);

Resulting in seeded_g_prng_zero not being set… which I guess is a way of detecting that something is wrong.

My suggestion would make seeded_g_prng_zero retain the fact that ZEROS had been used.

None of the above solutions detect this invalid case:

0SeedRandomStateForTest(SeedRand::FIXED_SEED);
1...fuzz target code using randomness...
2SeedRandomStateForTest(SeedRand::ZEROS);

It would be more robust with my suggested change, + adding a separate seeded_g_prng_fixed bool which is (|=-)set when FIXED_SEED is used, so that we could error on that.

maflcko commented at 3:44 pm on December 16, 2024:

adding a separate seeded_g_prng_fixed bool

No strong opinion. There is only one call to SeedRandomForTest(SeedRand::FIXED_SEED), so hopefully it won’t be a problem going forward and the code doesn’t have to be written and reviewed. Happy to reconsider, if other reviewers want it as well.

in src/test/fuzz/fuzz.cpp:82 in fa5a1d524a outdated

76@@ -77,6 +77,32 @@ void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target,
77 
78 static std::string_view g_fuzz_target;
79 static const TypeTestOneInput* g_test_one_input{nullptr};
80+bool ever_used_g_prng{false};
81+
82+inline void test_one_input(FuzzBufferType buffer)

hodlinator commented at 9:43 pm on December 13, 2024:

nit: Bit of a naming clash with FuzzTarget::test_one_input. :/

FuzzTarget::test_one_input is a subpar as a field name, but if you don’t want to touch that:

0inline void execute_one_input(FuzzBufferType buffer)

maflcko commented at 10:00 am on December 16, 2024:

Leaving as-is for now, as I don’t want to touch unrelated lines. test_one_input was already used before as the name, also it is used identically in https://github.com/bitcoin/bitcoin/pull/30882/files#diff-7f9d6992a63bfa29310450a7af7629ff9c324b275c4d94f102e8eca9daf118fd, so I’ll also leave as-is to avoid (more) merge conflicts.

hodlinator commented at 9:50 pm on December 13, 2024: contributor

Concept ACK fa5a1d524aa5647ca3a5f61c664adccd1904849e

Great to push fuzz test determinism forward.

Seems like SeedRandomStateForTest(SeedRand::ZEROS) should be removed from initialize() in fuzz.cpp?

maflcko force-pushed on Dec 16, 2024

maflcko commented at 10:00 am on December 16, 2024: member

Seems like SeedRandomStateForTest(SeedRand::ZEROS) should be removed from initialize() in fuzz.cpp?

No, why? Initialization happens before iterating over the inputs, so it is separate and isn’t affected by this pull request.

marcofleon commented at 1:18 pm on December 16, 2024: contributor

You think this same concept is worth doing for GetTime() and SetMockTime() as well? I’d be happy to implement it if so, or you could add it to this PR if that’s easier.

maflcko force-pushed on Dec 16, 2024

maflcko commented at 2:00 pm on December 16, 2024: member

Makes sense to extend the approach to other globals as well. I’ve moved it to a new module, so that the linter/check does not clutter the main fuzz.cpp file, if it is extended more. As for the (global) system time check, I presume it would fail, depending on the args that were used to fuzz (e.g. --printtoconsole=1), so I think I’ll leave it as-is for now. However, I am happy to push a commit, if it works. Also, I am happy to review a follow-up pull. I can also look into opening one myself.

fuzz: Abort when using global PRNG without re-seed fa18acb457

maflcko force-pushed on Dec 16, 2024

DrahtBot added the label CI failed on Dec 16, 2024

DrahtBot commented at 2:24 pm on December 16, 2024: contributor

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

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.

in src/test/fuzz/util/check_globals.cpp:12 in fa18acb457 outdated

 7+#include <test/util/random.h>
 8+
 9+#include <iostream>
10+#include <memory>
11+#include <optional>
12+#include <string>

hodlinator commented at 2:29 pm on December 16, 2024:

nit: Unused

maflcko commented at 3:53 pm on December 16, 2024:

thx. Will fix if I have to re-push. In the future, it would be better to check this in the CI, see https://github.com/bitcoin/bitcoin/pull/31308

in src/random.cpp:678 in fa18acb457 outdated

670@@ -671,9 +671,11 @@ void MakeRandDeterministicDANGEROUS(const uint256& seed) noexcept
671 {
672     GetRNGState().MakeDeterministic(seed);
673 }
674+std::atomic<bool> g_used_g_prng{false}; // Only accessed from tests
675 
676 void GetRandBytes(Span<unsigned char> bytes) noexcept
677 {
678+    g_used_g_prng = true;

hodlinator commented at 2:34 pm on December 16, 2024:

Skip in production

0    if constexpr (G_FUZZING) {
1        g_used_g_prng = true;
2    }

or use #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION.

Maybe an optimizer would remove it as a dead-store, but better not to rely on that.

maflcko commented at 3:36 pm on December 16, 2024:

Is there a (micro-)benchmark that would improve?

Also, this would make it harder to use in other tests, such as the unit tests.

hodlinator commented at 4:22 pm on December 16, 2024:

This is about excluding test-only code from production (currently fuzz-only since check_globals.h/cpp is in fuzz/util/). Not sure why that would be undesirable. Even though the conditional adds verbosity, it adds context and decreases need for comments.

Might be worth introducing a TESTS_ENABLED #define… but I’m guessing that has been discussed before?

maflcko commented at 7:52 am on December 17, 2024:

Might be worth introducing a TESTS_ENABLED #define… but I’m guessing that has been discussed before?

I don’t think it has. Maybe a separate issue or pull request can be created?

Whatever is done here also applies to the function immediately above, which is unrelated, so I’ll leave this as-is for now.

hodlinator commented at 8:36 am on December 17, 2024:

Interesting that it hasn’t really been discussed. There are trade-offs to adding it, but yeah, might experiment with adding it and create an issue.

Correct, MakeRandDeterministicDANGEROUS() should not exist unless TESTS_ENABLED was #defined (in that hypothetical scenario).

in src/test/fuzz/util/check_globals.cpp:21 in fa18acb457 outdated

16+    {
17+        g_used_g_prng = false;
18+        g_seeded_g_prng_zero = false;
19+    }
20+    ~CheckGlobalsImpl()
21+    {

hodlinator commented at 2:47 pm on December 16, 2024:

Should skip this if fuzz target throws an exception.

0    {
1        if (std::uncaught_exceptions() > 0) {
2            std::cerr << "Skipping checking of PRNG globals as exception is being thrown.\n";
3            return;
4        }
5

maflcko commented at 3:51 pm on December 16, 2024:

It seems more important to make the code reproducible and stable before extended fuzzing. Also, if the crash is reproducible, it will be re-detected anyway when the code is fixed. So I think the extra code isn’t needed.

hodlinator commented at 4:29 pm on December 16, 2024:

Hm.. I guess it should pretty much never happen :tm: as the exception would have to be thrown before the seeding of the RNG at the beginning of a test.

hodlinator commented at 3:24 pm on December 16, 2024: contributor

Code reviewed fa18acb457e91cc0fa6b3640b6b55c6bc61572ee

Seems like SeedRandomStateForTest(SeedRand::ZEROS) should be removed from initialize() in fuzz.cpp?

No, why? Initialization happens before iterating over the inputs, so it is separate and isn’t affected by this pull request.

Yes, but does it still provide any value when the fuzz targets themselves are now pushed to call it?

DrahtBot removed the label CI failed on Dec 16, 2024

maflcko commented at 3:36 pm on December 16, 2024: member

Seems like SeedRandomStateForTest(SeedRand::ZEROS) should be removed from initialize() in fuzz.cpp?

No, why? Initialization happens before iterating over the inputs, so it is separate and isn’t affected by this pull request.

Yes, but does it still provide any value when the fuzz targets themselves are now pushed to call it?

Yes, because initialization should be deterministic as well. Otherwise, there could be stability or determinism issues as well.

hodlinator commented at 4:57 pm on December 16, 2024: contributor

Seems like SeedRandomStateForTest(SeedRand::ZEROS) should be removed from initialize() in fuzz.cpp?

..

[…] initialization should be deterministic as well. […]

Ah, sorry for not getting it the first time.

Current code:

https://github.com/bitcoin/bitcoin/blob/fa18acb457e91cc0fa6b3640b6b55c6bc61572ee/src/test/fuzz/fuzz.cpp#L111-L117

BasicTestingSetup which is used during several fuzz initialization functions has the only such call you mentioned in another thread:

https://github.com/bitcoin/bitcoin/blob/fa18acb457e91cc0fa6b3640b6b55c6bc61572ee/src/test/util/setup_common.cpp#L142

Should that call be disabled for fuzz-tests?

hodlinator commented at 5:13 pm on December 16, 2024: contributor

You think this same concept is worth doing for GetTime() and SetMockTime() as well? I’d be happy to implement it if so, or you could add it to this PR if that’s easier.

Nice idea! Has anyone discussed the possibility of adding a linter to catch rogue direct calls to std::chrono::system_clock::now() etc?

marcofleon commented at 7:13 pm on December 16, 2024: contributor

Has anyone discussed the possibility of adding a linter to catch rogue direct calls to std::chrono::system_clock::now() etc?

I’m working on adding it on top of this PR, will post a branch here soon. A couple of the targets are giving me a bit of trouble… but I’ll figure it out🧐

maflcko commented at 7:39 am on December 17, 2024: member

Should that call be disabled for fuzz-tests?

It could be. Either way is harmless, so I’ll leave it as-is for now.

Edit: Pushed a commit to clarify that only ZEROS is allowed in fuzz tests.

fuzz: Clarify that only SeedRandomStateForTest(SeedRand::ZEROS) is allowed fae63bf130

maflcko force-pushed on Dec 17, 2024

hodlinator commented at 9:41 am on December 17, 2024: contributor

While I do much appreciate the intent behind adding commit fae63bf13033adec80c7e6d73144a21ea3cfbc6d, I’m worried that it will cause issues in CMake configurations such as the “dev-mode”-preset which includes both fuzzing, unit tests, and a lot of other stuff. Having

0if constexpr (G_FUZZING)

(or the negation of that) would cause unit tests to no longer use FIXED_SEEDS when fuzzing is also enabled.

Was instead thinking the fuzz-target initialization-functions would pass down a parameter into the BasicTestingSetup-ctor (maybe a new field in TestOpts?).

For the change inside SeedRandomStateForTest(), adding a dynamic g_seeded_g_prng_fixed_seed that can later be checked when fuzzing would enable unit tests to still operate in that mode.

(My suggestion would just have needlessly set g_used_g_prng in these builds when running unit tests).

I’m happy to explore implementing a dynamic version of that commit as outlined above if you don’t feel like doing it but still might approve of it.

maflcko commented at 9:53 am on December 17, 2024: member

(or the negation of that) would cause unit tests to no longer use FIXED_SEEDS when fuzzing is also enabled.

No, it is only possible to run fuzz tests with G_FUZZING, and unit tests without it. This pull request is not changing that.

In any case, I don’t really care about the commit. I am happy to drop it, or any other commit in this pull request and review a follow-up.

hodlinator approved

hodlinator commented at 12:41 pm on December 17, 2024: contributor

ACK fae63bf13033adec80c7e6d73144a21ea3cfbc6d

Makes fuzzing more deterministic through re-seeding the PRNG with the same value before testing every input. Might increase the efficiency of fuzzers in covering more code paths for some targets.

Tested removing SeedRandomStateForTest(SeedRand::ZEROS) from src/test/fuzz/process_message.cpp, ran the fuzz target and got the expected error output.

I see no remaining blockers.

No, it is only possible to run fuzz tests with G_FUZZING, and unit tests without it. This pull request is not changing that.

Ah, I was conflating BUILD_FOR_FUZZING with BUILD_FUZZ_BINARY in the “dev-mode”-preset. The unit test binary is not built when BUILD_FOR_FUZZING is set:

https://github.com/bitcoin/bitcoin/blob/fae63bf13033adec80c7e6d73144a21ea3cfbc6d/CMakeLists.txt#L211-L223

DrahtBot requested review from dergoegge on Dec 17, 2024

maflcko requested review from marcofleon on Dec 17, 2024

dergoegge approved

dergoegge commented at 1:20 pm on December 17, 2024: member

utACK fae63bf13033adec80c7e6d73144a21ea3cfbc6d

marcofleon commented at 4:14 pm on December 17, 2024: contributor

Tested ACK fae63bf13033adec80c7e6d73144a21ea3cfbc6d

The CheckGlobals module is a good addition, makes it easy to extend for other globals. I’ll open a PR that checks for system time calls after this is merged.

ryanofsky merged this on Dec 17, 2024

ryanofsky closed this on Dec 17, 2024

in src/test/fuzz/utxo_total_supply.cpp:29 in fae63bf130

25@@ -26,6 +26,7 @@ FUZZ_TARGET(utxo_total_supply)
26             .extra_args = {"-testactivationheight=bip34@2"},
27         },
28     };
29+    SeedRandomStateForTest(SeedRand::ZEROS); // Can not be done before test_setup

maflcko commented at 7:37 am on December 18, 2024:

Actually, with the third commit (fae63bf13033adec80c7e6d73144a21ea3cfbc6d), this looks now wrong. I can and must be done before test_setup.

maflcko commented at 8:14 am on December 18, 2024:

Fixed in https://github.com/bitcoin/bitcoin/pull/31521

maflcko commented at 8:12 am on December 18, 2024: member

Found a small issue after self re-review, sorry for missing it.

maflcko deleted the branch on Dec 18, 2024

fuzz: Abort when using global PRNG without re-seed #31486

Code Coverage & Benchmarks

Reviews

Conflicts