Fuzz timeouts may hint at bugs or performance issues. Also, they may block the fuzz CI and fuzz farms from making progress in a reasonable time.
Thus, they should be fixed.
One timeout was fixed today in commit 2b3f43b96ef0b674bf350f50f317477b8d3e1e56.
Currently OSS-Fuzz reports other timeouts (as non-deterministic):
Someone should investigate them.
The files are:
clusterfuzz-testcase-mocked_descriptor_parse-4571796256718848.not.txt.not.txt clusterfuzz-testcase-bloom_filter-5968259782148096.dmp.not.txt clusterfuzz-testcase-policy_estimator-4689779316621312.dmp.not.txt clusterfuzz-testcase-buffered_file-4703454593155072.dmp.not.txt clusterfuzz-testcase-rpc-5237241399869440.dmp.not.txt
Flame graph for mocked_descriptor_parse:
(Most of the time is spent in pubkey derive)
I can add steps to reproduce, if you want.
That would be great.
Steps to reproduce flamegraph:
perf record, for example via FUZZ=mocked_descriptor_parse perf record -g --call-graph dwarf -F 1001 ./src/test/fuzz/fuzz -runs=2 /tmp/fuzz_input.dat (-runs can be increased to discount an expensive global setup initialization, --call-graph and frequency can be modified depending on your system)hotspot ./perf.data
Another one: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64593 cc @brunoerg
I don’t have access to see it in depth 🙁.
I suppose it might be related to the descriptor parse. I think we can use ImportDescriptors from wallet_notifications in scriptpubkeyman.
I don’t have access to see it in depth 🙁.
The “Reproducer Testcase” should be public.
I suppose it might be related to the descriptor parse.
Ah, good point. This was done intentionally. :sweat_smile:
The “Reproducer Testcase” should be public.
“Access denied” when I try to access it.
Another one: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64593 cc @brunoerg
I think i’ve fixed it in #28832 (comment). @maflcko can you share the repro from oss-fuzz here so i can confirm it?
I guess it was:
0$ base64 ~/Downloads/clusterfuzz-testcase-scriptpubkeyman-5467976815738880
1dHIoJWQxLzEvMC8wMC8wMC8wLzAwLzAvMC8wLzAwLzE5LzEwOTAvMC8wLzAvMC8wLzAwLzE5LzEw
2OTAvMC8wLzAwLzAvMC8wLzAwLzEvMC8wMC8wLzAvMDAvMDkvMTkvMC8wLzAvMC8wMC8wLzAvMC8w
3MC8xOS8wLzAwMC8wMC8wLzAvMC8wMC8xLzAvMDAvMC8wLzAwLzA5LzE5LzAvMC8wLzAvMDAvMC8w
4LzAvMDAvMTkvMC8wMC8wLzAvMC8wMC8xOS8xLzAwLyonKQ==
So currently, I think, the remaining ones are:
I think this is due to a missing std::move for j: which makes BuildScript copy subs[0]: https://github.com/bitcoin/bitcoin/blob/063a8b83875997068b3eb506b5f30f2691d18052/src/script/miniscript.h#L762-L763.
EDIT: no, my bad, only the first argument to BuildScript can be moved.
Reproducing locally now.
Looks like there are different flames in descriptor_parse. Another one that took two minutes:
1eab7f1648dd012d8efee262a9898d2a9b044fd2.bin.txt
Looks like it is just a long sh(thresh(1,1,1,1,1,0,1,1,1,1,1,0,1,1,1,1....
This one is just hitting the performance of our code. Some logic in the constructor of a thresh fragment is quadratic in the number of sub-fragments. This input is a thresh with more than 130k sub-fragments.
Technically it would be possible to not have this logic in the constructor and cache it instead. But 1) that would only punt the issue 2) i don’t think it’s worth modifying the logic “just” to avoid the fuzz timeouts.
I think we’ll have to limit the size of the input, unfortunately.
Alright so i think we can still pinpoint this issue to avoid having to limit the size of the input. In the same way we limit the derivation paths depth, we could limit the number of sub-fragments per fragment in the input:
0bool HasLargeFrag(const FuzzBufferType& buff, const int max_subs)
1{
2 std::stack<int> counts;
3 for (const auto& ch: buff) {
4 if (ch == '(') {
5 counts.push(1);
6 } else if (ch == ',' && !counts.empty()) {
7 if (++counts.top() > max_subs) return true;
8 } else if (ch == ')' && !counts.empty()) {
9 counts.pop();
10 }
11 }
12 return false;
13}
I’ll PR something to this effect.
clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt
For those there is probably something to be done by someone more experienced at C++ than i am. I’ve tried fiddling around with trying to move the content of other arguments than the first one in BuildScript, which led me into unsuccessfully looking into prevector.
Instead of having to dive in this rabbit hole (which could also potentially lead to touch consensus code), i’m going to limit the number of wrappers a fragment can have in the fuzz target in the same fashion as for ruling out the fragments with too many subs.
I think we’ll have to limit the size of the input, unfortunately.
Yes, as long as all possible real-world scenarios are covered, this is fine as well.
I stumbled upon a fuzz seed that oom’ed in CI for package_rbf. Since the seed appears to be too big to paste, I provide the output of $ base64 95a3b1b42d43ac7190f24ff116b2cd6515e7a566 > /tmp/base64-95a3b1b4-oom.txt:
base64-95a3b1b4-oom.txt
If the prior format is unsatisfactory, the problematic seed 95a3b1b42d43ac7190f24ff116b2cd6515e7a566 is also in this commit to qa-assets: https://github.com/bitcoin-core/qa-assets/pull/186/commits/8d624b44419f46bcb5117baa16047dbf4bdb339f