fuzz: add target for `DescriptorScriptPubKeyMan`

brunoerg commented at 1:03 am on October 4, 2023: contributor

This PR adds fuzz target for DescriptorScriptPubKeyMan. Also, moves MockedDescriptorConverter to fuzz/util/descriptor to be used here and in descriptor target.

DrahtBot commented at 1:03 am on October 4, 2023: 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	maflcko, dergoegge
Concept ACK	Ayush170-Future

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Oct 4, 2023

in src/wallet/test/fuzz/scriptpubkeyman.cpp:26 in 6aaf739363 outdated

21+{
22+    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(ChainType::MAIN);
23+    g_setup = testing_setup.get();
24+}
25+
26+static std::string CreateDescriptorString(FuzzedDataProvider& fuzzed_data_provider)

maflcko commented at 6:59 am on October 4, 2023:

How is this different from MOCKED_DESC_CONVERTER.GetDescriptor(mocked_descriptor) ?

brunoerg commented at 12:10 pm on October 4, 2023:

Good question, let me check. If they are similar, we can move MockedDescriptorConverter to a util file and use it here.

brunoerg commented at 9:28 pm on October 4, 2023:

done!

in src/wallet/test/fuzz/scriptpubkeyman.cpp:1 in 6aaf739363 outdated

0@@ -0,0 +1,138 @@
1+// Copyright (c) 2022 The Bitcoin Core developers

maflcko commented at 7:00 am on October 4, 2023:

0// Copyright (c) 2023-present The Bitcoin Core developers

nit: Wrong year? Also, for new code could omit the years completely, or use -present to avoid having to ever touch it again.

brunoerg commented at 12:03 pm on October 4, 2023:

fixed

maflcko approved

maflcko commented at 7:01 am on October 4, 2023: member

concept ack

brunoerg force-pushed on Oct 4, 2023

DrahtBot added the label CI failed on Oct 4, 2023

brunoerg force-pushed on Oct 4, 2023

DrahtBot removed the label CI failed on Oct 5, 2023

in src/wallet/test/fuzz/scriptpubkeyman.cpp:33 in 5e78c17563 outdated

28+    MOCKED_DESC_CONVERTER.Init();
29+}
30+
31+static DescriptorScriptPubKeyMan* CreateDescriptor(FuzzedDataProvider& fuzzed_data_provider, CWallet& keystore)
32+{
33+    const auto random_bytes{ConsumeRandomLengthByteVector(fuzzed_data_provider)};

maflcko commented at 8:10 am on October 5, 2023:

can use fuzzed_data_provider.ConsumeRandomLengthString directly

brunoerg commented at 11:45 am on October 5, 2023:

done

brunoerg force-pushed on Oct 5, 2023

dergoegge commented at 2:42 pm on October 10, 2023: member

0$ echo "cGsoJTg0KVwzKVwAQWlwZTgzKVwAQQAAAAAAADmDg4ODg4ODg4ODgYODg4ODg4Nwa2hhe3t7g4ODg4ODg4OD" | base64 --decode > scriptpubkeyman.crash
1$ FUZZ=scriptpubkeyman ./src/test/fuzz/fuzz scriptpubkeyman.crash
2...
3Running: scriptpubkeyman.crash
4fuzz: wallet/test/fuzz/scriptpubkeyman.cpp:93: auto wallet::(anonymous namespace)::scriptpubkeyman_fuzz_target(FuzzBufferType)::(anonymous class)::operator()() const: Assertion `spk_manager->GetDescriptorString(descriptor, fuzzed_data_provider.ConsumeBool())' failed.
5==1391778== ERROR: libFuzzer: deadly signal
6...

brunoerg force-pushed on Oct 10, 2023

brunoerg commented at 6:46 pm on October 10, 2023: contributor

Thanks, @dergoegge. Fixed it by removing the assert, we can’t ensure it.

dergoegge commented at 8:45 am on October 11, 2023: member

0$ echo "cGsoJTQxLyopXDMAXABhZGRyKGJjMYMxMTAwMDIwMDAwMDBnJyw2ZGRyKGI1bikp" | base64 --decode > scriptpubkeyman2.crash
1$ FUZZ=scriptpubkeyman ./src/test/fuzz/fuzz scriptpubkeyman2.crash
2...
3fuzz: wallet/scriptpubkeyman.cpp:2008: virtual util::Result<CTxDestination> wallet::DescriptorScriptPubKeyMan::GetNewDestination(const OutputType): Assertion `desc_addr_type' failed.
4==1499877== ERROR: libFuzzer: deadly signal
5...

brunoerg force-pushed on Oct 11, 2023

dergoegge commented at 9:09 am on October 12, 2023: member

0$ echo "dHIoJTA1LyopXDQXFw=="  | base64 --decode > scriptpubkeyman3.crash
1$ FUZZ=scriptpubkeyman ./src/test/fuzz/fuzz scriptpubkeyman3.crash
2...
3terminate called after throwing an instance of 'std::runtime_error'
4  what():  GetNewDestination: Types are inconsistent. Stored type does not match type of newly generated address
5...

I would recommend that you run the targets that you write for a while before opening a PR. It could very well be that it finds actual bugs that we don’t want to be public right away.

brunoerg force-pushed on Oct 12, 2023

brunoerg commented at 8:50 pm on October 12, 2023: contributor

I would recommend that you run the targets that you write for a while before opening a PR. It could very well be that it finds actual bugs that we don’t want to be public right away.

Sure, sorry for that, left it running but not in my main machine so didn’t get some of them. Force-pushed with a fix for GetNewDestination. We can’t fuzz it with random outputs types because any inconsistence will throw a runtime error. Any other one will return a util::Error. Left it running for some hours before pushing and will continue running it overnight.

DrahtBot added the label CI failed on Oct 18, 2023

DrahtBot removed the label CI failed on Oct 20, 2023

Ayush170-Future approved

Ayush170-Future commented at 5:34 pm on November 4, 2023: contributor

ACK

Reviewed the whole code. Looks great to me!

dergoegge commented at 9:49 am on November 6, 2023: member

Here’s a coverage report: https://dergoegge.github.io/bitcoin-coverage/pr28578/fuzz.coverage/index.html (~1000h CPU time).

Is there a reason not to cover more of DescriptorScriptPubKeyMan in this PR? e.g. DescriptorScriptPubKeyMan::SignTransaction or DescriptorScriptPubKeyMan::FillPSBT.

brunoerg commented at 9:19 pm on November 6, 2023: contributor

Is there a reason not to cover more of DescriptorScriptPubKeyMan in this PR? e.g. DescriptorScriptPubKeyMan::SignTransaction or DescriptorScriptPubKeyMan::FillPSBT.

Not exactly, perhaps one of them may be very slow, let me test more functions and then I can add them all here, np.

brunoerg force-pushed on Nov 8, 2023

brunoerg commented at 2:05 pm on November 8, 2023: contributor

Force-pushed covering more functions. Left it running for a long time and didn’t see any downside.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:154 in 43f7fc6a9e outdated

141+            },
142+            [&] {
143+                std::optional<PartiallySignedTransaction> opt_psbt{ConsumeDeserializable<PartiallySignedTransaction>(fuzzed_data_provider)};
144+                if (!opt_psbt) {
145+                    return;
146+                }

dergoegge commented at 12:00 pm on November 10, 2023:

We probably want to do what marco did in #28815 here, i.e. break out of the loop on “bad” data

brunoerg commented at 12:46 pm on November 10, 2023:

Yes, I’m addressing it.

brunoerg commented at 1:42 pm on November 10, 2023:

done

brunoerg force-pushed on Nov 10, 2023

brunoerg commented at 1:43 pm on November 10, 2023: contributor

Force-pushed addressing #28578 (review)

in src/wallet/test/fuzz/scriptpubkeyman.cpp:35 in ca5f6ac306 outdated

30+
31+static std::optional<std::pair<WalletDescriptor, FlatSigningProvider>> CreateWalletDescriptor(FuzzedDataProvider& fuzzed_data_provider)
32+{
33+    const std::string mocked_descriptor{fuzzed_data_provider.ConsumeRandomLengthString()};
34+    const auto desc_str{MOCKED_DESC_CONVERTER.GetDescriptor(mocked_descriptor)};
35+    if (!desc_str.has_value()) return std::nullopt;

dergoegge commented at 9:40 am on November 13, 2023:

I think this runs into the same timeouts as the mocked_descriptor_parse target. We should probably wait for #28832 to be merged so HasDeepDerivPath can be used here as well.

brunoerg commented at 11:56 am on November 13, 2023:

I agree. I will draft this to wait for #28832.

brunoerg marked this as a draft on Nov 13, 2023

brunoerg commented at 11:58 am on November 13, 2023: contributor

To avoid timeouts in this target, I will leave it as draft until #28815 (or other solution) gets merged.

DrahtBot added the label CI failed on Nov 16, 2023

maflcko commented at 11:11 am on November 20, 2023: member

I think this can go ahead without having to wait on the unrelated and separately handled pull request to fix a timeout in a function that is called in this target? Seems odd to block this, if it is useful. Better to have some fuzz inputs time out than to have no fuzz coverage?

brunoerg force-pushed on Nov 20, 2023

brunoerg commented at 3:02 pm on November 20, 2023: contributor

force-pushed to fix CI error

brunoerg marked this as ready for review on Nov 20, 2023

in src/test/fuzz/util/descriptor.h:8 in 7774df9694 outdated

0@@ -0,0 +1,47 @@
1+// Copyright (c) 2023-present The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_TEST_FUZZ_UTIL_DESCRIPTOR_H
6+#define BITCOIN_TEST_FUZZ_UTIL_DESCRIPTOR_H
7+
8+#include <functional>

maflcko commented at 3:12 pm on November 20, 2023:

style nit: Please put std:: includes in a new section

brunoerg commented at 7:02 pm on November 20, 2023:

done

in src/wallet/test/fuzz/scriptpubkeyman.cpp:57 in 7774df9694 outdated

52+
53+FUZZ_TARGET(scriptpubkeyman, .init = initialize_spkm)
54+{
55+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
56+    const auto& node{g_setup->m_node};
57+    Chainstate* chainstate = &node.chainman->ActiveChainstate();

maflcko commented at 3:14 pm on November 20, 2023:

0    Chainstate& chainstate = node.chainman->ActiveChainstate();

any reason to turn a reference into a pointer when you can keep the reference?

brunoerg commented at 7:02 pm on November 20, 2023:

nop! done.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:42 in 7774df9694 outdated

37+    FlatSigningProvider keys;
38+    std::string error;
39+    std::unique_ptr<Descriptor> parsed_desc{Parse(desc_str.value(), keys, error, false)};
40+    if (!parsed_desc) return std::nullopt;
41+
42+    WalletDescriptor w_desc{WalletDescriptor(std::move(parsed_desc), 0, 0, 1, 1)};

maflcko commented at 3:15 pm on November 20, 2023:

0    WalletDescriptor w_desc{(std::move(parsed_desc), 0, 0, 1, 1)};

nit: no need to mention the type twice for a single symbol

maflcko commented at 3:25 pm on November 20, 2023:

Using clang-tidy named args would be better than just listing 0,0,1,1, which is hard to understand without meaning.

brunoerg commented at 7:02 pm on November 20, 2023:

done

maflcko commented at 3:33 pm on November 20, 2023: member

lgtm ACK 7774df96942ba10aeb5c818b100aedc9cb7d4b4f

DrahtBot requested review from Ayush170-Future on Nov 20, 2023

DrahtBot removed the label CI failed on Nov 20, 2023

fuzz: move `MockedDescriptorConverter` to `fuzz/util` 2e1833ca13

fuzz: create ConsumeCoins 641dddf018

fuzz: add target for `DescriptorScriptPubKeyMan` 47e5c9994c

brunoerg force-pushed on Nov 20, 2023

brunoerg commented at 7:03 pm on November 20, 2023: contributor

force-pushed addressing: #28578 (review) #28578 (review) #28578 (review)

maflcko commented at 7:46 am on November 21, 2023: member

lgtm ACK 47e5c9994c087d1826ccc0d539e916845b5648fb 🏓

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 47e5c9994c087d1826ccc0d539e916845b5648fb 🏓
3n6LCX8TDcBqt4LlIUR6QxLtOOGcMzxg9UC9JX6H6d7ZkevyCsXylwzmblnXut4yESlWL+QQbvGi+mc5kXm16BQ==

DrahtBot removed review request from Ayush170-Future on Nov 21, 2023

DrahtBot requested review from Ayush170-Future on Nov 21, 2023

fanquake requested review from dergoegge on Nov 22, 2023

dergoegge approved

dergoegge commented at 12:50 pm on November 23, 2023: member

ACK 47e5c9994c087d1826ccc0d539e916845b5648fb

DrahtBot removed review request from Ayush170-Future on Nov 23, 2023

DrahtBot requested review from Ayush170-Future on Nov 23, 2023

fanquake merged this on Nov 23, 2023

fanquake closed this on Nov 23, 2023

in src/wallet/test/fuzz/scriptpubkeyman.cpp:102 in 47e5c9994c

 97+                    assert(spk_manager->IsMine(spk) == ISMINE_SPENDABLE);
 98+                    CTxDestination dest;
 99+                    bool extract_dest{ExtractDestination(spk, dest)};
100+                    if (extract_dest) {
101+                        const std::string msg{fuzzed_data_provider.ConsumeRandomLengthString()};
102+                        PKHash pk_hash{fuzzed_data_provider.ConsumeBool() ? PKHash{ConsumeUInt160(fuzzed_data_provider)} : *std::get_if<PKHash>(&dest)};

maflcko commented at 11:22 am on November 24, 2023:

*std::get_if may be UB? In any case, it would be good to submit fuzz inputs that cover everything here.

fanquake commented at 3:16 pm on November 25, 2023: member

Null-dereference READ · wallet::scriptpubkeyman_fuzz_target

 0Command: /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds-honggfuzz_bitcoin-core_83aef6625aaeafa301867de74608b320f3c923fe/revisions/scriptpubkeyman
 1	Time ran: 0.5496230125427246
 2	
 3	Accepting input from '[STDIN]'
 4	Usage for fuzzing: honggfuzz -P [flags] -- /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds-honggfuzz_bitcoin-core_83aef6625aaeafa301867de74608b320f3c923fe/revisions/scriptpubkeyman
 5	AddressSanitizer:DEADLYSIGNAL
 6	=================================================================
 7	==14133==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x79a1f8670aeb bp 0x7ffd95472db0 sp 0x7ffd95472578 T0)
 8	==14133==The signal is caused by a READ memory access.
 9	==14133==Hint: address points to the zero page.
10	SCARINESS: 10 (null-deref)
11	    [#0](/bitcoin-bitcoin/0/) 0x79a1f8670aeb in memcpy /build/glibc-SzIz7B/glibc-2.31/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:142
12	    [#1](/bitcoin-bitcoin/1/) 0x589ab4429a18 in __asan_memcpy /src/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
13	    [#2](/bitcoin-bitcoin/2/) 0x589ab44cf869 in operator() [bitcoin-core/src/wallet/test/fuzz/scriptpubkeyman.cpp:0](https://github.com/bitcoin/bitcoin/blob/b5a271334ca81a6adcb1c608d85c83621a9eae47/src/wallet/test/fuzz/scriptpubkeyman.cpp#L0)
14	    [#3](/bitcoin-bitcoin/3/) 0x589ab44cf869 in CallOneOf<(lambda at wallet/test/fuzz/scriptpubkeyman.cpp:75:13), (lambda at wallet/test/fuzz/scriptpubkeyman.cpp:87:13), (lambda at wallet/test/fuzz/scriptpubkeyman.cpp:94:13), (lambda at wallet/test/fuzz/scriptpubkeyman.cpp:108:13), (lambda at wallet/test/fuzz/scriptpubkeyman.cpp:117:13), (lambda at wallet/test/fuzz/scriptpubkeyman.cpp:121:13), (lambda at wallet/test/fuzz/scriptpubkeyman.cpp:135:13), (lambda at wallet/test/fuzz/scriptpubkeyman.cpp:149:13)> [bitcoin-core/src/test/fuzz/util.h:43](https://github.com/bitcoin/bitcoin/blob/b5a271334ca81a6adcb1c608d85c83621a9eae47/src/test/fuzz/util.h#L43):27
15	    [#4](/bitcoin-bitcoin/4/) 0x589ab44cf869 in wallet::(anonymous namespace)::scriptpubkeyman_fuzz_target(Span<unsigned char const>) [bitcoin-core/src/wallet/test/fuzz/scriptpubkeyman.cpp:73](https://github.com/bitcoin/bitcoin/blob/b5a271334ca81a6adcb1c608d85c83621a9eae47/src/wallet/test/fuzz/scriptpubkeyman.cpp#L73):9
16	    [#5](/bitcoin-bitcoin/5/) 0x589ab446d2d1 in __invoke<void (*&)(Span<const unsigned char>), Span<const unsigned char> > /usr/local/include/c++/v1/type_traits:3592:23
17	    [#6](/bitcoin-bitcoin/6/) 0x589ab446d2d1 in __call<void (*&)(Span<const unsigned char>), Span<const unsigned char> > /usr/local/include/c++/v1/__functional/invoke.h:61:9
18	    [#7](/bitcoin-bitcoin/7/) 0x589ab446d2d1 in operator() /usr/local/include/c++/v1/__functional/function.h:181:16
19	    [#8](/bitcoin-bitcoin/8/) 0x589ab446d2d1 in std::__1::__function::__func<void (*)(Span<unsigned char const>), std::__1::allocator<void (*)(Span<unsigned char const>)>, void (Span<unsigned char const>)>::operator()(Span<unsigned char const>&&) /usr/local/include/c++/v1/__functional/function.h:355:12
20	    [#9](/bitcoin-bitcoin/9/) 0x589ab49ef0b8 in operator() /usr/local/include/c++/v1/__functional/function.h:508:16
21	    [#10](/bitcoin-bitcoin/10/) 0x589ab49ef0b8 in operator() /usr/local/include/c++/v1/__functional/function.h:1185:12
22	    [#11](/bitcoin-bitcoin/11/) 0x589ab49ef0b8 in LLVMFuzzerTestOneInput [bitcoin-core/src/test/fuzz/fuzz.cpp:178](https://github.com/bitcoin/bitcoin/blob/b5a271334ca81a6adcb1c608d85c83621a9eae47/src/test/fuzz/fuzz.cpp#L178):5
23	    [#12](/bitcoin-bitcoin/12/) 0x589ab642bdeb in main
24	    [#13](/bitcoin-bitcoin/13/) 0x79a1f85d9082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
25	    [#14](/bitcoin-bitcoin/14/) 0x589ab43a93bd in _start

maflcko commented at 3:01 pm on November 29, 2023: member

Fixed the crash in https://github.com/bitcoin/bitcoin/pull/28968

fanquake referenced this in commit d00d50e78a on Nov 29, 2023

hebasto referenced this in commit 6da3579d34 on Mar 29, 2024

hebasto referenced this in commit 43f5de0612 on Mar 29, 2024

hebasto referenced this in commit 9397478cd5 on Apr 2, 2024

hebasto referenced this in commit b2c1f64a1e on Apr 2, 2024

bitcoin locked this on Nov 28, 2024

fuzz: add target for DescriptorScriptPubKeyMan #28578

Code Coverage

Reviews

Conflicts

fuzz: add target for `DescriptorScriptPubKeyMan` #28578