fuzz: wallet: fix crypter target

brunoerg commented at 9:37 pm on March 21, 2025: contributor

The crypter target has an issue, it’s calling DecryptKey with a random secret and a random public key that will unlikely be related to the key used to encrypt, so it won’t have any effect. This PR changes fixes it and also removes the DecryptSecret call since this function is already (and only) called within DecryptKey.

fuzz: wallet: fix crypter target 28dc118001

DrahtBot commented at 9:38 pm on March 21, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32118.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

DrahtBot added the label Tests on Mar 21, 2025

in src/wallet/test/fuzz/crypter.cpp:23 in 28dc118001

19@@ -20,6 +20,7 @@ void initialize_crypter()
20 
21 FUZZ_TARGET(crypter, .init = initialize_crypter)
22 {
23+    SeedRandomStateForTest(SeedRand::ZEROS);

maflcko commented at 8:07 am on March 22, 2025:

Why is this needed?

brunoerg commented at 9:48 pm on March 22, 2025:

Without it I get:

 0[#47708](/bitcoin-bitcoin/47708/)	REDUCE cov: 475 ft: 1809 corp: 217/12024b lim: 98 exec/s: 3669 rss: 79Mb L: 77/98 MS: 2 PersAutoDict-EraseBytes- DE: "\034\000"-
 1
 2
 3The current fuzz target used the global random state.
 4
 5This is acceptable, but requires the fuzz target to call
 6SeedRandomStateForTest(SeedRand::ZEROS) in the first line
 7of the FUZZ_TARGET function.
 8
 9An alternative solution would be to avoid any use of globals.
10
11Without a solution, fuzz instability and non-determinism can lead
12to non-reproducible bugs or inefficient fuzzing.
13
14
15==37661== ERROR: libFuzzer: deadly signal
16    [#0](/bitcoin-bitcoin/0/) 0x0001071d358c in __sanitizer_print_stack_trace+0x14 (libclang_rt.ubsan_osx_dynamic.dylib:arm64+0xb58c)
17    [#1](/bitcoin-bitcoin/1/) 0x0001059d8910 in fuzzer::PrintStackTrace() FuzzerUtil.cpp:210
18    [#2](/bitcoin-bitcoin/2/) 0x0001059beba8 in fuzzer::Fuzzer::CrashCallback() FuzzerLoop.cpp:231
19    [#3](/bitcoin-bitcoin/3/) 0x000183ee1a20 in _sigtramp+0x34 (libsystem_platform.dylib:arm64+0x3a20)
20    [#4](/bitcoin-bitcoin/4/) 0xc1f800183eb1cbc  (<unknown module>)
21    [#5](/bitcoin-bitcoin/5/) 0xbf78000183dbda3c  (<unknown module>)
22    [#6](/bitcoin-bitcoin/6/) 0xcd75800104c013bc  (<unknown module>)
23    [#7](/bitcoin-bitcoin/7/) 0x000104c011fc in CheckGlobals::~CheckGlobals() check_globals.cpp:57
24    [#8](/bitcoin-bitcoin/8/) 0x000104c094b0 in LLVMFuzzerTestOneInput fuzz.cpp:205
25    [#9](/bitcoin-bitcoin/9/) 0x0001059c0154 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:614
26    [#10](/bitcoin-bitcoin/10/) 0x0001059bf9d4 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) FuzzerLoop.cpp:516
27    [#11](/bitcoin-bitcoin/11/) 0x0001059c0fe0 in fuzzer::Fuzzer::MutateAndTestOne() FuzzerLoop.cpp:760
28    [#12](/bitcoin-bitcoin/12/) 0x0001059c1c40 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&) FuzzerLoop.cpp:905
29    [#13](/bitcoin-bitcoin/13/) 0x0001059b234c in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:915
30    [#14](/bitcoin-bitcoin/14/) 0x0001059d9314 in main FuzzerMain.cpp:20
31    [#15](/bitcoin-bitcoin/15/) 0x000183b310dc  (<unknown module>)
32    [#16](/bitcoin-bitcoin/16/) 0x71087ffffffffffc  (<unknown module>)
33
34NOTE: libFuzzer has rudimentary signal handlers.
35      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
36SUMMARY: libFuzzer: deadly signal
37MS: 2 InsertByte-CopyPart-; base unit: a4f5fd02ccbbc73b36cc7b134ec52d21b18f25f0
380x0,0x0,0x94,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0xff,0xff,0xff,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0x5f,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0x5f,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xd9,0xdf,0xdf,0xdf,0xdf,0x34,
39\000\000\224\000\000\000\000\000\000\000\000\377\377\377\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337_\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337_\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\331\337\337\337\3374
40artifact_prefix='./'; Test unit written to ./crash-d072ccb4a7016c772755ac6021155420b6b0c3f7
41Base64: AACUAAAAAAAAAAD////f39/f39/f39/f39/f39/f39/f399f39/f39/f39/f39/f39/f39/f39/f39/f39/f39/f39/f39/f399f39/f39/f39/f39/f39/f39/Z39/f3zQ=

maflcko commented at 8:08 am on March 22, 2025: member

Would be nice to add a short line on how to test/observe this fix

yancyribbens commented at 6:22 pm on March 23, 2025: contributor

nit: the commit message is pretty sparse. Could add a “why” section to the commit.

brunoerg commented at 1:27 pm on March 24, 2025: contributor

Would be nice to add a short line on how to test/observe this fix

I would say that simply checking the coverage would be enough. Note that https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/crypter.cpp.gcov.html L143 and L144 are unreachable and probably never be reachable with the current harness.

fuzz: wallet: fix crypter target #32118

Code Coverage & Benchmarks

Reviews