fuzz: replace every fuzzer-controlled while loop with a macro #22508

apoelstra · 2021-07-20T15:09:09Z

Limits the number of iterations to 1000 rather than letting the fuzzer do millions or billions of iterations on a single core.

apoelstra · 2021-07-20T15:11:09Z

There are several loops in the fuzz test where the number of iterations is directly controlled by the fuzzer, which oftentimes will go for millions of iterations, leading to certain fuzz tests taking forever on a single core. This PR limits these fuzzer-controlled loops to 1000 iterations, an arbitrary high number.

IMHO letting the fuzzer control the number of iterations directly like this, with no restrictions, is a bad idea because the fuzzer will naturally try to hit super high numbers.

I'm looking for a concept ACK (should we limit these) and an approach ACK (is doing this with a macro a reasonable way to go?). Actual ACKs/merges would also be nice :).

apoelstra · 2021-07-20T15:12:10Z

On an EPYC 7502 with 256Gb of RAM, just running time ./test/fuzz/test_runner.py ../../qa-assets/fuzz_seed_corpus/:

before, -j1

real    67m58.883s
user    152m7.845s
sys     4m36.080s

after, -j1

real    35m57.697s
user    103m10.803s
sys     3m52.910s

before, -j32

real    60m6.374s
user    153m29.631s
sys     5m14.991s

after, -j32

real    28m54.078s
user    102m52.337s
sys     4m16.605s

maflcko · 2021-07-20T15:21:57Z

Concept ACK. @DrahtBot please list the conflicts for the alternative solution that has been proposed.

DrahtBot · 2021-07-20T15:22:43Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22919 (fees: skip pointless fee parameter calculation during IBD by RohitRanjangit)
#22702 (Add allocator for node based containers by martinus)

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.

maflcko · 2021-07-20T15:34:06Z

Ok, that didn't work. Basically, it would be nice to be able to still overwrite the limit per fuzz target:

apoelstra · 2021-07-20T16:54:04Z

@MarcoFalke I added a #define variable and capped the tx_pool test to 300 iterations to demonstrate how to use it.

src/test/fuzz/tx_pool.cpp

src/test/fuzz/addrman.cpp

practicalswift · 2021-07-24T19:43:35Z

Concept ACK

Nice idea!

apoelstra · 2021-09-08T18:00:44Z

Redid the PR. Now it takes the macro from #22649 and just applies it to every single remaining fuzzer loop.

practicalswift · 2021-09-09T08:13:31Z

Thanks for fixing this!

Is the mixing of LIMITED_WHILE ( and LIMITED_WHILE( intentional? (Personally I prefer the latter.)

ACK modulo spacing consistency :)

laanwj · 2021-09-09T11:31:32Z

There are several loops in the fuzz test where the number of iterations is directly controlled by the fuzzer, which oftentimes will go for millions of iterations, leading to certain fuzz tests taking forever on a single core.

Concept and code review ACK be655e8

I would slightly prefer this "arbitrary high number" to be a constant as it's repeated everywhere.

maflcko · 2021-09-09T11:54:27Z

I would slightly prefer this "arbitrary high number" to be a constant as it's repeated everywhere.

I'd say the number is dependent on the test context. The current ones in master are 30, 300, 3000, and 30000.

I think each number needs to be picked carefully to be in the right 10^x.

Is the mixing of LIMITED_WHILE ( and LIMITED_WHILE( intentional? (Personally I prefer the latter.)

clang-format(-diff) should take care of those in any case.

apoelstra · 2021-09-09T14:22:25Z

Oops, I will fix the spacing inconsistency. It probably happened when I switched from manual edits to using sed.

@MarcoFalke I think we could probably start with an "anbitrary high number", say 30000 (or 10000), and then when when tests reveal that certain tests are taking too long we can reduce them in individual cases. The current situation effectively has a limit of infinity and things are mostly OK, so I don' t think this'll be a big deal.

Once we're at 5-digit numbers I don't think there are any situations where we'd want to increase them.

maflcko · 2021-10-05T15:11:16Z

Sorry, will review soon

apoelstra · 2021-10-25T20:03:28Z

Rebased and changed formatting to be consistent.

maflcko

ACK, beside comments

src/test/fuzz/versionbits.cpp

maflcko · 2021-11-03T09:46:31Z

src/test/fuzz/rpc.cpp

@@ -348,7 +348,7 @@ FUZZ_TARGET_INIT(rpc, initialize_rpc)
        return;
    }
    std::vector<std::string> arguments;
-    while (fuzzed_data_provider.ConsumeBool()) {
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {


This seems a bit high? ConsumeRPCArgument might call ConsumeArrayRPCArgument, so the limit is 100'000'000. A user shouldn't be passing in more than 100 items. So any value between 100-1000 seems more appropriate.

See also https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=40648

Which is running base58 encoding (expensive) of one rpc arg 6000 times, thus times out. Your limit of 10000 wouldn't affect that, I think.

ACK. I reduced it to 100.

I worry that reducing this will meaningfully affect test coverage since we presumably want to test what happens when you give gazillions of RPC arguments ... but probably a general-purpose fuzz test is the wrong place to try to hit that.

Sorry, I also meant in the line above in this file. 100 * 100 is still 10k.

src/test/fuzz/versionbits.cpp

maflcko · 2021-11-12T16:10:17Z

src/test/fuzz/rpc.cpp

@@ -294,7 +294,7 @@ std::string ConsumeScalarRPCArgument(FuzzedDataProvider& fuzzed_data_provider)
 std::string ConsumeArrayRPCArgument(FuzzedDataProvider& fuzzed_data_provider)
 {
    std::vector<std::string> scalar_arguments;
-    while (fuzzed_data_provider.ConsumeBool()) {
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {


This one I meant, too ;)

#22508 (comment)

Blindly chose a cap of 10000 iterations for every loop, except for the two in script_ops.cpp and scriptnum_ops.cpp which appeared to (sometimes) be deserializing individual bytes; capped those to one million to ensure that sometimes we try working with massive scripts. There was also one fuzzer-controlled loop in timedata.cpp which was already capped, so I left that alone. git grep 'while (fuzz' should now run clean except for timedata.cpp

maflcko · 2021-11-15T15:50:30Z

Some times:

master | this | relative | name

	206.2	76.67	0.3718234724	rpc			
	149.06	157.45	1.056286059	process_messages																				
	27.21	27.19	0.9992649761	addrman																				
	24.19	24.89	1.028937578	script_sign																				
	23.93	26.08	1.089845382	process_message																				
	23.32	20.44	0.8765008576	coins_view																				
	15.22	15.75	1.034822602	policy_estimator																				
	14.46	15.32	1.059474412	script_ops																				
	13.06	12.85	0.9839203675	bloom_filter																				
	12.35	12.41	1.0048583	pow																				
	11.93	8.79	0.7367979883	connman																				
	10.71	9.38	0.8758169935	net																				
	9.22	7.21	0.7819956616	node_eviction																				
	7.7	8.23	1.068831169	rbf

maflcko · 2021-11-15T15:51:16Z

So yeah, this should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=40648

maflcko · 2021-11-15T15:51:41Z

cr ACK 214d905

… with a macro 214d905 fuzz: replace every fuzzer-controlled loop with a LIMITED_WHILE loop (Andrew Poelstra) Pull request description: Limits the number of iterations to 1000 rather than letting the fuzzer do millions or billions of iterations on a single core. ACKs for top commit: MarcoFalke: cr ACK 214d905 Tree-SHA512: 9741c32ccd126ea656e5c93371b7136eaa2f92dc9a490dd4d39642503b1a41174f3368245153e508c3b608fe37ab89800b67ada97b740e3b5a3728bb506429d3

DrahtBot added the Tests label Jul 20, 2021

This was referenced Jul 21, 2021

fuzz: Extend addrman fuzz test with deserialize #22493

Merged

fuzz: Limit max ops in tx_pool fuzz targets #22454

Merged

fuzz: check that ser+unser produces the same AddrMan #21129

Merged

maflcko reviewed Jul 22, 2021

View reviewed changes

src/test/fuzz/tx_pool.cpp Outdated Show resolved Hide resolved

apoelstra force-pushed the 2021-07--fuzzer-loops branch from 5b26624 to 9b7a105 Compare July 22, 2021 14:56

maflcko reviewed Jul 22, 2021

View reviewed changes

src/test/fuzz/addrman.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jul 22, 2021

maflcko mentioned this pull request Aug 6, 2021

fuzz: Avoid OOM in system fuzz target #22649

Merged

apoelstra force-pushed the 2021-07--fuzzer-loops branch from 9b7a105 to be655e8 Compare September 8, 2021 18:00

DrahtBot removed the Needs rebase label Sep 8, 2021

DrahtBot mentioned this pull request Sep 9, 2021

fees: skip pointless fee parameter calculation during IBD #22919

Closed

This was referenced Sep 9, 2021

Add allocator for node based containers #22702

Closed

[p2p] Pimpl AddrMan to abstract implementation details #22950

Merged

Bossday mentioned this pull request Sep 21, 2021

. #23055

Closed

DrahtBot mentioned this pull request Oct 4, 2021

[fuzz] Use public methods in addrman fuzz tests #23053

Merged

DrahtBot added the Needs rebase label Oct 5, 2021

apoelstra force-pushed the 2021-07--fuzzer-loops branch from be655e8 to 9116bc1 Compare October 25, 2021 20:03

DrahtBot removed the Needs rebase label Oct 25, 2021

maflcko approved these changes Nov 3, 2021

View reviewed changes

apoelstra force-pushed the 2021-07--fuzzer-loops branch from 9116bc1 to 6cfcbda Compare November 12, 2021 12:24

maflcko reviewed Nov 12, 2021

View reviewed changes

src/test/fuzz/versionbits.cpp Outdated Show resolved Hide resolved

apoelstra force-pushed the 2021-07--fuzzer-loops branch from 6cfcbda to 244eb3c Compare November 12, 2021 15:57

maflcko reviewed Nov 12, 2021

View reviewed changes

apoelstra force-pushed the 2021-07--fuzzer-loops branch from 244eb3c to 214d905 Compare November 12, 2021 19:52

maflcko merged commit 36d184d into bitcoin:master Nov 15, 2021

apoelstra deleted the 2021-07--fuzzer-loops branch November 16, 2021 17:32

bitcoin locked and limited conversation to collaborators Nov 17, 2022

fuzz: replace every fuzzer-controlled while loop with a macro #22508

fuzz: replace every fuzzer-controlled while loop with a macro #22508

Uh oh!

Conversation

apoelstra commented Jul 20, 2021

Uh oh!

apoelstra commented Jul 20, 2021

Uh oh!

apoelstra commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 20, 2021

Uh oh!

DrahtBot commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Jul 20, 2021

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jul 24, 2021

Uh oh!

apoelstra commented Sep 8, 2021

Uh oh!

practicalswift commented Sep 9, 2021

Uh oh!

laanwj commented Sep 9, 2021

Uh oh!

maflcko commented Sep 9, 2021

Uh oh!

apoelstra commented Sep 9, 2021

Uh oh!

maflcko commented Oct 5, 2021

Uh oh!

apoelstra commented Oct 25, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

apoelstra Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 15, 2021

Uh oh!

maflcko commented Nov 15, 2021

Uh oh!

maflcko commented Nov 15, 2021

Uh oh!

Uh oh!

apoelstra commented Jul 20, 2021 •

edited

Loading

DrahtBot commented Jul 20, 2021 •

edited

Loading

maflcko commented Jul 20, 2021 •

edited

Loading