fuzz: Limit parse_univalue input length

maflcko commented at 2:17 pm on July 17, 2024: member

The new limit should be more than enough, and hopefully avoids fuzz input bloat, such as parse_univalue/0426365704e09ddd704a058cc2add1cbf104c1a9. C.f. https://cirrus-ci.com/task/6178647134961664?logs=ci#L3805

 0Run parse_univalue with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/parse_univalue')]INFO: Running with entropic power schedule (0xFF, 100).
 1INFO: Seed: 572704560
 2INFO: Loaded 1 modules   (623498 inline 8-bit counters): 623498 [0x561cba23a518, 0x561cba2d28a2), 
 3INFO: Loaded 1 PC tables (623498 PCs): 623498 [0x561cba2d28a8,0x561cbac56148), 
 4INFO:     3224 files found in /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/parse_univalue
 5INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
 6INFO: seed corpus: files: 3224 min: 1b max: 1050370b total: 25114084b rss: 112Mb
 7[#1024](/bitcoin-bitcoin/1024/)	pulse  cov: 10458 ft: 33444 corp: 906/32Kb exec/s: 341 rss: 154Mb
 8[#2048](/bitcoin-bitcoin/2048/)	pulse  cov: 12081 ft: 55461 corp: 1668/192Kb exec/s: 227 rss: 228Mb
 9Slowest unit: 15 s:
10artifact_prefix='./'; Test unit written to ./slow-unit-9df6997f2f4726843e82b3dfde46862599904d56
11Slowest unit: 309 s:
12artifact_prefix='./'; Test unit written to ./slow-unit-0426365704e09ddd704a058cc2add1cbf104c1a9
13[#3226](/bitcoin-bitcoin/3226/)	INITED cov: 12246 ft: 66944 corp: 2358/3510Kb exec/s: 6 rss: 1610Mb
14[#3226](/bitcoin-bitcoin/3226/)	DONE   cov: 12246 ft: 66944 corp: 2358/3510Kb lim: 282379 exec/s: 6 rss: 1610Mb
15Done 3226 runs in 477 second(s)

DrahtBot commented at 2:17 pm on July 17, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, brunoerg

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

DrahtBot added the label Tests on Jul 17, 2024

brunoerg approved

brunoerg commented at 4:24 pm on July 17, 2024: contributor

utACK fa8aadb8f22d322d813f91ea7d8aa7967403a820

Just verified that 10,000 is really more than enough for UniValue. I didn’t see anything bigger than 200 in practice, so it’s ok.

dergoegge commented at 7:31 am on July 18, 2024: member

Could you elaborate what exactly is making this input so slow? I doubt it’s the actual json parsing. It looks more like another descriptor parsing “bug”.

maflcko commented at 7:34 am on July 18, 2024: member

Correct, the workaround for the descriptor parsing are not applied here. So an alternative would be to apply them here.

maflcko commented at 7:36 am on July 18, 2024: member

With “workaround” I mean https://github.com/bitcoin/bitcoin/pull/30197

fuzz: Limit parse_univalue input length fa80b16b20

in src/test/fuzz/parse_univalue.cpp:21 in fa8aadb8f2 outdated

17@@ -18,6 +18,7 @@ void initialize_parse_univalue()
18 
19 FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue)
20 {
21+    if (buffer.size() > 10'000) return;

dergoegge commented at 1:20 pm on July 19, 2024:

Perhaps we move this check down to right before the descriptor related code? Afaict, there’s no need to restrict the testing of the json parsing to 10k bytes.

As a side note, this test does too many things at once in my opinion and should probably be split up.

maflcko commented at 1:39 pm on July 19, 2024:

Perhaps we move this check down to right before the descriptor related code? Afaict, there’s no need to restrict the testing of the json parsing to 10k bytes.

Good idea. Done!

brunoerg commented at 4:24 pm on July 19, 2024:

Nice

maflcko force-pushed on Jul 19, 2024

dergoegge approved

dergoegge commented at 1:41 pm on July 19, 2024: member

utACK fa80b16b20dffcb85b80f75fee64ed333f2062f9

DrahtBot requested review from brunoerg on Jul 19, 2024

brunoerg approved

brunoerg commented at 4:25 pm on July 19, 2024: contributor

utACK fa80b16b20dffcb85b80f75fee64ed333f2062f9

in src/test/fuzz/parse_univalue.cpp:80 in fa80b16b20

76@@ -77,7 +77,7 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue)
77     }
78     try {
79         FlatSigningProvider provider;
80-        (void)EvalDescriptorStringOrObject(univalue, provider);
81+        if (buffer.size() < 10'000) (void)EvalDescriptorStringOrObject(univalue, provider);

paplorinc commented at 12:51 pm on July 21, 2024:

would it make sense to use a trimmed version here instead of completely skipping the call?

maflcko commented at 7:15 am on July 22, 2024:

No, because the fuzz engine will think that the trimming code is then relevant to the fuzz target. Thus, it will treat coverage feedback in the trimming code as useful and bloat the fuzz input set. Skipping it should be effective to avoid that, apart from #27552 which should be a “complete” fix. But this can be done later.

fanquake merged this on Jul 22, 2024

fanquake closed this on Jul 22, 2024

maflcko deleted the branch on Jul 22, 2024

fuzz: Limit parse_univalue input length #30473

Code Coverage

Reviews