Skip to content

Conversation

@maflcko
Copy link
Contributor

@maflcko maflcko commented Oct 17, 2023

Doing this in a single commit to allow the fuzz engine to create a smaller set of files.

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, appears to just add 30k odd new fuzz inputs

ACK 0dbdd55

@maflcko
Copy link
Contributor Author

maflcko commented Oct 17, 2023

You are adding around 20k, so I wonder if there is an overlap. Should I redo with your folder added?

@murchandamus
Copy link
Contributor

Sure, feel free to pull it in, that’s what I pushed the branches for! :)

@maflcko
Copy link
Contributor Author

maflcko commented Oct 18, 2023

Ok, running that now.

Also, it looks like we are nearing the time limit here.

So, I'd say to merge this and then right after open the pull request to delete-nonreduced.sh?

@maflcko
Copy link
Contributor Author

maflcko commented Oct 18, 2023

@murchandamus
Copy link
Contributor

Ok, running that now.

Also, it looks like we are nearing the time limit here.

So, I'd say to merge this and then right after open the pull request to delete-nonreduced.sh?

Approach ACK

@maflcko
Copy link
Contributor Author

maflcko commented Oct 18, 2023

Hmm, for some reason I am running out of disk space on my tmpfs /tmp. I'll run again over night and push tomorrow morning, if it works 🥲

@maflcko
Copy link
Contributor Author

maflcko commented Oct 19, 2023

Now the task is DONE, but it failed (:smiling_face_with_tear:)

#131072	pulse  cov: 3399 exec/s: 672 rss: 219Mb
#262144	pulse  cov: 6464 exec/s: 579 rss: 219Mb
#451694	DONE   cov: 7097 exec/s: 422 rss: 316Mb
MERGE-OUTER: successful in 1 attempt(s)
MERGE-OUTER: the control file has 4052306816 bytes
==1059184== ERROR: libFuzzer: out-of-memory (used: 2084Mb; limit: 2048Mb)
   To change the out-of-memory limit use -rss_limit_mb=<N>

SUMMARY: libFuzzer: out-of-memory

@maflcko
Copy link
Contributor Author

maflcko commented Oct 19, 2023

Ok, now it passed, with this patch applied to avoid out-of-memory and out-of-storage:

diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py
index 9d64591111..591689b11c 100755
--- a/test/fuzz/test_runner.py
+++ b/test/fuzz/test_runner.py
@@ -278,6 +278,8 @@ def merge_inputs(*, fuzz_pool, corpus, test_list, src_dir, build_dir, merge_dirs
         args = [
             os.path.join(build_dir, 'src', 'test', 'fuzz', 'fuzz'),
             '-set_cover_merge=1',
+            '-rss_limit_mb=8000',
+            f'-merge_control_file=/run/media/marco/scratch_fde/fuzz/A_m_{t}.txt',
             # set_cover_merge is used instead of -merge=1 to reduce the overall
             # size of the qa-assets git repository a bit, but more importantly,
             # to cut the runtime to iterate over all fuzz inputs [0].

@maflcko maflcko requested a review from murchandamus October 19, 2023 13:15
Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified that the second commit only modifies a bunch of I2P. Both the first and the second commit’s I2P seeds pass on my machine:

➜  qa-assets git:(main) ✗ git reset --hard maflcko/main
Updating files: 100% (36457/36457), done.
HEAD is now at c764d6c7842 Revert i2p fuzz inputs to work around crash for now
➜  build git:(2023-10-refactor-coinselector-tests) ✗ FUZZ=i2p src/test/fuzz/fuzz ../../qa-assets/fuzz_seed_corpus/i2p                                                                     <<<
i2p: succeeded against 1247 files in 2s.
➜  qa-assets git:(main) ✗ git reset --hard 00c956e4a9c555da5640946ec28cdd5e501c0554
HEAD is now at 00c956e4a9c Add fuzz inputs from my servers, murch, and oss-fuzz
➜  build git:(2023-10-refactor-coinselector-tests) ✗ FUZZ=i2p src/test/fuzz/fuzz ../../qa-assets/fuzz_seed_corpus/i2p
i2p: succeeded against 1256 files in 2s.

🤷

The first commits seems to add a bunch of files into the fuzz_seed_corpus directly instead of one of the subfolders. Any idea what may have happened there?

@maflcko
Copy link
Contributor Author

maflcko commented Oct 19, 2023

The first commits seems to add a bunch of files into the fuzz_seed_corpus directly instead of one of the subfolders. Any idea what may have happened there?

Can you give an example?

@maflcko
Copy link
Contributor Author

maflcko commented Oct 19, 2023

Both the first and the second commit’s I2P seeds pass on my machine:

That also seems funny. You could reproduce yesterday (at least in your folder), I can reproduce locally, and both CI tasks failed on your pull requests with this error.

@murchandamus
Copy link
Contributor

Sorry, PEBCAK on the files. In my tig window, it dropped the fuzz target folder on some files in the change list, but not the others. I didn’t realize that it would present me different levels of the path in one list. 🤦
image.

➜  qa-assets git:(main) ✗ git reset --hard maflcko/main                                   
HEAD is now at c764d6c7842 Revert i2p fuzz inputs to work around crash for now
➜  bitcoin git:(master) ✗ cd ../bitcoin/build                                            
➜  build git:(master) ✗ FUZZ=i2p src/test/fuzz/fuzz ../../qa-assets/fuzz_seed_corpus/i2p
i2p: succeeded against 1247 files in 27s.
➜  build git:(master) ✗ cd ../../qa-assets
➜  qa-assets git:(main) ✗ git reset --hard 00c956e4a9c555da5640946ec28cdd5e501c0554       
HEAD is now at 00c956e4a9c Add fuzz inputs from my servers, murch, and oss-fuzz
➜  qa-assets git:(main) ✗ cd -                                                            
~/Workspace/bitcoin/build
➜  build git:(master) ✗ FUZZ=i2p src/test/fuzz/fuzz ../../qa-assets/fuzz_seed_corpus/i2p
i2p: succeeded against 1256 files in 26s.
➜  build git:(master) ✗ 

I don’t know why it doesn’t crash on my machine but I can not replicate it crashing with either of your two commits here.

@maflcko
Copy link
Contributor Author

maflcko commented Oct 19, 2023

In any case, let's move discussion to bitcoin/bitcoin#28665

@maflcko
Copy link
Contributor Author

maflcko commented Oct 19, 2023

Which fuzzing engine are you using? Can you try with libFuzzer?

I've never seen this command and output:

FUZZ=i2p src/test/fuzz/fuzz ../../qa-assets/fuzz_seed_corpus/i2p
i2p: succeeded against 1247 files in 27s.

@maflcko
Copy link
Contributor Author

maflcko commented Oct 20, 2023

@hebasto
Copy link
Member

hebasto commented Oct 21, 2023

Could this PR cause CI timeouts?

See: bitcoin/bitcoin#28703.

fanquake added a commit to bitcoin-core/gui that referenced this pull request Oct 23, 2023
fa21535 fuzz: Increase merge -rss_limit_mb (MarcoFalke)

Pull request description:

  For some reason, the limit is hit. (Presumably due to `-set_cover_merge=1` eating more memory, or by simply having more fuzz inputs).

  Fix it by increasing it for the merge operation.

ACKs for top commit:
  dergoegge:
    ACK fa21535
  hebasto:
    ACK fa21535, considering the discussion in bitcoin-core/qa-assets#155.

Tree-SHA512: 4fed0f254eccc6fe0b53656bc345ff898b13811dc39387387317d34b521ab77cee03d82b0896dd92d253b7546b6a7e4bdcd478749f47064374ab44ad759ab9ff
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Nov 28, 2023
fa21535 fuzz: Increase merge -rss_limit_mb (MarcoFalke)

Pull request description:

  For some reason, the limit is hit. (Presumably due to `-set_cover_merge=1` eating more memory, or by simply having more fuzz inputs).

  Fix it by increasing it for the merge operation.

ACKs for top commit:
  dergoegge:
    ACK fa21535
  hebasto:
    ACK fa21535, considering the discussion in bitcoin-core/qa-assets#155.

Tree-SHA512: 4fed0f254eccc6fe0b53656bc345ff898b13811dc39387387317d34b521ab77cee03d82b0896dd92d253b7546b6a7e4bdcd478749f47064374ab44ad759ab9ff
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
fa21535 fuzz: Increase merge -rss_limit_mb (MarcoFalke)

Pull request description:

  For some reason, the limit is hit. (Presumably due to `-set_cover_merge=1` eating more memory, or by simply having more fuzz inputs).

  Fix it by increasing it for the merge operation.

ACKs for top commit:
  dergoegge:
    ACK fa21535
  hebasto:
    ACK fa21535, considering the discussion in bitcoin-core/qa-assets#155.

Tree-SHA512: 4fed0f254eccc6fe0b53656bc345ff898b13811dc39387387317d34b521ab77cee03d82b0896dd92d253b7546b6a7e4bdcd478749f47064374ab44ad759ab9ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants