fuzz: add target for `DescriptorScriptPubKeyMan` #28578

pull brunoerg wants to merge 3 commits into bitcoin:master from brunoerg:2023-07-fuzz-scriptpubkey-descriptor changing 9 files +311 −108
  1. 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.

  2. DrahtBot commented at 1:03 AM on October 4, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Oct 4, 2023
  4. 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!

  5. 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: 
    // 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

  6. maflcko approved
  7. maflcko commented at 7:01 AM on October 4, 2023: member

    concept ack

  8. brunoerg force-pushed on Oct 4, 2023
  9. DrahtBot added the label CI failed on Oct 4, 2023
  10. brunoerg force-pushed on Oct 4, 2023
  11. DrahtBot removed the label CI failed on Oct 5, 2023
  12. 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

  13. brunoerg force-pushed on Oct 5, 2023
  14. dergoegge commented at 2:42 PM on October 10, 2023: member 
    $ echo "cGsoJTg0KVwzKVwAQWlwZTgzKVwAQQAAAAAAADmDg4ODg4ODg4ODgYODg4ODg4Nwa2hhe3t7g4ODg4ODg4OD" | base64 --decode > scriptpubkeyman.crash
$ FUZZ=scriptpubkeyman ./src/test/fuzz/fuzz scriptpubkeyman.crash
...
Running: scriptpubkeyman.crash
fuzz: 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.
==1391778== ERROR: libFuzzer: deadly signal
...
  15. brunoerg force-pushed on Oct 10, 2023
  16. brunoerg commented at 6:46 PM on October 10, 2023: contributor

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

  17. dergoegge commented at 8:45 AM on October 11, 2023: member 
    $ echo "cGsoJTQxLyopXDMAXABhZGRyKGJjMYMxMTAwMDIwMDAwMDBnJyw2ZGRyKGI1bikp" | base64 --decode > scriptpubkeyman2.crash
$ FUZZ=scriptpubkeyman ./src/test/fuzz/fuzz scriptpubkeyman2.crash
...
fuzz: wallet/scriptpubkeyman.cpp:2008: virtual util::Result<CTxDestination> wallet::DescriptorScriptPubKeyMan::GetNewDestination(const OutputType): Assertion `desc_addr_type' failed.
==1499877== ERROR: libFuzzer: deadly signal
...
  18. brunoerg force-pushed on Oct 11, 2023
  19. dergoegge commented at 9:09 AM on October 12, 2023: member 
    $ echo "dHIoJTA1LyopXDQXFw=="  | base64 --decode > scriptpubkeyman3.crash
$ FUZZ=scriptpubkeyman ./src/test/fuzz/fuzz scriptpubkeyman3.crash
...
terminate called after throwing an instance of 'std::runtime_error'
  what():  GetNewDestination: Types are inconsistent. Stored type does not match type of newly generated address
...

    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.

  20. brunoerg force-pushed on Oct 12, 2023
  21. 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.

  22. DrahtBot added the label CI failed on Oct 18, 2023
  23. DrahtBot removed the label CI failed on Oct 20, 2023
  24. Ayush170-Future approved
  25. Ayush170-Future commented at 5:34 PM on November 4, 2023: contributor

    ACK

    Reviewed the whole code. Looks great to me!

  26. 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.

  27. 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.

  28. brunoerg force-pushed on Nov 8, 2023
  29. 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.

  30. 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

  31. brunoerg force-pushed on Nov 10, 2023
  32. brunoerg commented at 1:43 PM on November 10, 2023: contributor

    Force-pushed addressing #28578 (review)

  33. 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.

  34. brunoerg marked this as a draft on Nov 13, 2023
  35. 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.

  36. DrahtBot added the label CI failed on Nov 16, 2023
  37. 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?

  38. brunoerg force-pushed on Nov 20, 2023
  39. brunoerg commented at 3:02 PM on November 20, 2023: contributor

    force-pushed to fix CI error

  40. brunoerg marked this as ready for review on Nov 20, 2023
  41. 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

  42. 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: 
        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.

  43. 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: 
        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

  44. maflcko commented at 3:33 PM on November 20, 2023: member

    lgtm ACK 7774df96942ba10aeb5c818b100aedc9cb7d4b4f

  45. DrahtBot requested review from Ayush170-Future on Nov 20, 2023
  46. DrahtBot removed the label CI failed on Nov 20, 2023
  47. fuzz: move `MockedDescriptorConverter` to `fuzz/util` 2e1833ca13
  48. fuzz: create ConsumeCoins 641dddf018
  49. fuzz: add target for `DescriptorScriptPubKeyMan` 47e5c9994c
  50. brunoerg force-pushed on Nov 20, 2023
  51. brunoerg commented at 7:03 PM on November 20, 2023: contributor

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

  52. maflcko commented at 7:46 AM on November 21, 2023: member

    lgtm ACK 47e5c9994c087d1826ccc0d539e916845b5648fb 🏓

    <details><summary>Show signature</summary>

    Signature:

    untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 47e5c9994c087d1826ccc0d539e916845b5648fb 🏓
n6LCX8TDcBqt4LlIUR6QxLtOOGcMzxg9UC9JX6H6d7ZkevyCsXylwzmblnXut4yESlWL+QQbvGi+mc5kXm16BQ==

    </details>

  53. DrahtBot removed review request from Ayush170-Future on Nov 21, 2023
  54. DrahtBot requested review from Ayush170-Future on Nov 21, 2023
  55. fanquake requested review from dergoegge on Nov 22, 2023
  56. dergoegge approved
  57. dergoegge commented at 12:50 PM on November 23, 2023: member

    ACK 47e5c9994c087d1826ccc0d539e916845b5648fb

  58. DrahtBot removed review request from Ayush170-Future on Nov 23, 2023
  59. DrahtBot requested review from Ayush170-Future on Nov 23, 2023
  60. fanquake merged this on Nov 23, 2023
  61. fanquake closed this on Nov 23, 2023
  62. 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.

  63. fanquake commented at 3:16 PM on November 25, 2023: member

    Null-dereference READ · wallet::scriptpubkeyman_fuzz_target

    Command: /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds-honggfuzz_bitcoin-core_83aef6625aaeafa301867de74608b320f3c923fe/revisions/scriptpubkeyman
	Time ran: 0.5496230125427246
	
	Accepting input from '[STDIN]'
	Usage for fuzzing: honggfuzz -P [flags] -- /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds-honggfuzz_bitcoin-core_83aef6625aaeafa301867de74608b320f3c923fe/revisions/scriptpubkeyman
	AddressSanitizer:DEADLYSIGNAL
	=================================================================
	==14133==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x79a1f8670aeb bp 0x7ffd95472db0 sp 0x7ffd95472578 T0)
	==14133==The signal is caused by a READ memory access.
	==14133==Hint: address points to the zero page.
	SCARINESS: 10 (null-deref)
	    [#0](/bitcoin-bitcoin/0/) 0x79a1f8670aeb in memcpy /build/glibc-SzIz7B/glibc-2.31/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:142
	    [#1](/bitcoin-bitcoin/1/) 0x589ab4429a18 in __asan_memcpy /src/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
	    [#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)
	    [#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
	    [#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
	    [#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
	    [#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
	    [#7](/bitcoin-bitcoin/7/) 0x589ab446d2d1 in operator() /usr/local/include/c++/v1/__functional/function.h:181:16
	    [#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
	    [#9](/bitcoin-bitcoin/9/) 0x589ab49ef0b8 in operator() /usr/local/include/c++/v1/__functional/function.h:508:16
	    [#10](/bitcoin-bitcoin/10/) 0x589ab49ef0b8 in operator() /usr/local/include/c++/v1/__functional/function.h:1185:12
	    [#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
	    [#12](/bitcoin-bitcoin/12/) 0x589ab642bdeb in main
	    [#13](/bitcoin-bitcoin/13/) 0x79a1f85d9082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
	    [#14](/bitcoin-bitcoin/14/) 0x589ab43a93bd in _start
  64. maflcko commented at 3:01 PM on November 29, 2023: member

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

  65. fanquake referenced this in commit d00d50e78a on Nov 29, 2023
  66. hebasto referenced this in commit 6da3579d34 on Mar 29, 2024
  67. hebasto referenced this in commit 43f5de0612 on Mar 29, 2024
  68. hebasto referenced this in commit 9397478cd5 on Apr 2, 2024
  69. hebasto referenced this in commit b2c1f64a1e on Apr 2, 2024
  70. bitcoin locked this on Nov 28, 2024
  71. brunoerg deleted the branch on Dec 29, 2025
Contributors
brunoergDrahtBotmaflckodergoeggeAyush170-Futurefanquake


Ayush170-Future

Labels
Tests
Linked (view graph)
#28968 fuzz: Fix nullptr deref in scriptpubkeyman
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me