Currently, when running the fuzz targets under asan, leak detection may be expensive when global memory is involved on every iteration. For example when SeedRandomStateForTest(SeedRand::ZEROS) is called on every iteration.
Fix it by disabling leak check on every iteration, but still do it at the end.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31481.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
277@@ -278,6 +278,7 @@ def job(command, t, t_env):
278 use_value_profile = int(random.random() < .3)
279 command = [
280 fuzz_bin,
281+ "-detect_leaks=0", # Disable leak detection on every iteration, it is still run on shutdown.
libfuzzer will usually do this by itself, after some time:
0...
1INFO: libFuzzer disabled leak detection after every mutation.
2 Most likely the target function accumulates allocated
3 memory in a global state w/o actually leaking it.
4 You may try running this binary with -trace_malloc=[12] to get a trace of mallocs and frees.
5 If LeakSanitizer is enabled in this process it will still
6 run on the process shutdown.
7...
However, it seems better to not rely on that and set the desired value from the start.
This message helped me find a bug recently (I was fuzzing something affecting global state and forgot to clean up after each mutation, so some global vector would grow with each iteration). While that was just a bug in my fuzzing code, I guess the same could happen for some remotely triggerable memory-exhaustion attacks.
So there may be some value in this info message itself (but obviously only if someone is actually looking at it).
I’d say the -rss_limit_mb setting is a better tool to find OOM issues than the leak detector. I understand that the default setting in libfuzzer, or the setting in this file for rss_limit_mb may not be effective at that.
Not sure what the best solution to this problem, but possibly rss_limit_mb could be set on a per-fuzz-target basis, possibly with a scaling factor for sanitizer builds with higher overhead?
-l DEBUG.
77@@ -78,6 +78,14 @@ void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target,
78 static std::string_view g_fuzz_target;
79 static const TypeTestOneInput* g_test_one_input{nullptr};
80
81+inline void test_one_input(FuzzBufferType buffer)
82+{
83+ // Re-seed the global random state for every input, to make every execution
84+ // deterministic and not depend on a previous unrelated input.
85+ SeedRandomStateForTest(SeedRand::ZEROS);
addition_overflow) and for me the iterations dropped by about 20%. I think this overall overhead is fine, even if not all fuzz targets need it. This is because any fast target will likely already be saturated, or can be saturated quickly, and any slow target will not see the overhead in the overall speed.
SeedRand::ZEROS. Happy to do this, if you think it is worth it.
For reference, asan has leak detection enabled by default, and it is also set in test/fuzz/test_runner.py in ASAN_OPTIONS.
For testing that leak detection still works, one can use a diff, something like:
0diff --git a/src/test/fuzz/addition_overflow.cpp b/src/test/fuzz/addition_overflow.cpp
1index 071e5fb029..a8c52d236a 100644
2--- a/src/test/fuzz/addition_overflow.cpp
3+++ b/src/test/fuzz/addition_overflow.cpp
4@@ -44,6 +44,10 @@ void TestAdditionOverflow(FuzzedDataProvider& fuzzed_data_provider)
5
6 FUZZ_TARGET(addition_overflow)
7 {
8+ const auto* leak = new uint8_t[777]{};
9+ FuzzedDataProvider fdp(leak, 777);
10+ TestAdditionOverflow<int64_t>(fdp);
11+
12 FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
13 TestAdditionOverflow<int64_t>(fuzzed_data_provider);
14 TestAdditionOverflow<uint64_t>(fuzzed_data_provider);
277@@ -278,6 +278,7 @@ def job(command, t, t_env):
278 use_value_profile = int(random.random() < .3)
(Comment about job-function further up)
Suggested logging improvement in commit 3dd6ef5c174ce36119660cd2ccc82ab09d82300b, here tested with early leak-detection still enabled.
0Generating corpus to foo
1Running '['/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz', '-rss_limit_mb=8000', '-max_total_time=10', '-reload=0', '-use_value_profile=0', PosixPath('foo/addition_overflow')]'
2Traceback (most recent call last):
3 File "/home/hodlinator/bitcoin/build_fuzz/test/fuzz/test_runner.py", line 412, in <module>
4 main()
5 File "/home/hodlinator/bitcoin/build_fuzz/test/fuzz/test_runner.py", line 180, in main
6 return generate_corpus(
7 ^^^^^^^^^^^^^^^^
8 File "/home/hodlinator/bitcoin/build_fuzz/test/fuzz/test_runner.py", line 291, in generate_corpus
9 future.result()
10 File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/concurrent/futures/_base.py", line 449, in result
11 return self.__get_result()
12 ^^^^^^^^^^^^^^^^^^^
13 File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/concurrent/futures/_base.py", line 401, in __get_result
14 raise self._exception
15 File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/concurrent/futures/thread.py", line 58, in run
16 result = self.fn(*self.args, **self.kwargs)
17 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
18 File "/home/hodlinator/bitcoin/build_fuzz/test/fuzz/test_runner.py", line 262, in job
19 subprocess.run(
20 File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/subprocess.py", line 571, in run
21 raise CalledProcessError(retcode, process.args,
22subprocess.CalledProcessError: Command '['/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz', '-rss_limit_mb=8000', '-max_total_time=10', '-reload=0', '-use_value_profile=0', PosixPath('foo/addition_overflow')]' returned non-zero exit status 77.
0Generating corpus to foo
1Running '['/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz', '-rss_limit_mb=8000', '-max_total_time=10', '-reload=0', '-use_value_profile=0', PosixPath('foo/addition_overflow')]'
2Command '['/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz', '-rss_limit_mb=8000', '-max_total_time=10', '-reload=0', '-use_value_profile=0', PosixPath('foo/addition_overflow')]' output:
3'INFO: Running with entropic power schedule (0xFF, 100).
4INFO: Seed: 168855887
5INFO: Loaded 1 modules (1305991 inline 8-bit counters): 1305991 [0x561fd5b16200, 0x561fd5c54f87),
6INFO: Loaded 1 PC tables (1305991 PCs): 1305991 [0x561fd5c54f88,0x561fd70427f8),
7INFO: 0 files found in foo/addition_overflow
8INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
9INFO: A corpus is not provided, starting from an empty corpus
10[#2](/bitcoin-bitcoin/2/) INITED cov: 797 ft: 798 corp: 1/1b exec/s: 0 rss: 230Mb
11==3390108==WARNING: invalid path to external symbolizer!
12==3390108==WARNING: Failed to use and restart external symbolizer!
13
14=================================================================
15==3390108==ERROR: LeakSanitizer: detected memory leaks
16
17Direct leak of 777 byte(s) in 1 object(s) allocated from:
18 [#0](/bitcoin-bitcoin/0/) 0x561fca60f978 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5da0978)
19 [#1](/bitcoin-bitcoin/1/) 0x561fca6129c9 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5da39c9)
20 [#2](/bitcoin-bitcoin/2/) 0x561fca61c3c9 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5dad3c9)
21 [#3](/bitcoin-bitcoin/3/) 0x561fca61c01f (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5dad01f)
22 [#4](/bitcoin-bitcoin/4/) 0x561fca61b603 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5dac603)
23 [#5](/bitcoin-bitcoin/5/) 0x561fcc4984de (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x7c294de)
24 [#6](/bitcoin-bitcoin/6/) 0x561fcc493c83 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x7c24c83)
25 [#7](/bitcoin-bitcoin/7/) 0x561fcc49391f (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x7c2491f)
26 [#8](/bitcoin-bitcoin/8/) 0x561fca4b3ab8 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c44ab8)
27 [#9](/bitcoin-bitcoin/9/) 0x561fca4b66a0 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c476a0)
28 [#10](/bitcoin-bitcoin/10/) 0x561fca4b7acb (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c48acb)
29 [#11](/bitcoin-bitcoin/11/) 0x561fca4b7ce7 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c48ce7)
30 [#12](/bitcoin-bitcoin/12/) 0x561fca4986d1 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c296d1)
31 [#13](/bitcoin-bitcoin/13/) 0x561fca29cd72 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5a2dd72)
32 [#14](/bitcoin-bitcoin/14/) 0x7ff7486dd27d (/nix/store/sl141d1g77wvhr050ah87lcyz2czdxa3-glibc-2.40-36/lib/libc.so.6+0x2a27d) (BuildId: 6a788357de379aee7162f608d25a7692f7162b15)
33
34Direct leak of 777 byte(s) in 1 object(s) allocated from:
35 [#0](/bitcoin-bitcoin/0/) 0x561fca60f978 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5da0978)
36 [#1](/bitcoin-bitcoin/1/) 0x561fca6129c9 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5da39c9)
37 [#2](/bitcoin-bitcoin/2/) 0x561fca61c3c9 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5dad3c9)
38 [#3](/bitcoin-bitcoin/3/) 0x561fca61c01f (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5dad01f)
39 [#4](/bitcoin-bitcoin/4/) 0x561fca61b603 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5dac603)
40 [#5](/bitcoin-bitcoin/5/) 0x561fcc4984de (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x7c294de)
41 [#6](/bitcoin-bitcoin/6/) 0x561fcc493c83 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x7c24c83)
42 [#7](/bitcoin-bitcoin/7/) 0x561fcc49391f (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x7c2491f)
43 [#8](/bitcoin-bitcoin/8/) 0x561fca4b3ab8 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c44ab8)
44 [#9](/bitcoin-bitcoin/9/) 0x561fca4b66a0 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c476a0)
45 [#10](/bitcoin-bitcoin/10/) 0x561fca4b72b0 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c482b0)
46 [#11](/bitcoin-bitcoin/11/) 0x561fca4b7ee7 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c48ee7)
47 [#12](/bitcoin-bitcoin/12/) 0x561fca4986d1 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c296d1)
48 [#13](/bitcoin-bitcoin/13/) 0x561fca29cd72 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5a2dd72)
49 [#14](/bitcoin-bitcoin/14/) 0x7ff7486dd27d (/nix/store/sl141d1g77wvhr050ah87lcyz2czdxa3-glibc-2.40-36/lib/libc.so.6+0x2a27d) (BuildId: 6a788357de379aee7162f608d25a7692f7162b15)
50
51Direct leak of 777 byte(s) in 1 object(s) allocated from:
52 [#0](/bitcoin-bitcoin/0/) 0x561fca60f978 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5da0978)
53 [#1](/bitcoin-bitcoin/1/) 0x561fca6129c9 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5da39c9)
54 [#2](/bitcoin-bitcoin/2/) 0x561fca61c3c9 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5dad3c9)
55 [#3](/bitcoin-bitcoin/3/) 0x561fca61c01f (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5dad01f)
56 [#4](/bitcoin-bitcoin/4/) 0x561fca61b603 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5dac603)
57 [#5](/bitcoin-bitcoin/5/) 0x561fcc4984de (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x7c294de)
58 [#6](/bitcoin-bitcoin/6/) 0x561fcc493c83 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x7c24c83)
59 [#7](/bitcoin-bitcoin/7/) 0x561fcc49391f (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x7c2491f)
60 [#8](/bitcoin-bitcoin/8/) 0x561fca4b3ab8 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c44ab8)
61 [#9](/bitcoin-bitcoin/9/) 0x561fca4b76d4 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c486d4)
62 [#10](/bitcoin-bitcoin/10/) 0x561fca4b7ce7 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c48ce7)
63 [#11](/bitcoin-bitcoin/11/) 0x561fca4986d1 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5c296d1)
64 [#12](/bitcoin-bitcoin/12/) 0x561fca29cd72 (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5a2dd72)
65 [#13](/bitcoin-bitcoin/13/) 0x7ff7486dd27d (/nix/store/sl141d1g77wvhr050ah87lcyz2czdxa3-glibc-2.40-36/lib/libc.so.6+0x2a27d) (BuildId: 6a788357de379aee7162f608d25a7692f7162b15)
66
67SUMMARY: AddressSanitizer: 2331 byte(s) leaked in 3 allocation(s).
68INFO: to ignore leaks on libFuzzer side use -detect_leaks=0.
69
70MS: 1 ChangeBit-; base unit: adc83b19e793491b1c6ea0fd8b46cd9f32e592fc
710x8,
72\010
73artifact_prefix='./'; Test unit written to ./leak-8d883f1577ca8c334b7c6d75ccb71209d71ced13
74Base64: CA==
75'
ACK eeee163513918540f483925f36b3399430de1c57
Changed -max_total_time=60 beneath the line this PR is modifying and ran with & without -detect_leaks=0. The performance improvement is VERY stark. The number of fuzz inputs generated where either around 67 (2 tests) without, and with, it was a staggering 1937 (average from 2 runs). Around 28x faster.
Without this PR, fuzzing stops early after 3 leaking runs (using suggested diff adding intential leaks). With this PR, it completes before reporting leaks. This matches the docs at https://llvm.org/docs/LibFuzzer.html which state:
-detect_leaks If 1 (default) and if LeakSanitizer is enabled try to detect memory leaks during fuzzing (i.e. not only at shut down).
Would prefer the title be renamed “fuzz: Faster leak check when generating corpus”, or at least pointing out that it is this part which is optimized in the PR summary.
More correct size?
0FuzzedDataProvider fdp(leak, 777);
Performance increase
Changed
-max_total_time=60beneath the line this PR is modifying and ran with & without-detect_leaks=0. The performance improvement is VERY stark. The number of fuzz inputs generated where either around 67 (2 tests) without, and with, it was a staggering 1937 (average from 2 runs). Around 28x faster.
Just for completeness, the performance increase should be mild overall, assuming that the leak detection is turned off automatically anyway for long runs, but I haven’t confirmed this.
If #31481 (review) is too controversial, an alternative could be to just disable it when empty_min_time is active, or just close this pull.
Just for completeness, the performance increase should be mild overall, assuming that the leak detection is turned off automatically anyway for long runs, but I haven’t confirmed this.
If #31481 (comment) is too controversial, an alternative could be to just disable it when
empty_min_timeis active, or just close this pull.
Yeah, if libfuzzer automatically stops leak detection after a while, I can see a case for closing this PR too, haven’t seriously generated corpuses yet, so weigh my ACK accordingly.