fuzz: add target for `DescriptorScriptPubKeyMan` #28578

brunoerg · 2023-10-04T01:03:23Z

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

DrahtBot · 2023-10-04T01:03:26Z

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.

maflcko

concept ack

src/wallet/test/fuzz/scriptpubkeyman.cpp

dergoegge · 2023-10-10T14:42:28Z

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

brunoerg · 2023-10-10T18:46:24Z

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

dergoegge · 2023-10-11T08:45:16Z

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

dergoegge · 2023-10-12T09:09:21Z

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

brunoerg · 2023-10-12T20:50:24Z

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.

Ayush170-Future

ACK

Reviewed the whole code. Looks great to me!

dergoegge · 2023-11-06T09:49:31Z

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 · 2023-11-06T21:19:24Z

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 · 2023-11-08T14:05:59Z

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

src/wallet/test/fuzz/scriptpubkeyman.cpp

brunoerg · 2023-11-10T13:43:10Z

Force-pushed addressing #28578 (comment)

src/wallet/test/fuzz/scriptpubkeyman.cpp

src/test/fuzz/util/descriptor.h

src/wallet/test/fuzz/scriptpubkeyman.cpp

maflcko

lgtm ACK 7774df9

brunoerg · 2023-11-20T19:03:24Z

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

maflcko · 2023-11-21T07:46:50Z

lgtm ACK 47e5c99 🏓

Show signature

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==

dergoegge

ACK 47e5c99

maflcko · 2023-11-24T11:22:04Z

src/wallet/test/fuzz/scriptpubkeyman.cpp

+                    bool extract_dest{ExtractDestination(spk, dest)};
+                    if (extract_dest) {
+                        const std::string msg{fuzzed_data_provider.ConsumeRandomLengthString()};
+                        PKHash pk_hash{fuzzed_data_provider.ConsumeBool() ? PKHash{ConsumeUInt160(fuzzed_data_provider)} : *std::get_if<PKHash>(&dest)};


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

fanquake · 2023-11-25T15:16:32Z

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 0x79a1f8670aeb in memcpy /build/glibc-SzIz7B/glibc-2.31/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:142
	    #1 0x589ab4429a18 in __asan_memcpy /src/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
	    #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 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 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 0x589ab446d2d1 in __invoke<void (*&)(Span<const unsigned char>), Span<const unsigned char> > /usr/local/include/c++/v1/type_traits:3592:23
	    #6 0x589ab446d2d1 in __call<void (*&)(Span<const unsigned char>), Span<const unsigned char> > /usr/local/include/c++/v1/__functional/invoke.h:61:9
	    #7 0x589ab446d2d1 in operator() /usr/local/include/c++/v1/__functional/function.h:181:16
	    #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 0x589ab49ef0b8 in operator() /usr/local/include/c++/v1/__functional/function.h:508:16
	    #10 0x589ab49ef0b8 in operator() /usr/local/include/c++/v1/__functional/function.h:1185:12
	    #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 0x589ab642bdeb in main
	    #13 0x79a1f85d9082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
	    #14 0x589ab43a93bd in _start

maflcko · 2023-11-29T15:01:45Z

Fixed the crash in #28968

faecde9 fuzz: Fix nullptr deref in scriptpubkeyman (MarcoFalke) Pull request description: This should fix the UB that was found by review (#28578 (comment)) and by fuzzing (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64487) ACKs for top commit: dergoegge: utACK faecde9 brunoerg: crACK faecde9 Tree-SHA512: ff726ed632d8d369c96d316bafebe87ff385e47b74b1d1da79409ddf296559eb991431883858057527e5df2414c01812ecbc99c21c69020228b0747f32b03121

Add a missed source file. See: bitcoin#28578

Add the missing source file. See: bitcoin#28578

List of the squashed commits: ============================= cmake: Add root `CMakeLists.txt` file cmake: Introduce core interface libraries to encapsulate common flags Also add a sanity check for non-encapsulated (directory-wide) build properties. cmake: Add `config/bitcoin-config.h` support cmake: Check system headers cmake: Check system symbols Co-authored-by: Cory Fields <cory-nospam-@coryfields.com> Co-authored-by: Vasil Dimov <vd@FreeBSD.org> cmake: Check compiler features cmake: Add position independent code support cmake: Add platform-specific definitions and properties cmake: Build `crc32c` static library cmake: Build `leveldb` static library Co-authored-by: Cory Fields <cory-nospam-@coryfields.com> cmake: Build `minisketch` static library cmake: Build `secp256k1` static library cmake: Build `univalue` static library cmake: Build `bitcoin_crypto` library cmake: Build `bitcoin_util` static library cmake: Build `bitcoin_consensus` library cmake: Build `bitcoind` executable depends: Amend handling flags environment variables If any of {C,CXX,CPP,LD}FLAGS is specified it should be assigned to a non-type-specific variable. build: Generate `share/toolchain.cmake` in depends cmake: Add cross-compiling support To configure CMake for cross-compiling, use `--toolchain depends/${HOST}/share/toolchain.cmake` command-line option. cmake: Add `TristateOption` module cmake: Add `ccache` support cmake: Add `libnatpmp` optional package support cmake: Add `libminiupnpc` optional package support cmake: Add `libzmq` optional package support cmake: Add `systemtap-sdt` optional package support cmake: Build `bitcoin-cli` executable cmake: Build `bitcoin-tx` executable cmake: Build `bitcoin-util` executable cmake: Add wallet functionality cmake: Add test config and runners cmake: Build `bench_bitcoin` executable cmake: Build `test_bitcoin` executable cmake: Include CTest cmake: Add `TryAppendCXXFlags` module cmake: Add `TryAppendLinkerFlag` module cmake: Add platform-specific compiler flags cmake: Add platform-specific linker flags cmake: Redefine/adjust per-configuration flags cmake: Add general compile options cmake: Add `HARDENING` option cmake: Add `REDUCE_EXPORTS` option cmake: Add `WERROR` option cmake: Implement `make install` cmake: Generate `obj/build.h` header cmake: Add `GenerateBuildInfo.cmake` script Revert "build, qt: Do not install *.prl files" This reverts commit 1155978. qt, build: Drop `QT_STATICPLUGIN` macro Our `QT_STATICPLUGIN` macro is effectively equivalent to the Qt's `QT_STATIC` macro. No need to handle both of them. cmake: Build `bitcoin-qt` executable cmake: Build `test_bitcoin-qt` executable qt: Drop `Q_IMPORT_PLUGIN` macros When using CMake, each plugin comes with a C++ stub file that automatically initializes the static plugin. Consequently, any target that links against a plugin has this C++ file added to its SOURCES, which makes the removed code redundant. cmake: Add `libqrencode` optional package support cmake: Add `SANITIZERS` option cmake: Add external signer support cmake: Add fuzzing options cmake: Add `AddWindowsResources` module cmake: Add `Maintenance` module cmake: Migrate Guix build scripts to CMake cmake: Add vcpkg manifest file cmake: Add preset for MSVC build Fix MSVC warning C4273 "inconsistent dll linkage" cmake, doc: Update `release-process.md` Only versioning has been updated for now. cmake: Add compiler diagnostic flags test: Fix MSVC warning C4101 "unreferenced local variable" ci: Test CMake edge cases Keep this commit at the top when rebasing. Add macOS cross compile jobs. Don't pass `-fno-extended-identifiers` to a linker Process linker flags properly. refactor! cmake: Redefine/adjust per-configuration flags Process linker flags properly. Add `deploy` target for macOS Add the missing source file. See: bitcoin#28578 Backport bitcoin#27375. Align with bitcoin#27699.

DrahtBot added the Tests label Oct 4, 2023

maflcko approved these changes Oct 4, 2023

View reviewed changes

src/wallet/test/fuzz/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

src/wallet/test/fuzz/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

brunoerg force-pushed the 2023-07-fuzz-scriptpubkey-descriptor branch from 6aaf739 to 9eb8913 Compare October 4, 2023 12:03

DrahtBot added the CI failed label Oct 4, 2023

brunoerg force-pushed the 2023-07-fuzz-scriptpubkey-descriptor branch from 9eb8913 to 5e78c17 Compare October 4, 2023 21:27

DrahtBot removed the CI failed label Oct 5, 2023

maflcko reviewed Oct 5, 2023

View reviewed changes

src/wallet/test/fuzz/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

brunoerg force-pushed the 2023-07-fuzz-scriptpubkey-descriptor branch from 5e78c17 to 9171e05 Compare October 5, 2023 11:45

brunoerg force-pushed the 2023-07-fuzz-scriptpubkey-descriptor branch from 9171e05 to 538dea8 Compare October 10, 2023 18:45

brunoerg force-pushed the 2023-07-fuzz-scriptpubkey-descriptor branch from 538dea8 to 9648539 Compare October 11, 2023 15:35

brunoerg force-pushed the 2023-07-fuzz-scriptpubkey-descriptor branch from 9648539 to 2f31c32 Compare October 12, 2023 20:45

DrahtBot added CI failed and removed CI failed labels Oct 18, 2023

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

Ayush170-Future approved these changes Nov 4, 2023

View reviewed changes

brunoerg force-pushed the 2023-07-fuzz-scriptpubkey-descriptor branch from 2f31c32 to 43f7fc6 Compare November 8, 2023 14:04

dergoegge reviewed Nov 10, 2023

View reviewed changes

src/wallet/test/fuzz/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

brunoerg force-pushed the 2023-07-fuzz-scriptpubkey-descriptor branch from 43f7fc6 to ca5f6ac Compare November 10, 2023 13:41

dergoegge reviewed Nov 13, 2023

View reviewed changes

src/wallet/test/fuzz/scriptpubkeyman.cpp Show resolved Hide resolved

brunoerg marked this pull request as draft November 13, 2023 11:56

brunoerg marked this pull request as ready for review November 20, 2023 15:02

maflcko reviewed Nov 20, 2023

View reviewed changes

src/test/fuzz/util/descriptor.h Outdated Show resolved Hide resolved

src/wallet/test/fuzz/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

src/wallet/test/fuzz/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

maflcko reviewed Nov 20, 2023

View reviewed changes

DrahtBot requested a review from Ayush170-Future November 20, 2023 15:33

DrahtBot removed the CI failed label Nov 20, 2023

brunoerg added 3 commits November 20, 2023 15:57

fuzz: move MockedDescriptorConverter to fuzz/util

2e1833c

fuzz: create ConsumeCoins

641dddf

fuzz: add target for DescriptorScriptPubKeyMan

47e5c99

brunoerg force-pushed the 2023-07-fuzz-scriptpubkey-descriptor branch from 7774df9 to 47e5c99 Compare November 20, 2023 19:02

DrahtBot requested review from Ayush170-Future and removed request for Ayush170-Future November 21, 2023 07:46

fanquake requested a review from dergoegge November 22, 2023 12:13

dergoegge approved these changes Nov 23, 2023

View reviewed changes

DrahtBot requested review from Ayush170-Future and removed request for Ayush170-Future November 23, 2023 12:50

fanquake merged commit f4073c5 into bitcoin:master Nov 23, 2023

maflcko reviewed Nov 24, 2023

View reviewed changes

maflcko mentioned this pull request Nov 29, 2023

Increase fuzz coverage in the wallet #27272

Closed

maflcko mentioned this pull request Nov 29, 2023

fuzz: Fix nullptr deref in scriptpubkeyman #28968

Merged

hebasto added a commit to hebasto/bitcoin that referenced this pull request Mar 29, 2024

fixup! cmake: Add fuzzing options

6da3579

Add a missed source file. See: bitcoin#28578

hebasto added a commit to hebasto/bitcoin that referenced this pull request Mar 29, 2024

fixup! cmake: Add fuzzing options

43f5de0

Add the missing source file. See: bitcoin#28578

pablomartin4btc mentioned this pull request Mar 30, 2024

cmake [FIXUP]: Add the missing source file hebasto/bitcoin#131

Merged

bitcoin locked and limited conversation to collaborators Nov 28, 2024

fuzz: add target for DescriptorScriptPubKeyMan #28578

fuzz: add target for DescriptorScriptPubKeyMan #28578

Uh oh!

Conversation

brunoerg commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dergoegge commented Oct 10, 2023

Uh oh!

brunoerg commented Oct 10, 2023

Uh oh!

dergoegge commented Oct 11, 2023

Uh oh!

dergoegge commented Oct 12, 2023

Uh oh!

brunoerg commented Oct 12, 2023

Uh oh!

Ayush170-Future left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge commented Nov 6, 2023

Uh oh!

brunoerg commented Nov 6, 2023

Uh oh!

brunoerg commented Nov 8, 2023

Uh oh!

Uh oh!

brunoerg commented Nov 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Nov 20, 2023

Uh oh!

maflcko commented Nov 21, 2023

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake commented Nov 25, 2023

Uh oh!

maflcko commented Nov 29, 2023

Uh oh!

Uh oh!

fuzz: add target for `DescriptorScriptPubKeyMan` #28578

fuzz: add target for `DescriptorScriptPubKeyMan` #28578

brunoerg commented Oct 4, 2023 •

edited

Loading

DrahtBot commented Oct 4, 2023 •

edited

Loading