net: Make DNS lookup mockable, add fuzzing harness #19415

practicalswift · 2020-06-29T22:33:26Z

Make DNS lookup mockable/testable/fuzzable.

Add fuzzing harness for Lookup(…)/LookupHost(…)/LookupNumeric(…)/LookupSubNet(…).

See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

Happy fuzzing :)

DrahtBot · 2020-06-30T02:48:34Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net, refactor: pass uint16 CService::port as uint16 #21328 (net, refactor: pass uint16 CService::port as uint16 by jonatack)
refactor: net: subnet lookup: use single-result LookupHost() #17160 (refactor: net: subnet lookup: use single-result LookupHost() by theStack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

naumenkogs · 2020-06-30T12:50:56Z

Concept ACK

Crypt-iQ

Tested ACK 426617a.
Coverage for ~48 hours of libfuzzer fuzzing here: https://crypt-iq.github.io/netbase_dns_cov_outs/src/netbase.cpp.gcov.html

src/netbase.cpp

vasild

ACK 522b3b4

Some nits below.

Ran this for some time and submitted the generated seeds at bitcoin-core/qa-assets#50.

src/netbase.cpp

src/test/fuzz/netbase_dns_lookup.cpp

src/netbase.h

vasild

ACK 24e3ce3

jonatack · 2021-03-06T21:51:08Z

Concept ACK, will review.

I get the following complaint when running --with-sanitizers=integer because CService expects uint16_t
netbase.cpp:240:37: runtime error: implicit conversion from type 'int' of value 210314121 (32-bit, signed) to type 'uint16_t' (aka 'unsigned short') changed the value to 9097 (16-bit, unsigned)
bitcoin/src/netbase.cpp

Line 240 in 1a22951

vAddr[i] = CService(vIP[i], port);

@Crypt-iQ Good find. This should be fixed with #21328.

Crypt-iQ

Some comments, will test again

src/netbase.cpp

src/test/fuzz/netbase_dns_lookup.cpp

jonatack

Tested approach ACK, a few comments below.

$ FUZZ=netbase_dns_lookup src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/
INFO: Seed: 3479425365
INFO: Loaded 1 modules   (643152 inline 8-bit counters): 643152 [0x555d231dc188, 0x555d232791d8), 
INFO: Loaded 1 PC tables (643152 PCs): 643152 [0x555d232791d8,0x555d23c496d8), 
INFO:   129774 files found in ../qa-assets/fuzz_seed_corpus/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
INFO: seed corpus: files: 129774 min: 1b max: 3986616b total: 1875939121b rss: 231Mb
#8192	pulse  cov: 970 ft: 1825 corp: 59/244b exec/s: 4096 rss: 337Mb
#16384	pulse  cov: 1763 ft: 3612 corp: 98/809b exec/s: 4096 rss: 436Mb
#32768	pulse  cov: 2090 ft: 6274 corp: 158/2458b exec/s: 4681 rss: 590Mb
#65536	pulse  cov: 2121 ft: 7423 corp: 192/5608b exec/s: 5041 rss: 593Mb
#129775	INITED cov: 2235 ft: 10320 corp: 297/984Kb exec/s: 3415 rss: 1271Mb
#129813	REDUCE cov: 2235 ft: 10320 corp: 297/984Kb lim: 286065 exec/s: 3416 rss: 1271Mb L: 62/286047 MS: 3 ChangeBit-ChangeBit-EraseBytes-
.../...
#113401432	REDUCE cov: 2669 ft: 12318 corp: 458/295Kb lim: 1048576 exec/s: 1595 rss: 1271Mb L: 942/16122 MS: 4 CMP-ChangeBit-ChangeBit-EraseBytes- DE: "\x9e\xff\xff\xff"-
#113449634	REDUCE cov: 2669 ft: 12318 corp: 458/295Kb lim: 1048576 exec/s: 1595 rss: 1271Mb L: 2682/16122 MS: 2 ShuffleBytes-EraseBytes-
#113480169	REDUCE cov: 2669 ft: 12318 corp: 458/295Kb lim: 1048576 exec/s: 1595 rss: 1271Mb L: 7438/16122 MS: 5 ChangeBit-EraseBytes-PersAutoDict-InsertRepeatedBytes-InsertRepeatedBytes- DE: "\x00\x00bP\x01_\x86\xac"-

src/netbase.cpp

src/test/fuzz/netbase_dns_lookup.cpp

…ric(...)/LookupSubNet(...)

practicalswift · 2021-03-08T23:20:49Z

@jonatack @Crypt-iQ Thanks for reviewing. Feedback addressed.

This PR should now hopefully be ready for final review :)

Crypt-iQ · 2021-03-14T23:39:10Z

cr ACK e528075

vasild

ACK e528075

maflcko · 2021-03-15T11:10:40Z

src/test/fuzz/netbase_dns_lookup.cpp

+#include <vector>
+
+namespace {
+FuzzedDataProvider* fuzzed_data_provider_ptr = nullptr;


style-nit: Any reason for this namespace and pointer? Looks like a lambda should be able to do the same in less code and smaller scope

Using a lambda here is a good idea: less code, smaller scope and overall beautiful :)

TBH I don't remember why I didn't do it that way nine months ago when this code was written: a lambda is strictly better AFAICT :)

Address in PR #21443.

jonatack · 2021-03-15T11:18:06Z

ACK e528075

Thanks for the update and s/aiTrav/ai_trav/ was a nice touchup.

e528075 tests: Add fuzzing harness for Lookup(...)/LookupHost(...)/LookupNumeric(...)/LookupSubNet(...) (practicalswift) c6b4bfb net: Make DNS lookup code testable (practicalswift) Pull request description: Make DNS lookup mockable/testable/fuzzable. Add fuzzing harness for `Lookup(…)`/`LookupHost(…)`/`LookupNumeric(…)`/`LookupSubNet(…)`. See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets). Happy fuzzing :) ACKs for top commit: Crypt-iQ: cr ACK e528075 vasild: ACK e528075 Tree-SHA512: 9984c2e2fedc3c1e1c3dbd701bb739ebd2f01766e6e83543dae5ae43eb8646c452bba0e101dd2f06079e5258bd5846c7d27a60ed5d77c1682b54c9544ffad443

7c8c140 fuzz: Implement fuzzed_dns_lookup_function as lambda (practicalswift) Pull request description: Implement `fuzzed_dns_lookup_function` as a lambda. As wisely suggested by MarcoFalke in bitcoin/bitcoin#19415 (comment). Thanks! :) ACKs for top commit: MarcoFalke: cr ACK 7c8c140 vasild: ACK 7c8c140 Tree-SHA512: b175f2ad42e9a2be1f769ac677b2872e73ae621731d27e9a24bedadc14d9a6682c7fd1946a0df436d37e7b0cc0d212c1eef69f0409fb975cf9c460cd45f6e4ac

Summary: This is a partial backport of [[bitcoin/bitcoin#19415 | core#19415]] bitcoin/bitcoin@c6b4bfb Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11067

DrahtBot added Build system P2P Tests labels Jun 29, 2020

maflcko removed Build system Tests labels Jun 29, 2020

maflcko changed the title ~~net: Make DNS lookup mockable/testable/fuzzable. Add fuzzing harness for Lookup(…)/LookupHost(…)/LookupNumeric(…)/LookupSubNet(…).~~ net: Make DNS lookup mockable, add fuzzing harness Jun 29, 2020

practicalswift force-pushed the fuzzers-dns-lookup branch from 0f3c9dc to 3fac214 Compare June 30, 2020 06:25

DrahtBot added the Needs rebase label Jul 9, 2020

practicalswift force-pushed the fuzzers-dns-lookup branch from 3fac214 to 836ff14 Compare July 9, 2020 21:10

DrahtBot removed the Needs rebase label Jul 9, 2020

practicalswift force-pushed the fuzzers-dns-lookup branch from 836ff14 to 60a8427 Compare July 10, 2020 20:54

DrahtBot added the Needs rebase label Jul 11, 2020

practicalswift force-pushed the fuzzers-dns-lookup branch from 60a8427 to 426617a Compare July 11, 2020 18:05

DrahtBot removed the Needs rebase label Jul 11, 2020

DrahtBot mentioned this pull request Jul 30, 2020

net: change CNetAddr::ip to have flexible size #19628

Merged

Crypt-iQ approved these changes Aug 3, 2020

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Aug 25, 2020

practicalswift force-pushed the fuzzers-dns-lookup branch from 426617a to 954fbb7 Compare August 25, 2020 19:03

DrahtBot removed the Needs rebase label Feb 15, 2021

vasild mentioned this pull request Feb 19, 2021

Add inputs for a newly added test netbase_dns_lookup bitcoin-core/qa-assets#50

Merged

vasild approved these changes Feb 19, 2021

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

src/netbase.cpp Outdated Show resolved Hide resolved

src/netbase.cpp Outdated Show resolved Hide resolved

src/netbase.cpp Outdated Show resolved Hide resolved

src/test/fuzz/netbase_dns_lookup.cpp Show resolved Hide resolved

vasild approved these changes Feb 19, 2021

View reviewed changes

src/netbase.h Show resolved Hide resolved

practicalswift force-pushed the fuzzers-dns-lookup branch from 522b3b4 to 24e3ce3 Compare February 22, 2021 15:53

vasild approved these changes Feb 22, 2021

View reviewed changes

DrahtBot mentioned this pull request Mar 2, 2021

net, refactor: pass uint16 CService::port as uint16 #21328

Merged

DrahtBot added the Needs rebase label Mar 3, 2021

practicalswift force-pushed the fuzzers-dns-lookup branch from 24e3ce3 to 173272b Compare March 3, 2021 15:16

DrahtBot removed the Needs rebase label Mar 3, 2021

Crypt-iQ reviewed Mar 7, 2021

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

src/test/fuzz/netbase_dns_lookup.cpp Show resolved Hide resolved

jonatack reviewed Mar 7, 2021

View reviewed changes

practicalswift force-pushed the fuzzers-dns-lookup branch 2 times, most recently from d29e3f8 to 77123b3 Compare March 8, 2021 23:01

practicalswift added 2 commits March 8, 2021 23:17

net: Make DNS lookup code testable

c6b4bfb

tests: Add fuzzing harness for Lookup(...)/LookupHost(...)/LookupNume…

e528075

…ric(...)/LookupSubNet(...)

practicalswift force-pushed the fuzzers-dns-lookup branch from 77123b3 to e528075 Compare March 8, 2021 23:18

vasild approved these changes Mar 15, 2021

View reviewed changes

maflcko merged commit eceb3f7 into bitcoin:master Mar 15, 2021

maflcko reviewed Mar 15, 2021

View reviewed changes

practicalswift mentioned this pull request Mar 15, 2021

fuzz: Implement fuzzed_dns_lookup_function as a lambda #21443

Merged

practicalswift deleted the fuzzers-dns-lookup branch April 10, 2021 19:44

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

net: Make DNS lookup mockable, add fuzzing harness #19415

net: Make DNS lookup mockable, add fuzzing harness #19415

Uh oh!

Conversation

practicalswift commented Jun 29, 2020

Uh oh!

DrahtBot commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

naumenkogs commented Jun 30, 2020

Uh oh!

Crypt-iQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Mar 6, 2021

Uh oh!

Crypt-iQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Mar 8, 2021

Uh oh!

Crypt-iQ commented Mar 14, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

practicalswift Mar 15, 2021 • 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!

jonatack commented Mar 15, 2021

Uh oh!

Uh oh!

DrahtBot commented Jun 30, 2020 •

edited

Loading

practicalswift Mar 15, 2021 •

edited

Loading