[tests] Add fuzz testing for BlockTransactions and BlockTransactionsRequest #10409

1. 
   practicalswift commented at 2:51 pm on May 16, 2017: contributor

   The BlockTransactions deserialization code is reachable with tainted data via ProcessMessage(…, "BLOCKTXN", vRecv [tainted], …).

   The same thing applies to BlockTransactionsRequest which is reachable via "GETBLOCKTXN".

2. fanquake added the label Tests on May 17, 2017
3. 
   sipa commented at 0:25 am on May 18, 2017: member

   utACK d36fe73294b57821a793cd270bb0b4365fcdba00
4. 
   TheBlueMatt commented at 8:15 pm on May 22, 2017: member

   Maybe add CBlockHeaderAndShortTxIDs while you’re at it?
5. 
   practicalswift commented at 9:40 am on May 23, 2017: contributor

   @TheBlueMatt Good point! Adding also CBlockHeaderAndShortTxIDs was my original intention, but when doing so I encountered a long chain of linking errors which I was unable to fully resolve.

   More specifically I didn’t manage to create a test_test_bitcoin_fuzzy_LDADD variable (in src/Makefile.test.include) which would allow test_bitcoin_fuzzy to be built cleanly with this patch applied:

    0diff --git a/src/test/test_bitcoin_fuzzy.cpp b/src/test/test_bitcoin_fuzzy.cpp
    1index 7b52ac9..173351f 100644
    2--- a/src/test/test_bitcoin_fuzzy.cpp
    3+++ b/src/test/test_bitcoin_fuzzy.cpp
    4@@ -48,6 +48,7 @@ enum TEST_ID {
    5     CTXOUTCOMPRESSOR_DESERIALIZE,
    6     BLOCKTRANSACTIONS_DESERIALIZE,
    7     BLOCKTRANSACTIONSREQUEST_DESERIALIZE,
    8+    CBLOCKHEADERANDSHORTTXIDS_DESERIALIZE,
    9     TEST_ID_END
   10 };
   11
   12@@ -273,6 +274,16 @@ int main(int argc, char **argv)
   13
   14             break;
   15         }
   16+        case CBLOCKHEADERANDSHORTTXIDS_DESERIALIZE:
   17+        {
   18+            try
   19+            {
   20+                CBlockHeaderAndShortTxIDs bhastids;
   21+                ds >> bhastids;
   22+            } catch (const std::ios_base::failure& e) {return 0;}
   23+
   24+            break;
   25+        }
   26         default:
   27             return 0;
   28     }
   

   Help with solving this would be appreciated!

6. practicalswift force-pushed on May 24, 2017
7. 
   practicalswift commented at 4:02 pm on May 24, 2017: contributor

   @TheBlueMatt I solved the linking issue. Fuzzing of CBlockHeaderAndShortTxIDs has been added as part of this PR! :-)
8. practicalswift renamed this:
   [tests] Add fuzz testing for BlockTransactions (BLOCKTXN) and BlockTransactionsRequest (GETBLOCKTXN) deserialization
   [tests] Add fuzz testing for BlockTransactions, BlockTransactionsRequest and CBlockHeaderAndShortTxIDs deserialization
   on May 24, 2017
9. practicalswift force-pushed on May 25, 2017
10. practicalswift renamed this:
    [tests] Add fuzz testing for BlockTransactions, BlockTransactionsRequest and CBlockHeaderAndShortTxIDs deserialization
    [tests] Add fuzz testing for BlockTransactions and BlockTransactionsRequest deserialization
    on May 25, 2017
11. 
    practicalswift commented at 10:17 am on May 25, 2017: contributor

    Reverted the CBlockHeaderAndShortTxIDs deserialization commit. Couldn’t create a proper test_test_bitcoin_fuzzy_LDADD in src/Makefile.test.include that would allow test_test_bitcoin_fuzzy to compile cleanly under all configuration options.
12. practicalswift force-pushed on May 25, 2017
13. practicalswift force-pushed on May 25, 2017
14. practicalswift renamed this:
    [tests] Add fuzz testing for BlockTransactions and BlockTransactionsRequest deserialization
    [tests] Add fuzz testing for BlockTransactions, BlockTransactionsRequest and CBlockHeaderAndShortTxIDs deserialization
    on May 25, 2017
15. 
    practicalswift commented at 3:52 pm on May 25, 2017: contributor

    Finally managed to make the version including fuzzing of CBlockHeaderAndShortTxIDs to compile cleanly under all configuration options.

    This PR should be ready for review now :-)

16. 
    practicalswift commented at 9:38 am on August 16, 2017: contributor

    Anyone willing to review? :-)
17. 
    practicalswift commented at 1:59 pm on October 5, 2017: contributor

    Friendly ping @TheBlueMatt or @sipa - is this one getting ready for merge? :-)
18. 
    practicalswift commented at 7:41 pm on October 5, 2017: contributor

    @laanwj Thanks for merging the libFuzzer support (#10440) today!

    This is a friendly ping about my only currently outstanding fuzzing PR.

    It adds fuzzing of the remaining deserialization code that is reachable with tainted data via ProcessMessage(…, "…", vRecv [tainted], …) but not currently covered by test_bitcoin_fuzzy. More specifically it adds fuzzing of the deserialization code for BlockTransactions, BlockTransactionsRequest and CBlockHeaderAndShortTxIDs.

    Do you think it might be getting ready for merge?

    When this PR is merged I’ll look into having command-line argument determine the type of fuzzer as suggested in #11045.

19. 
    TheBlueMatt commented at 10:14 pm on October 5, 2017: member

    Looks good, though does the significant extra linking cause issues? See comment at #11045 (comment) indicating that at least some fuzzers may get much slower just cause of the extra binary size.
20. practicalswift force-pushed on Oct 13, 2017
21. 
    practicalswift commented at 10:05 am on October 24, 2017: contributor

    Seems like I’m unable to create a correct Makefile.test.include for this. Anyone willing to help me resolve that issue? :-)

    If not I’ll probably remove the CBlockHeaderAndShortTxIDs part of this PR to avoid the linking dependency order nightmare :-)

22. [tests] Add fuzz testing for BlockTransactions and BlockTransactionsRequest fd3a2f3130
23. practicalswift force-pushed on Oct 25, 2017
24. practicalswift renamed this:
    [tests] Add fuzz testing for BlockTransactions, BlockTransactionsRequest and CBlockHeaderAndShortTxIDs deserialization
    [tests] Add fuzz testing for BlockTransactions and BlockTransactionsRequest
    on Oct 25, 2017
25. 
    practicalswift commented at 8:10 pm on October 25, 2017: contributor

    Changed PR to only cover BlockTransactions and BlockTransactionsRequest. Hence no extra linking.

    This one should be ready for merge :-)

26. 
    TheBlueMatt commented at 8:28 pm on October 27, 2017: member

    utACK fd3a2f3130ebd1d1001c5dff80c1ff026654b00d
27. 
    laanwj commented at 2:19 pm on October 28, 2017: member

    utACK fd3a2f3
28. laanwj merged this on Oct 28, 2017
29. laanwj closed this on Oct 28, 2017

    ---

30. laanwj referenced this in commit b5545d8df9 on Oct 28, 2017
31. PastaPastaPasta referenced this in commit 6626da440c on Dec 22, 2019
32. PastaPastaPasta referenced this in commit 7254b88d62 on Jan 2, 2020
33. PastaPastaPasta referenced this in commit 92ac6f6cc6 on Jan 4, 2020
34. PastaPastaPasta referenced this in commit 319fd5d712 on Jan 12, 2020
35. PastaPastaPasta referenced this in commit bf0ecb0649 on Jan 12, 2020
36. PastaPastaPasta referenced this in commit aa5d97d340 on Jan 12, 2020
37. PastaPastaPasta referenced this in commit c8f0d86b23 on Jan 12, 2020
38. PastaPastaPasta referenced this in commit f9bd655557 on Jan 12, 2020
39. PastaPastaPasta referenced this in commit ed20891d4d on Jan 12, 2020
40. PastaPastaPasta referenced this in commit 7cbf80b7a7 on Jan 16, 2020
41. PastaPastaPasta referenced this in commit 72d8f89c91 on Jan 22, 2020
42. PastaPastaPasta referenced this in commit b83d4fbd37 on Jan 29, 2020
43. PastaPastaPasta referenced this in commit f16d38d366 on Jan 29, 2020
44. ckti referenced this in commit e98ae2561c on Mar 28, 2021
45. practicalswift deleted the branch on Apr 10, 2021
46. gades referenced this in commit 10b8cd1717 on Feb 12, 2022
47. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift sipa TheBlueMatt laanwj

Labels
Tests