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

Ayush170-Future · 2023-07-13T13:43:32Z

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 · 2023-07-13T13:43:36Z

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.

Ayush170-Future · 2023-07-13T13:49:36Z

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.

src/wallet/test/fuzz/crypter.cpp

brunoerg

ACK 6c98f4c

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:

diff --git a/src/wallet/test/fuzz/crypter.cpp b/src/wallet/test/fuzz/crypter.cpp
index 7ba43821b..03ad97a35 100644
--- a/src/wallet/test/fuzz/crypter.cpp
+++ b/src/wallet/test/fuzz/crypter.cpp
@@ -24,6 +24,9 @@ FUZZ_TARGET_INIT(crypter, initialize_crypter)
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
 
     CCrypter crypt;
+    std::vector<unsigned char> cipher_text_ed;
+    CKeyingMaterial plain_text_ed;
+
 
     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000)
     {
@@ -49,14 +52,16 @@ FUZZ_TARGET_INIT(crypter, initialize_crypter)
             },
             [&] {
                 const std::vector<unsigned char> random_vector = ConsumeRandomLengthByteVector(fuzzed_data_provider);
-                const CKeyingMaterial plain_text(random_vector.begin(), random_vector.end());
-                std::vector<unsigned char> cipher_text;
-                crypt.Encrypt(plain_text, cipher_text);
+                plain_text_ed = CKeyingMaterial(random_vector.begin(), random_vector.end());
             },
             [&] {
-                CKeyingMaterial plain_text;
-                const std::vector<unsigned char> cipher_text = ConsumeRandomLengthByteVector(fuzzed_data_provider);
-                crypt.Decrypt(cipher_text, plain_text);
+                std::vector<unsigned char> cipher_text_ed = ConsumeRandomLengthByteVector(fuzzed_data_provider);
+            },
+            [&] {
+                crypt.Encrypt(plain_text_ed, cipher_text_ed);
+            },
+            [&] {
+                crypt.Decrypt(cipher_text_ed, plain_text_ed);
             },
             [&] {

Ayush170-Future · 2023-07-13T16:21:42Z

Thanks, @MarcoFalke and @brunoerg for the reviews.

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

brunoerg

reACK e010fdd

src/wallet/test/fuzz/crypter.cpp

Ayush170-Future · 2023-07-17T17:25:54Z

Forced pushed updating changes in PR 28065.

brunoerg

reACK 7570e22

maflcko · 2023-07-22T09:08:09Z

src/wallet/test/fuzz/crypter.cpp

+                plain_text_ed = CKeyingMaterial(random_vector.begin(), random_vector.end());
+            },
+            [&] {
+                std::vector<unsigned char> cipher_text_ed = ConsumeRandomLengthByteVector(fuzzed_data_provider);


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

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

Ayush170-Future · 2023-07-22T09:27:24Z

Forced pushed fixing the Shadow variable issue.

dergoegge · 2023-08-03T13:04:01Z

src/wallet/test/fuzz/crypter.cpp

+                const std::string random_string = fuzzed_data_provider.ConsumeRandomLengthString();
+                SecureString secure_string(random_string.begin(), random_string.end());
+
+                const unsigned int nDerivationMethod = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<unsigned int>();


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

Suggested change

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

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

dergoegge · 2023-08-03T13:09:16Z

src/wallet/test/fuzz/crypter.cpp

+                crypt.SetKeyFromPassphrase(/*strKeyData=*/secure_string,
+                                           /*chSalt=*/ConsumeRandomLengthByteVector(fuzzed_data_provider),
+                                           /*nRounds=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, 25000),
+                                           /*nDerivationMethod=*/ nDerivationMethod);


style nit:

Suggested change

/*nDerivationMethod=*/ nDerivationMethod);

/*nDerivationMethod=*/nDerivationMethod);

dergoegge · 2023-08-03T13:27:40Z

src/wallet/test/fuzz/crypter.cpp

+    g_setup = testing_setup.get();
+}
+
+FUZZ_TARGET(crypter, .init = initialize_crypter)


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:

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

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?)

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?

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?

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)

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)

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.

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

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

Ayush170-Future · 2023-08-03T18:20:07Z

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 · 2024-04-30T23:37:39Z

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

Ayush170-Future · 2024-04-30T23:52:43Z

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

maflcko · 2024-05-01T06:07:31Z

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

… be used fa9be2f lint: [doc] Clarify Windows line endings (CR LF) not to be used (MarcoFalke) Pull request description: It has been this case since the linter was introduced years ago. Given a misunderstanding (#28074 (comment)), clarify the docs. ACKs for top commit: brunoerg: ACK fa9be2f Tree-SHA512: be714db9df533e0962ed84102ffdb72717902949b930d58cf5a79cba36297f6b2b1f75e65a2c1f46bcb8e2f4ad5d025f3d15210f468a5ec9631a580b74f923ea

Ayush170-Future · 2024-05-02T08:33:14Z

Fixed the lint issue

Ayush170-Future · 2024-05-11T18:38:03Z

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

maflcko · 2024-05-13T09:22:52Z

utACK d7290d6

brunoerg

utACK d7290d6

marcofleon

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

maflcko · 2024-05-17T14:25:25Z

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

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:

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

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 · 2024-05-18T15:50:22Z

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 · 2024-05-18T16:09:48Z

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 · 2024-05-19T10:34:53Z

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 · 2024-06-05T00:32:20Z

ACK d7290d6

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.

maflcko · 2024-06-09T10:07:33Z

src/wallet/test/fuzz/crypter.cpp

+                (void)crypt.Encrypt(plain_text_ed, cipher_text_ed);
+            },
+            [&] {
+                (void)crypt.Decrypt(cipher_text_ed, plain_text_ed);


DrahtBot added the Tests label Jul 13, 2023

maflcko reviewed Jul 13, 2023

View reviewed changes

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

maflcko approved these changes Jul 13, 2023

View reviewed changes

brunoerg approved these changes Jul 13, 2023

View reviewed changes

Ayush170-Future force-pushed the fuzz-coverage-crypter branch 2 times, most recently from 0e2c657 to f4ba405 Compare July 13, 2023 16:16

DrahtBot added the CI failed label Jul 13, 2023

Ayush170-Future force-pushed the fuzz-coverage-crypter branch from f4ba405 to e010fdd Compare July 13, 2023 18:31

DrahtBot removed the CI failed label Jul 13, 2023

Ayush170-Future requested a review from brunoerg July 14, 2023 12:33

brunoerg approved these changes Jul 14, 2023

View reviewed changes

maflcko approved these changes Jul 14, 2023

View reviewed changes

maflcko reviewed Jul 17, 2023

View reviewed changes

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

Ayush170-Future force-pushed the fuzz-coverage-crypter branch from e010fdd to 7570e22 Compare July 17, 2023 17:22

brunoerg approved these changes Jul 17, 2023

View reviewed changes

bitcoin deleted a comment Jul 22, 2023

maflcko reviewed Jul 22, 2023

View reviewed changes

Ayush170-Future force-pushed the fuzz-coverage-crypter branch from 7570e22 to fe1a271 Compare July 22, 2023 09:23

dergoegge suggested changes Aug 3, 2023

View reviewed changes

Ayush170-Future force-pushed the fuzz-coverage-crypter branch from fe1a271 to 04a2a36 Compare August 3, 2023 18:16

Ayush170-Future force-pushed the fuzz-coverage-crypter branch from 04a2a36 to 066402c Compare August 3, 2023 18:22

achow101 requested a review from brunoerg September 20, 2023 17:25

Ayush170-Future force-pushed the fuzz-coverage-crypter branch from a8ec1f1 to 6697ada Compare April 30, 2024 23:41

maflcko mentioned this pull request May 1, 2024

lint: [doc] Clarify Windows line endings (CR LF) not to be used #30010

Merged

fuzz: wallet, add target for Crypter

d7290d6

Ayush170-Future force-pushed the fuzz-coverage-crypter branch from 6697ada to d7290d6 Compare May 1, 2024 19:45

DrahtBot removed the CI failed label May 2, 2024

DrahtBot requested a review from brunoerg May 13, 2024 09:22

brunoerg approved these changes May 13, 2024

View reviewed changes

marcofleon reviewed May 16, 2024

View reviewed changes

marcofleon reviewed May 17, 2024

View reviewed changes

achow101 merged commit b3a61bd into bitcoin:master Jun 5, 2024

bitcoin deleted a comment Jun 5, 2024

maflcko reviewed Jun 9, 2024

View reviewed changes

marcofleon mentioned this pull request Jun 19, 2024

Initial vecdeque inputs bitcoin-core/qa-assets#190

Merged

hebasto added the Needs CMake port label Aug 25, 2024

maflcko removed the Needs CMake port label Aug 29, 2024

hebasto mentioned this pull request Aug 29, 2024

ci: Add missed configuration options to "Win64 native" job #30755

Merged

bitcoin locked and limited conversation to collaborators Aug 29, 2025

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

	/nDerivationMethod=/ nDerivationMethod);
	/nDerivationMethod=/nDerivationMethod);

fuzz: wallet, add target for Crypter #28074

fuzz: wallet, add target for Crypter #28074

Conversation

Ayush170-Future commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Uh oh!

Ayush170-Future commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

brunoerg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush170-Future commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ayush170-Future commented Jul 17, 2023

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush170-Future commented Jul 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush170-Future commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ayush170-Future commented Apr 30, 2024

Uh oh!

Ayush170-Future commented Apr 30, 2024

Uh oh!

maflcko commented May 1, 2024

Uh oh!

Ayush170-Future commented May 2, 2024

Uh oh!

Ayush170-Future commented May 11, 2024

Uh oh!

maflcko commented May 13, 2024

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

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

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

Ayush170-Future commented Jul 13, 2023 •

edited

Loading

DrahtBot commented Jul 13, 2023 •

edited

Loading

Ayush170-Future commented Jul 13, 2023 •

edited

Loading

brunoerg left a comment •

edited

Loading

Ayush170-Future commented Jul 13, 2023 •

edited

Loading

maflcko Jul 22, 2023 •

edited

Loading

maflcko Aug 3, 2023 •

edited

Loading

Ayush170-Future commented Aug 3, 2023 •

edited

Loading

marcofleon left a comment •

edited

Loading

brunoerg commented May 17, 2024 •

edited

Loading

marcofleon commented May 18, 2024 •

edited

Loading

marcofleon commented May 19, 2024 •

edited

Loading