tests: Add fuzzing harness for DecodeHexTx(…) #17777

practicalswift commented at 8:11 PM on December 19, 2019: contributor

Add fuzzing harness for DecodeHexTx(…).

To test this PR:

$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
      --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/decode_tx
…

fanquake added the label Tests on Dec 19, 2019

tests: Add fuzzing harness for DecodeHexTx(...) bcad0144ef

build: Sort fuzzing harnesses to avoid future merge conflicts 3f95fb085e

practicalswift force-pushed on Dec 19, 2019

DrahtBot commented at 10:41 PM on December 19, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17926 (tests: Add key_io fuzzing harness. Fuzz additional functions in existing fuzzing harnesses. by practicalswift)
#17860 (fuzz: BIP 42, BIP 30, CVE-2018-17144 by MarcoFalke)
#17771 (tests: Add fuzzing harness for V1TransportDeserializer (P2P transport) by practicalswift)

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.

in src/Makefile.test.include:245 in 3f95fb085e

 241 |  
 242 | -test_fuzz_block_deserialize_SOURCES = $(FUZZ_SUITE) test/fuzz/deserialize.cpp
 243 | -test_fuzz_block_deserialize_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -DBLOCK_DESERIALIZE=1
 244 | -test_fuzz_block_deserialize_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 245 | -test_fuzz_block_deserialize_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
 246 | -test_fuzz_block_deserialize_LDADD = $(FUZZ_SUITE_LD_COMMON)

MarcoFalke commented at 11:37 PM on December 19, 2019:

not sure if it makes sense to reorder the whole file to "reduce merge conflicts". This causes a merge conflict ...

practicalswift commented at 6:23 AM on December 20, 2019:

That is intentional: it introduces one merge conflict now but avoids more or less all future merge conflicts by making it clear where to add new fuzzing harnesses (and thus minimises the risk of merge conflicts). It makes sense given that clarification?

Commit message:

build: Sort fuzzing harnesses to avoid future merge conflicts

paymog commented at 11:41 AM on December 21, 2019:

Should sorting these be done in a separate MR to keep this one more focused?

practicalswift commented at 5:28 PM on December 21, 2019:

@paymog It is discouraged to submit this kind of change as separate PR. Changes like these should if possible made when touching the files for other purposes like in this case :)

jonatack commented at 12:45 PM on December 24, 2019:

@paymog it's true that the refactor section of CONTRIBUTING.md seems to encourage making separate PRs, but in practice it seems to me that when the change is not really worth making its own PR and is trivial/non-controversial or made/requested by an regular contributor, making the change in a separate commit when touching relevant code can be a good option. That doesn't mean there won't be pushback on it ;)

paymog commented at 1:01 PM on December 24, 2019:

I guess there's a deeper philosphical question of what makes a change worth a separate PR. When I work professionally I try to keep changes as focused as possible - though of course I'm not perfect.

Regarding non-controversiality, it seems that @MarcoFalke and @promag find this a bit controversial 😉. I don't want to push any ideas here since I'm still new to the community and I'm not sure there's much to gain either way in this instance - this change is pretty small. Making a new PR has overhead but might slightly reduce the review load.

jonatack commented at 1:20 PM on December 24, 2019:

@paymog I was attempting to clarify the general case, not bikeshed. YMMV. I've found that there are reasons why things in this project might be somewhat different from one's professional work philosophy in other projects. Cheers.

paymog commented at 1:42 PM on December 24, 2019:

@jonatack I realize that my comment may not come across as I meant it. I didn't think you were trying to bikeshed and I'm not either - like I say I'm still new and learning how this community operates. I appreciate you trying to bring clarity. Hopefully I didn't offend you and if I did, I'm sorry. It wasn't my intention.

jonatack commented at 2:31 PM on December 24, 2019:

No worries, I'm trying to grok things too and I make a lot of mistakes while doing it.

paymog commented at 11:56 AM on December 21, 2019: none

Concept ACK

promag commented at 12:11 AM on December 23, 2019: member

Is 3f95fb085e73b5537dda6d7258bfdab72d695fa9 really necessary?

practicalswift commented at 7:26 AM on December 23, 2019: contributor

@promag I'm scratching my own itch with that commit: the current sort order is a mess which makes it unclear where to add a new fuzzer which in turn leads to unnecessary merge conflicts. 3f95fb0 solves that issue once and for all :)

jonatack commented at 1:07 PM on December 24, 2019: member

ACK bcad0144eff3192cb54f65fa7737be53e03f8b0f

bitcoin (pr/17777)$ src/test/fuzz/decode_tx
INFO: Seed: 2616529759
INFO: Loaded 1 modules   (17312 inline 8-bit counters): 17312 [0x55dc3e14ce40, 0x55dc3e1511e0), 
INFO: Loaded 1 PC tables (17312 PCs): 17312 [0x55dc3e1511e0,0x55dc3e194be0), 
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
[#2](/bitcoin-bitcoin/2/)	INITED cov: 536 ft: 537 corp: 1/1b lim: 4 exec/s: 0 rss: 41Mb
[#4](/bitcoin-bitcoin/4/)	NEW    cov: 542 ft: 714 corp: 2/3b lim: 4 exec/s: 0 rss: 41Mb L: 2/2 MS: 2 ChangeByte-InsertByte-
	NEW_FUNC[1/18]: 0x55dc3dfabdc0 in prevector<28u, unsigned char, unsigned int, int>::~prevector() /home/jon/projects/bitcoin/bitcoin/src/./prevector.h:456

I did not review 3f95fb085e73b5537dda6d7258bfdab72d695fa9

practicalswift commented at 6:40 PM on January 12, 2020: contributor

These commands can be useful to quickly verify that the commit 3f95fb085e73b5537dda6d7258bfdab72d695fa9 is just sorting lines:

# ignore all added lines which have a matching identical removed line
$ git diff 3f95fb085e73b5537dda6d7258bfdab72d695fa9~1..3f95fb085e73b5537dda6d7258bfdab72d695fa9 \
      | grep -E '^[+-][^+-]' | cut -b2-  | sort | uniq -c | grep -v '^ *2 '
$

jonatack commented at 7:18 PM on January 12, 2020: member

ACK 3f95fb0

laanwj referenced this in commit daae6403d8 on Jan 20, 2020

laanwj merged this on Jan 20, 2020

laanwj closed this on Jan 20, 2020

sidhujag referenced this in commit 4148037dd7 on Jan 24, 2020

sidhujag referenced this in commit 55bc8ea65e on Nov 10, 2020

practicalswift deleted the branch on Apr 10, 2021

kittywhiskers referenced this in commit 87f059ed52 on Feb 27, 2022

kittywhiskers referenced this in commit ed147eeb0d on Feb 27, 2022

kittywhiskers referenced this in commit 4808aff72d on Feb 28, 2022

kittywhiskers referenced this in commit f2d58b9b81 on Feb 28, 2022

kittywhiskers referenced this in commit 893de1e840 on Feb 28, 2022

kittywhiskers referenced this in commit a1e2a55efe on Mar 13, 2022

kittywhiskers referenced this in commit 347c0f75ee on Mar 24, 2022

DrahtBot locked this on Aug 16, 2022