fuzz: wallet, add target for `Crypter`

Ayush170-Future commented at 1:43 pm on July 13, 2023: contributor

This PR adds fuzz coverage for wallet/crypter.

Motivation: Issue 27272

I ran this for a long time with Sanitizers on and had no crashes; the average exec/sec also looks good to me. However, I would really appreciate it if some of the reviewers could try it on their machines too, and give their feedback.

DrahtBot commented at 1:43 pm on July 13, 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, brunoerg, achow101

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

DrahtBot added the label Tests on Jul 13, 2023

Ayush170-Future commented at 1:49 pm on July 13, 2023: contributor

In addition to this and CoinControl, I’m working on the FeeBumper and Spend files. The majority of my work in FeeBumper is finished, and I’ll open a PR for it as soon as we reach a conclusion on this PR. I’m currently working on the Spend file, which should be finished this week or next, hopefully.

in src/wallet/test/fuzz/crypter.cpp:42 in 6c98f4c137 outdated

37+
38+                // Limiting the value of nRounds since it is otherwise uselessly expensive and causes a timeout when fuzzing.
39+                crypt.SetKeyFromPassphrase(/*strKeyData=*/secure_string,
40+                                           /*chSalt=*/ConsumeRandomLengthByteVector(fuzzed_data_provider),
41+                                           /*nRounds=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, 25000),
42+                                           /*nDerivationMethod*/ nDerivationMethod);

maflcko commented at 2:11 pm on July 13, 2023:

0                                           /*nDerivationMethod=*/ nDerivationMethod);

Ayush170-Future commented at 3:59 pm on July 13, 2023:

Thanks! Updated.

maflcko approved

brunoerg approved

brunoerg commented at 2:26 pm on July 13, 2023: contributor

ACK 6c98f4c137c9d557c78ebd52379711ebbd23e24a

non blocker, just an idea: Perhaps we could have encrypt/decrypt stuff outside of LIMITED_WHILE and then we could have a call just to fill them. I suppose this way we would have more chance of decrypting a previous encrypted thing. Just an example:

 0diff --git a/src/wallet/test/fuzz/crypter.cpp b/src/wallet/test/fuzz/crypter.cpp
 1index 7ba43821b..03ad97a35 100644
 2--- a/src/wallet/test/fuzz/crypter.cpp
 3+++ b/src/wallet/test/fuzz/crypter.cpp
 4@@ -24,6 +24,9 @@ FUZZ_TARGET_INIT(crypter, initialize_crypter)
 5     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
 6 
 7     CCrypter crypt;
 8+    std::vector<unsigned char> cipher_text_ed;
 9+    CKeyingMaterial plain_text_ed;
10+
11 
12     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000)
13     {
14@@ -49,14 +52,16 @@ FUZZ_TARGET_INIT(crypter, initialize_crypter)
15             },
16             [&] {
17                 const std::vector<unsigned char> random_vector = ConsumeRandomLengthByteVector(fuzzed_data_provider);
18-                const CKeyingMaterial plain_text(random_vector.begin(), random_vector.end());
19-                std::vector<unsigned char> cipher_text;
20-                crypt.Encrypt(plain_text, cipher_text);
21+                plain_text_ed = CKeyingMaterial(random_vector.begin(), random_vector.end());
22             },
23             [&] {
24-                CKeyingMaterial plain_text;
25-                const std::vector<unsigned char> cipher_text = ConsumeRandomLengthByteVector(fuzzed_data_provider);
26-                crypt.Decrypt(cipher_text, plain_text);
27+                std::vector<unsigned char> cipher_text_ed = ConsumeRandomLengthByteVector(fuzzed_data_provider);
28+            },
29+            [&] {
30+                crypt.Encrypt(plain_text_ed, cipher_text_ed);
31+            },
32+            [&] {
33+                crypt.Decrypt(cipher_text_ed, plain_text_ed);
34             },
35             [&] {

Ayush170-Future force-pushed on Jul 13, 2023

Ayush170-Future commented at 4:21 pm on July 13, 2023: contributor

Thanks, @MarcoFalke and @brunoerg for the reviews.

Moved Encrypt/Decrypt variables outside of LIMITED_WHILE and added two functions to update them regularly.

DrahtBot added the label CI failed on Jul 13, 2023

Ayush170-Future force-pushed on Jul 13, 2023

DrahtBot removed the label CI failed on Jul 13, 2023

Ayush170-Future requested review from brunoerg on Jul 14, 2023

brunoerg approved

brunoerg commented at 2:10 pm on July 14, 2023: contributor

reACK e010fddf15a6d4ff84bfebdcb33c9c1d9cea7f0a

maflcko approved

in src/wallet/test/fuzz/crypter.cpp:22 in e010fddf15 outdated

17+{
18+    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
19+    g_setup = testing_setup.get();
20+}
21+
22+FUZZ_TARGET_INIT(crypter, initialize_crypter)

maflcko commented at 12:52 pm on July 17, 2023:

Needs rebase

Ayush170-Future commented at 5:24 pm on July 17, 2023:

Thanks, done!

Ayush170-Future force-pushed on Jul 17, 2023

Ayush170-Future commented at 5:25 pm on July 17, 2023: contributor

Forced pushed updating changes in PR 28065.

brunoerg approved

brunoerg commented at 5:40 pm on July 17, 2023: contributor

reACK 7570e2228e22c3b22e119e159c6fa07c4c891d98

bitcoin deleted a comment on Jul 22, 2023

in src/wallet/test/fuzz/crypter.cpp:58 in 7570e2228e outdated

53+            [&] {
54+                const std::vector<unsigned char> random_vector = ConsumeRandomLengthByteVector(fuzzed_data_provider);
55+                plain_text_ed = CKeyingMaterial(random_vector.begin(), random_vector.end());
56+            },
57+            [&] {
58+                std::vector<unsigned char> cipher_text_ed = ConsumeRandomLengthByteVector(fuzzed_data_provider);

maflcko commented at 9:08 am on July 22, 2023:

That’s a no-op assignment due to the symbol shadow.

Ayush170-Future commented at 9:25 am on July 22, 2023:

Thank you so much for noticing this. I don’t know how I missed this.

Ayush170-Future force-pushed on Jul 22, 2023

Ayush170-Future commented at 9:27 am on July 22, 2023: contributor

Forced pushed fixing the Shadow variable issue.

in src/wallet/test/fuzz/crypter.cpp:39 in fe1a27153e outdated

34+            fuzzed_data_provider,
35+            [&] {
36+                const std::string random_string = fuzzed_data_provider.ConsumeRandomLengthString();
37+                SecureString secure_string(random_string.begin(), random_string.end());
38+
39+                const unsigned int nDerivationMethod = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<unsigned int>();

dergoegge commented at 1:04 pm on August 3, 2023:

style nit, see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

0                const unsigned int derivation_method = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<unsigned int>();

in src/wallet/test/fuzz/crypter.cpp:45 in fe1a27153e outdated

40+
41+                // Limiting the value of nRounds since it is otherwise uselessly expensive and causes a timeout when fuzzing.
42+                crypt.SetKeyFromPassphrase(/*strKeyData=*/secure_string,
43+                                           /*chSalt=*/ConsumeRandomLengthByteVector(fuzzed_data_provider),
44+                                           /*nRounds=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, 25000),
45+                                           /*nDerivationMethod=*/ nDerivationMethod);

dergoegge commented at 1:09 pm on August 3, 2023:

style nit:

0                                           /*nDerivationMethod=*/nDerivationMethod);

in src/wallet/test/fuzz/crypter.cpp:21 in fe1a27153e outdated

17+{
18+    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
19+    g_setup = testing_setup.get();
20+}
21+
22+FUZZ_TARGET(crypter, .init = initialize_crypter)

dergoegge commented at 1:27 pm on August 3, 2023:

I don’t quite understand why the CI isn’t failing here. You’re using the new macro from #28065 but you haven’t rebased past that PR, so this shouldn’t work.

These are the errors that i get locally:

 0wallet/test/fuzz/crypter.cpp:22:22: error: too many arguments provided to function-like macro invocation
 1FUZZ_TARGET(crypter, .init = initialize_crypter)
 2                     ^
 3./test/fuzz/fuzz.h:31:9: note: macro 'FUZZ_TARGET' defined here
 4#define FUZZ_TARGET(name) \
 5        ^
 6wallet/test/fuzz/crypter.cpp:22:1: error: a type specifier is required for all declarations
 7FUZZ_TARGET(crypter, .init = initialize_crypter)
 8^
 9wallet/test/fuzz/crypter.cpp:24:5: error: 'FuzzedDataProvider' does not refer to a value
10    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
11    ^
12./test/fuzz/FuzzedDataProvider.h:31:7: note: declared here
13class FuzzedDataProvider {
14      ^
15wallet/test/fuzz/crypter.cpp:89:2: error: expected ';' after top level declarator
16}
17 ^
18 ;
194 errors generated.

maflcko commented at 1:31 pm on August 3, 2023:

CI does a “rebase” before each run. Generally this detects more issues (silent merge conflicts), and I think this is the first case that CI missed a compile failure of this kind. (Generally it is assumed that developers at a minimum try to compile their changes locally, so it would be odd to have a dedicated CI check for that, but maybe there should be?)

maflcko commented at 1:32 pm on August 3, 2023:

Now that we are moving to persistent workers, we can even compile all commits in a pull request. Maybe as a separate task on a runner where it doesn’t matter when the CI run takes a day or two?

dergoegge commented at 1:35 pm on August 3, 2023:

Now that we are moving to persistent workers, we can even compile all commits in a pull request.

Sounds good but would that really take one or two days? With caching this should only take marginally longer, no?

maflcko commented at 1:43 pm on August 3, 2023:

I am not sure how much CPU we want to spend on this. It should only ever find an issue once every couple of months. And there could be quite a backlog easily. For example, 3 devs could each force push a 15 commit pull that touches serialize.h at the same time. So the cache would be useless and the CI would have to do 45 builds from scratch. (Assuming each takes half an hour, that’d be a day of backlog)

maflcko commented at 5:46 pm on September 20, 2023:

In any case, should be trivial to implement by removing the if: github.event.pull_request.commits != 1 and git checkout HEAD~ from #28497 (comment)

Ayush170-Future commented at 2:20 pm on November 10, 2023:

This is out of the blue, but the reason I struggle to compile after each change is that I’m currently using a Windows 10 OS. And I can’t run Wallet Fuzz tests on my machine for some unknown reason. I’ve attempted Fuzz tests outside of Wallet code, and they run fine. I’ve also experimented with other Windows machines, and this issue seems to persists across all of them.

So, I’ve been using an Ubuntu VM for code testing. I’ve gone through Makefile.test.include, and everything appears to be in order. I’m uncertain about the reason of this problem here.

maflcko commented at 2:23 pm on November 10, 2023:

Seems fine to report an issue about this, with exact steps to reproduce.

Ayush170-Future commented at 5:21 pm on November 13, 2023:

Yes, I’m on it. Will try to reproduce it once again and create an issue then.

dergoegge changes_requested

Ayush170-Future force-pushed on Aug 3, 2023

Ayush170-Future commented at 6:20 pm on August 3, 2023: contributor

Thank you for pointing this up, @dergoegge. Actually, I thought it was just a one-line change and became overly relaxed about it. But I’m sorry, and from now on I’ll compile before pushing every time.

I’m also going to address your style nits in the next push.

Ayush170-Future force-pushed on Aug 3, 2023

achow101 requested review from brunoerg on Sep 20, 2023

achow101 requested review from achow101 on Sep 20, 2023

in src/wallet/test/fuzz/crypter.cpp:10 in 066402c5c4 outdated

 5+#include <test/fuzz/FuzzedDataProvider.h>
 6+#include <test/fuzz/fuzz.h>
 7+#include <test/fuzz/util.h>
 8+#include <test/util/setup_common.h>
 9+#include <wallet/crypter.h>
10+#include <wallet/test/util.h>

maflcko commented at 5:30 pm on September 20, 2023:

Is this needed?

From iwyu:

 0wallet/test/fuzz/crypter.cpp should add these lines:
 1#include <memory>                          // for unique_ptr
 2#include <optional>                        // for optional
 3#include <string>                          // for string
 4#include <vector>                          // for vector
 5#include "key.h"                           // for CKey
 6#include "pubkey.h"                        // for CPubKey
 7#include "support/allocators/secure.h"     // for secure_allocator, SecureString
 8#include "uint256.h"                       // for uint256
 9
10wallet/test/fuzz/crypter.cpp should remove these lines:
11- #include <wallet/test/util.h>  // lines 10-10

Ayush170-Future commented at 8:12 pm on October 7, 2023:

No, it is not needed. Removing this, thanks!

in src/wallet/test/fuzz/crypter.cpp:61 in 066402c5c4 outdated

56+            },
57+            [&] {
58+                cipher_text_ed = ConsumeRandomLengthByteVector(fuzzed_data_provider);
59+            },
60+            [&] {
61+                crypt.Encrypt(plain_text_ed, cipher_text_ed);

brunoerg commented at 12:18 pm on October 2, 2023:

If you’re not using the returned value, you could do:

0                (void)crypt.Encrypt(plain_text_ed, cipher_text_ed);

However, I think you could check if Encrypt returned true then you could try to decrypt it.

Ayush170-Future commented at 7:45 pm on October 2, 2023:

Yeah, I think that is better. I’ll push these changes before the weekend (have college exams throughout this week, apologies for the delay). Thanks!

Ayush170-Future commented at 8:01 pm on October 7, 2023:

However, I think you could check if Encrypt returned true then you could try to decrypt it.

I believe it will attempt to Decrypt it, perhaps not instantly but eventually, so I suppose for a long Fuzz run it won’t matter much. Should I simply (void) it?

Ayush170-Future force-pushed on Oct 21, 2023

Ayush170-Future commented at 10:57 pm on October 21, 2023: contributor

Rebased.
Removed the unnecessary <wallet/test/util.h> import.
Added (void) before the Encrypt function call.

DrahtBot added the label CI failed on Oct 22, 2023

Ayush170-Future force-pushed on Oct 23, 2023

Ayush170-Future commented at 6:49 pm on October 23, 2023: contributor

I think check failures are coming after I rebased the code. Because my changes are very basic to produce these fails.

lint failure is due to spelling mistakes in other files.
I’m not sure why the fuzzer is failing, it seems unrelated to my file.

In the meantime, I can pull up a PR correcting all the spelling mistakes in the lint. Or I can correct those in this PR only.

maflcko commented at 8:03 am on October 24, 2023: member

The lint task output seems confusing and fragile, but the error here is:

0This diff appears to have added new lines with trailing whitespace.
1The following changes were suspected:
2
3diff --git a/src/wallet/test/fuzz/crypter.cpp b/src/wallet/test/fuzz/crypter.cpp
4@@ -0,0 +1,90 @@
5+    // These values are regularly updated within `CallOneOf`

I guess in the future it could make sense to clarify that the spelling output is only generated for people that care about fixing spelling mistakes, and can be ignored by everyone else.

Ayush170-Future force-pushed on Oct 24, 2023

Ayush170-Future commented at 9:05 am on October 24, 2023: contributor

Force pushed fixing the lint issue.
I’m currently looking into why the Fuzzer is failing. Will try to fix it ASAP.

maflcko commented at 9:15 am on October 24, 2023: member

The fuzz timeout can be ignored for now and will be fixed this week.

DrahtBot removed the label CI failed on Oct 25, 2023

in src/wallet/test/fuzz/crypter.cpp:81 in 2a3ffe50db outdated

76+            },
77+            [&] {
78+                std::optional<CPubKey> random_pub_key = ConsumeDeserializable<CPubKey>(fuzzed_data_provider);
79+                if (!random_pub_key) return;
80+                const CPubKey pub_key = *random_pub_key;
81+                const std::vector<unsigned char> random_key = ConsumeRandomLengthByteVector(fuzzed_data_provider);

maflcko commented at 10:55 am on December 6, 2023:

Could also move them out of the loop, like the others?

Ayush170-Future commented at 11:12 pm on January 5, 2024:

Did it. I was thinking of moving master_key outside as well but since it is const it wouldn’t make sense. Should I add a function in CallOneOf that updates the random_key occasionally as well?

maflcko approved

maflcko commented at 10:56 am on December 6, 2023: member

lgtm

Ayush170-Future force-pushed on Jan 5, 2024

DrahtBot added the label CI failed on Jan 13, 2024

achow101 requested review from dergoegge on Apr 9, 2024

achow101 requested review from brunoerg on Apr 9, 2024

Ayush170-Future commented at 5:28 am on April 18, 2024: contributor

Can someone tell me why is the previous releases, qt5 dev package and depends packages, DEBUG test is failing?

I’ve ran this on my machine and it works fine.

maflcko commented at 5:30 am on April 18, 2024: member

It is using an old CI config. You can rebase on current master to get a fresh CI run.

in src/wallet/test/fuzz/crypter.cpp:64 in 87dfbe9f56 outdated

59+            },
60+            [&] {
61+                (void)crypt.Encrypt(plain_text_ed, cipher_text_ed);
62+            },
63+            [&] {
64+                crypt.Decrypt(cipher_text_ed, plain_text_ed);

brunoerg commented at 1:56 pm on April 18, 2024:

0                (void)crypt.Decrypt(cipher_text_ed, plain_text_ed);

in src/wallet/test/fuzz/crypter.cpp:78 in 87dfbe9f56 outdated

72+                const CKeyingMaterial master_key(random_key.begin(), random_key.end());
73+                const uint256 iv = ConsumeUInt256(fuzzed_data_provider);
74+                DecryptSecret(master_key, cipher_text_ed, iv, plain_text_ed);
75+            },
76+            [&] {
77+                std::optional<CPubKey> random_pub_key = ConsumeDeserializable<CPubKey>(fuzzed_data_provider);

brunoerg commented at 1:59 pm on April 18, 2024:

Since you’re using ConsumeDeserializable, I do believe you should adopt the approach introduced in “fuzz: Avoid timeout and bloat in fuzz targets” fabb5046a7365af3079e6e45606d63576bc6ad12

Ayush170-Future commented at 11:35 pm on April 30, 2024:

Did the necessary changes. Thanks!

brunoerg commented at 9:34 am on April 29, 2024: contributor

Up for grabs?

Ayush170-Future commented at 7:50 pm on April 29, 2024: contributor

Up for grabs?

Updating in a day or too. Just got over with my final year college exams. Will address your changes and rebase to the main.

Same for the Spend file.

Ayush170-Future force-pushed on Apr 30, 2024

Ayush170-Future commented at 11:37 pm on April 30, 2024: contributor

Rebased to the main.
Used the new good_data approach to avoid unnecessary fuzz runs.

Ayush170-Future force-pushed on Apr 30, 2024

Ayush170-Future commented at 11:52 pm on April 30, 2024: contributor

lint seems wrong. I couldn’t find any trailing whitespaces in the file.

maflcko commented at 6:07 am on May 1, 2024: member

It is not wrong. No file in this repo uses Windows newlines and they are problematic when shown in git.

Screenshot from 2024-05-01 08-05-25

fuzz: wallet, add target for Crypter d7290d662f

Ayush170-Future force-pushed on May 1, 2024

fanquake referenced this in commit 59b773f42a on May 2, 2024

DrahtBot removed the label CI failed on May 2, 2024

Ayush170-Future commented at 8:33 am on May 2, 2024: contributor

Fixed the lint issue

Ayush170-Future commented at 6:38 pm on May 11, 2024: contributor

A soft ping. Can we move forward with this PR if this looks good?

maflcko commented at 9:22 am on May 13, 2024: member

utACK d7290d662f494503f28e087dd728b492c0bb2c5f

DrahtBot requested review from brunoerg on May 13, 2024

brunoerg approved

brunoerg commented at 12:10 pm on May 13, 2024: contributor

utACK d7290d662f494503f28e087dd728b492c0bb2c5f

marcofleon commented at 8:48 pm on May 16, 2024: contributor

My fuzzer is crashing when I run this. I’m going to look more into it tomorrow.

maflcko commented at 2:25 pm on May 17, 2024: member

My fuzzer is crashing when I run this. I’m going to look more into it tomorrow.

Please indicate the traceback, and the sanitizers you used, also the fuzz input file.

marcofleon commented at 5:47 pm on May 17, 2024: contributor

I used ASan and UBSan. I think it might just be a mac specifc issue, I’m fuzzing on a mac with m3 chip.

Here’s the error:

 0INFO: seed corpus: files: 111934 min: 1b max: 5242880b total: 2040057759b rss: 216Mb
 1[#8192](/bitcoin-bitcoin/8192/)	pulse  cov: 938 ft: 2162 corp: 81/730b exec/s: 4096 rss: 318Mb
 2[#16384](/bitcoin-bitcoin/16384/)	pulse  cov: 1049 ft: 2510 corp: 127/2096b exec/s: 5461 rss: 416Mb
 3[#32768](/bitcoin-bitcoin/32768/)	pulse  cov: 1571 ft: 4427 corp: 219/9334b exec/s: 4681 rss: 565Mb
 4[#65536](/bitcoin-bitcoin/65536/)	pulse  cov: 1774 ft: 7969 corp: 371/56Kb exec/s: 4681 rss: 565Mb
 5libc++abi: terminating due to uncaught exception of type std::bad_alloc: std::bad_alloc
 6==15322== ERROR: libFuzzer: deadly signal
 7    [#0](/bitcoin-bitcoin/0/) 0x10824cee4 in __sanitizer_print_stack_trace+0x28 (libclang_rt.asan_osx_dynamic.dylib:arm64+0x5cee4)
 8    [#1](/bitcoin-bitcoin/1/) 0x105025088 in fuzzer::PrintStackTrace() FuzzerUtil.cpp:210
 9    [#2](/bitcoin-bitcoin/2/) 0x10500ae70 in fuzzer::Fuzzer::CrashCallback() FuzzerLoop.cpp:231
10    [#3](/bitcoin-bitcoin/3/) 0x1944b3580 in _sigtramp+0x34 (libsystem_platform.dylib:arm64+0x4580)
11    [#4](/bitcoin-bitcoin/4/) 0xf96f000194482c1c  (<unknown module>)
12    [#5](/bitcoin-bitcoin/5/) 0xda0700019438fa1c  (<unknown module>)
13    [#6](/bitcoin-bitcoin/6/) 0x2560000194439d2c  (<unknown module>)
14    [#7](/bitcoin-bitcoin/7/) 0x5f79800194429fc8  (<unknown module>)
15    [#8](/bitcoin-bitcoin/8/) 0x23020001940c81dc  (<unknown module>)
16    [#9](/bitcoin-bitcoin/9/) 0x31568001944390f0  (<unknown module>)
17    [#10](/bitcoin-bitcoin/10/) 0x756e00019443c344  (<unknown module>)
18    [#11](/bitcoin-bitcoin/11/) 0x462e00019443c288  (<unknown module>)
19    [#12](/bitcoin-bitcoin/12/) 0x4867000102c4910c  (<unknown module>)
20    [#13](/bitcoin-bitcoin/13/) 0x102c4b74c in std::__1::vector<unsigned char, secure_allocator<unsigned char>>::__vallocate[abi:ne180100](unsigned long) vector:741
21    [#14](/bitcoin-bitcoin/14/) 0x102c45a44 in wallet::(anonymous namespace)::crypter_fuzz_target(Span<unsigned char const>) crypter.cpp:34
22    [#15](/bitcoin-bitcoin/15/) 0x103361680 in LLVMFuzzerTestOneInput fuzz.cpp:179
23    [#16](/bitcoin-bitcoin/16/) 0x10500c298 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:614
24    [#17](/bitcoin-bitcoin/17/) 0x10500bb9c in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) FuzzerLoop.cpp:516
25    [#18](/bitcoin-bitcoin/18/) 0x10500d9b8 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&) FuzzerLoop.cpp:829
26    [#19](/bitcoin-bitcoin/19/) 0x10500da74 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&) FuzzerLoop.cpp:867
27    [#20](/bitcoin-bitcoin/20/) 0x104ffe380 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:914
28    [#21](/bitcoin-bitcoin/21/) 0x105025970 in main FuzzerMain.cpp:20
29    [#22](/bitcoin-bitcoin/22/) 0x1940fa0dc  (<unknown module>)
30    [#23](/bitcoin-bitcoin/23/) 0x810fffffffffffc  (<unknown module>)

And here is the super long crash input https://gist.github.com/marcofleon/f2f50cc2e5eb3c2f5efabc511e4fb6ef

If I add limits (1024 for example) to ConsumeRandomLengthByteVector and ConsumeRandomLengthString the harness works. But that could be defeating the purpose of fuzzing.

brunoerg commented at 8:33 pm on May 17, 2024: contributor

I couldn’t reproduce on MacOS 14.3 with that input file.

If I add limits (1024 for example) to ConsumeRandomLengthByteVector and ConsumeRandomLengthString the harness works. But that could be defeating the purpose of fuzzing.

Changing the harness can invalidate the input. Also, ConsumeRandomLengthByteVector is used in a lot of other targets without a limit. Does it happen for other targets as well, @marcofleon?

marcofleon commented at 3:50 pm on May 18, 2024: contributor

If I don’t start with a seed corpus (inputs from scratch) then it seems to run fine. I let it go for a while and no crashes happen. It’s only when I run something like this: FUZZ=crypter src/test/fuzz/fuzz -set_cover_merge=1 -shuffle=0 -prefer_small=1 -use_value_profile=0 ../qa-assets/fuzz_corpus_copy/crypter ../qa-assets/fuzz_corpus_copy/*

Then it continuously fails to merge and that bad alloc error keeps coming up over and over. I tried this same command on other harnesses that contain ConsumeRandomLengthByteVector (crypto, crypto_aes256cbc, addrman, net, http_request, script, connman) and they all result in a normal output. @brunoerg, if you don’t mind could you try a couple of these other crash inputs too? https://gist.github.com/marcofleon/b4f59f23c5b3e2a37da362f556fdbd4f https://gist.github.com/marcofleon/62558bc4a18e90c92a164a7498f486c2 https://gist.github.com/marcofleon/8b44613426d50ada97f87db3c3195442 https://gist.github.com/marcofleon/dfdc88df73800617a19ca9f509f20417 https://gist.github.com/marcofleon/ff19b5a5b7dcde735e75fc583a1343c3 https://gist.github.com/marcofleon/77ac845c8d43d3b9375bb5163374fce7

maflcko commented at 4:09 pm on May 18, 2024: member

set_cover_merge

This uses more memory than -merge=1, so my recommendation would be to try to add more memory or swap to your machine, or to use -merge=1 for now.

marcofleon commented at 10:34 am on May 19, 2024: contributor

Adjusted some things on my machine and used -merge=1 and it successfully merges after several attempts. Thank you for the help @maflcko and @brunoerg

achow101 commented at 0:32 am on June 5, 2024: member

ACK d7290d662f494503f28e087dd728b492c0bb2c5f

I get that any coverage of these functions is better than no coverage, but it seems less useful to me when we aren’t checking the output. There may not be a crash, but each function could be doing something unexpected, and we should be checking to make sure that they aren’t. I’m going to merge this for now though just so there is coverage, but a followup which verifies the behavior would be welcomed.

achow101 merged this on Jun 5, 2024

achow101 closed this on Jun 5, 2024

bitcoin deleted a comment on Jun 5, 2024

in src/wallet/test/fuzz/crypter.cpp:65 in d7290d662f

60+            },
61+            [&] {
62+                (void)crypt.Encrypt(plain_text_ed, cipher_text_ed);
63+            },
64+            [&] {
65+                (void)crypt.Decrypt(cipher_text_ed, plain_text_ed);

maflcko commented at 10:07 am on June 9, 2024:

https://github.com/bitcoin/bitcoin/issues/30251

hebasto added the label Needs CMake port on Aug 25, 2024

maflcko removed the label Needs CMake port on Aug 29, 2024

fuzz: wallet, add target for Crypter #28074

Code Coverage

Reviews

fuzz: wallet, add target for `Crypter` #28074