Skip to content

[fuzz] Improve transport deserialization fuzz test coverage #22029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 27, 2021

Conversation

dhruv
Copy link
Contributor

@dhruv dhruv commented May 24, 2021

This PR has 3 commits that increase the fuzz test coverage:

Before commit 1:

#306853 REDUCE cov: 798 ft: 5820 corp: 150/375Kb lim: 68333 exec/s: 1382 rss: 461Mb L: 254/63171 MS: 1 EraseBytes-
#1453105 REDUCE cov: 798 ft: 5820 corp: 150/369Kb lim: 79613 exec/s: 1467 rss: 461Mb L: 6027/60873 MS: 1 EraseBytes-

After commit 1 (adds serialization to de-serialization test):

#303389 NEW cov: 1202 ft: 8382 corp: 157/382Kb lim: 68189 exec/s: 1451 rss: 447Mb L: 1386/65459 MS: 1 CopyPart-
#1428759 REDUCE cov: 1202 ft: 8512 corp: 169/389Kb lim: 78749 exec/s: 1528 rss: 463Mb L: 1627/60488 MS: 1 EraseBytes-

After commit 2 (provides an occasional checksum assist to the fuzzer inputs):

#304820 NEW cov: 1440 ft: 4452 corp: 92/12551b lim: 2237 exec/s: 3386 rss: 486Mb L: 47/1111 MS: 1 ChangeByte-
#1416181 REDUCE cov: 1442 ft: 5681 corp: 125/59Kb lim: 4096 exec/s: 3522 rss: 535Mb L: 2164/4049 MS: 1 EraseBytes-

After commit 3 (provides an occasional magic bytes assist to the fuzzer inputs):

#302684 NEW cov: 1454 ft: 3936 corp: 84/7056b lim: 2424 exec/s: 4146 rss: 477Mb L: 65/1108 MS: 3 CopyPart-CrossOver-CMP- DE: "\x0e\x00\x00\x00"-
#1383925 REDUCE cov: 1454 ft: 4828 corp: 102/14573b lim: 4096 exec/s: 3954 rss: 534Mb L: 116/4050 MS: 2 EraseBytes-ChangeByte-

If reviewers only accept the first commit, the seeds are not invalidated and new seeds are at: bitcoin-core/qa-assets#61. In this case, we can also revert the test name change.

If reviewers accept all three commits, the existing seeds are invalidated.

@practicalswift
Copy link
Contributor

Concept ACK

Note that the harness rename will unnecessarily invalidate all existing seeds. Non-blocking suggestion: could skip the rename? :)

@dhruv
Copy link
Contributor Author

dhruv commented May 24, 2021

Note that the harness rename will unnecessarily invalidate all existing seeds. Non-blocking suggestion: could skip the rename? :)

I renamed the directory for seeds in the first commit here. Will seeds still be invalidated? I'm happy to skip the rename if that is not sufficient.

Separately, I added some instrumentation and saw 67% of the seeds are not getting more coverage because of an invalid dbl-sha256 checksum - the fuzzer could use a little help. Another 30% because of an invalid message type. I am preparing a patch that uses ConsumeBool() to inject a valid checksum half the time. I'll report back if I see meaningfully increased coverage. Do you think that would be a good change, @practicalswift ?

@maflcko
Copy link
Member

maflcko commented May 25, 2021

Agree that the rename is better skipped

dhruv added 3 commits May 25, 2021 08:08
Before commit:
306853	REDUCE cov: 798 ft: 5820 corp: 150/375Kb lim: 68333 exec/s: 1382 rss: 461Mb L: 254/63171 MS: 1 EraseBytes-
1453105	REDUCE cov: 798 ft: 5820 corp: 150/369Kb lim: 79613 exec/s: 1467 rss: 461Mb L: 6027/60873 MS: 1 EraseBytes-

After commit:
303389	NEW    cov: 1202 ft: 8382 corp: 157/382Kb lim: 68189 exec/s: 1451 rss: 447Mb L: 1386/65459 MS: 1 CopyPart-
1428759	REDUCE cov: 1202 ft: 8512 corp: 169/389Kb lim: 78749 exec/s: 1528 rss: 463Mb L: 1627/60488 MS: 1 EraseBytes-
Before commit:
Unable to deserialize: 0%
Wrong message start  : ~1.27%
Header too large     : ~0.5%
Wrong checksum       : ~67.99%
Invalid message type : ~30.1%

303389	NEW    cov: 1202 ft: 8382 corp: 157/382Kb lim: 68189 exec/s: 1451 rss: 447Mb L: 1386/65459 MS: 1 CopyPart-
1428759	REDUCE cov: 1202 ft: 8512 corp: 169/389Kb lim: 78749 exec/s: 1528 rss: 463Mb L: 1627/60488 MS: 1 EraseBytes-

After commit(new seeds; old seeds invalidated):
Unable to deserialize: 0%
Wrong message start  : ~45.62%
Header too large     : ~14.5%
Wrong checksum       : ~38.13%
Invalid message type : ~1.78%

304820	NEW    cov: 1440 ft: 4452 corp: 92/12551b lim: 2237 exec/s: 3386 rss: 486Mb L: 47/1111 MS: 1 ChangeByte-
1416181	REDUCE cov: 1442 ft: 5681 corp: 125/59Kb lim: 4096 exec/s: 3522 rss: 535Mb L: 2164/4049 MS: 1 EraseBytes-
Before commit:
Unable to deserialize : 0%
Wrong message start   : ~45.62%
Header too large      : ~14.5%
Wrong checksum        : ~38.13%
Invalid message type  : ~1.78%

304820	NEW    cov: 1440 ft: 4452 corp: 92/12551b lim: 2237 exec/s: 3386 rss: 486Mb L: 47/1111 MS: 1 ChangeByte-
1416181	REDUCE cov: 1442 ft: 5681 corp: 125/59Kb lim: 4096 exec/s: 3522 rss: 535Mb L: 2164/4049 MS: 1 EraseBytes-

After commit:
Unable to deserialize : 0%
Wrong message start   : ~39.6%
Header too large      : ~30.85%
Wrong checksum        : ~25.54%
Invalid message type  : ~4.01%

302684	NEW    cov: 1454 ft: 3936 corp: 84/7056b lim: 2424 exec/s: 4146 rss: 477Mb L: 65/1108 MS: 3 CopyPart-CrossOver-CMP- DE: "\x0e\x00\x00\x00"-
1383925	REDUCE cov: 1454 ft: 4828 corp: 102/14573b lim: 4096 exec/s: 3954 rss: 534Mb L: 116/4050 MS: 2 EraseBytes-ChangeByte-
@dhruv dhruv force-pushed the v1-transport-serializer-fuzzing branch from 7e7d146 to e337145 Compare May 25, 2021 15:23
@dhruv dhruv changed the title [fuzz] Add serialization to transport deserialization test [fuzz] Improve transport deserialization fuzz test coverage May 25, 2021
@dhruv
Copy link
Contributor Author

dhruv commented May 25, 2021

Thank you @MarcoFalke, @practicalswift. I ended up adding two more commits which meaningfully increased coverage by on occasion, assisting the fuzzer inputs with valid magic bytes and checksums (see updated PR description). These will invalidate the seeds. Please take another look when you can. If the feedback is to skip the last two commits, I will revert the name change.

Ready for further review.

@practicalswift
Copy link
Contributor

Tested ACK e337145

Coverage is increased significantly.

Thanks for improving the fuzzing harnesses!

@laanwj laanwj merged commit e20745c into bitcoin:master May 27, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 27, 2021
…st coverage

e337145 [fuzz] Occasional valid magic bytes for transport serialization test (Dhruv Mehta)
35571d8 [fuzz] Occasional valid checksum for transport serialization fuzz test (Dhruv Mehta)
654472a [fuzz] Add serialization to deserialization test (Dhruv Mehta)

Pull request description:

  This PR has 3 commits that increase the fuzz test coverage:

  Before commit 1:
  ```
  #306853 REDUCE cov: 798 ft: 5820 corp: 150/375Kb lim: 68333 exec/s: 1382 rss: 461Mb L: 254/63171 MS: 1 EraseBytes-
  #1453105 REDUCE cov: 798 ft: 5820 corp: 150/369Kb lim: 79613 exec/s: 1467 rss: 461Mb L: 6027/60873 MS: 1 EraseBytes-
  ```

  After commit 1 (adds serialization to de-serialization test):
  ```
  #303389 NEW cov: 1202 ft: 8382 corp: 157/382Kb lim: 68189 exec/s: 1451 rss: 447Mb L: 1386/65459 MS: 1 CopyPart-
  #1428759 REDUCE cov: 1202 ft: 8512 corp: 169/389Kb lim: 78749 exec/s: 1528 rss: 463Mb L: 1627/60488 MS: 1 EraseBytes-
  ```

  After commit 2 (provides an occasional checksum assist to the fuzzer inputs):
  ```
  #304820 NEW cov: 1440 ft: 4452 corp: 92/12551b lim: 2237 exec/s: 3386 rss: 486Mb L: 47/1111 MS: 1 ChangeByte-
  #1416181 REDUCE cov: 1442 ft: 5681 corp: 125/59Kb lim: 4096 exec/s: 3522 rss: 535Mb L: 2164/4049 MS: 1 EraseBytes-
  ```

  After commit 3 (provides an occasional magic bytes assist to the fuzzer inputs):
  ```
  #302684 NEW cov: 1454 ft: 3936 corp: 84/7056b lim: 2424 exec/s: 4146 rss: 477Mb L: 65/1108 MS: 3 CopyPart-CrossOver-CMP- DE: "\x0e\x00\x00\x00"-
  #1383925 REDUCE cov: 1454 ft: 4828 corp: 102/14573b lim: 4096 exec/s: 3954 rss: 534Mb L: 116/4050 MS: 2 EraseBytes-ChangeByte-
  ```

  If reviewers only accept the first commit, the seeds are not invalidated and new seeds are at: bitcoin-core/qa-assets#61. In this case, we can also revert the test name change.

  If reviewers accept all three commits, the existing seeds are invalidated.

ACKs for top commit:
  practicalswift:
    Tested ACK e337145

Tree-SHA512: d37f06eea0249322b00a99c4827359eb53aeb711751e5571f4681eeca06dc257e0c4cd4887150fc37cc2f689e26986112d768066ad274361615ba9b6a522c61a
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

5 participants