fuzz: add I2P harness #30230

pull marcofleon wants to merge 1 commits into bitcoin:master from marcofleon:2024/05/add-i2p-test changing 2 files +64 −0
  1. marcofleon commented at 1:41 pm on June 5, 2024: contributor

    Addresses #28803. This updated harness sets mock time at the beginning of each iteration and deletes the private key file at the end of each iteration. Mock time is used to make the fuzz test more stable, as GetTime is called at points in i2p. Deleting the private key file ensures that each iteration is independent from the last. Now, a new key is generated in i2p every time, so the fuzzer can eventually make progress through the target code.

    Re-working this harness also led me and dergoegge to resolve a couple of issues in FuzzedSock, which allows for full coverage of the i2p code. Those changes can be seen in #30211.

    The SAM protocol for interacting with I2P requires some specifc inputs so it’s best to use a dictionary when running this harness.

     0"HELLO VERSION"
     1"HELLO REPLY RESULT=OK VERSION="
     2"HELLO REPLY RESULT=NOVERSION"
     3"HELLO REPLY RESULT=I2P_ERROR"
     4"SESSION CREATE"
     5"SESSION STATUS RESULT=OK DESTINATION="
     6"SESSION STATUS RESULT=DUPLICATED_ID"
     7"SESSION STATUS RESULT=DUPLICATED_DEST"
     8"SESSION STATUS RESULT=INVALID_ID"
     9"SESSION STATUS RESULT=INVALID_KEY"
    10"SESSION STATUS RESULT=I2P_ERROR MESSAGE="
    11"SESSION ADD"
    12"SESSION REMOVE"
    13"STREAM CONNECT"
    14"STREAM STATUS RESULT=OK"
    15"STREAM STATUS RESULT=INVALID_ID"
    16"STREAM STATUS RESULT=INVALID_KEY"
    17"STREAM STATUS RESULT=CANT_REACH_PEER"
    18"STREAM STATUS RESULT=I2P_ERROR MESSAGE="
    19"STREAM ACCEPT"
    20"STREAM FORWARD"
    21"DATAGRAM SEND"
    22"RAW SEND"
    23"DEST GENERATE"
    24"DEST REPLY PUB= PRIV="
    25"DEST REPLY RESULT=I2P_ERROR"
    26"NAMING LOOKUP"
    27"NAMING REPLY RESULT=OK NAME= VALUE="
    28"DATAGRAM RECEIVED DESTINATION= SIZE="
    29"RAW RECEIVED SIZE="
    30"NAMING REPLY RESULT=INVALID_KEY NAME="
    31"NAMING REPLY RESULT=KEY_NOT_FOUND NAME="
    32"MIN"
    33"MAX"
    34"STYLE"
    35"ID"
    36"SILENT"
    37"DESTINATION"
    38"NAME"
    39"SIGNATURE_TYPE"
    40"CRYPTO_TYPE"
    41"SIZE"
    42"HOST"
    43"PORT"
    44"FROM_PORT"
    45"TRANSIENT"
    46"STREAM"
    47"DATAGRAM"
    48"RAW"
    49"MASTER"
    50"true"
    51"false"
    

    I’ll add this dict to qa-assets later on.

  2. DrahtBot commented at 1:41 pm on June 5, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK dergoegge, brunoerg, vasild

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Tests on Jun 5, 2024
  4. in src/test/fuzz/i2p.cpp:3 in b9a754f24c outdated
    0@@ -0,0 +1,64 @@
    1+// Copyright (c) 2020-2022 The Bitcoin Core developers
    2+// Distributed under the MIT software license, see the accompanying
    3+// // file COPYING or http://www.opensource.org/licenses/mit-license.php.
    


    dergoegge commented at 1:47 pm on June 5, 2024:
    0// Copyright (c) The Bitcoin Core developers
    1// Distributed under the MIT software license, see the accompanying
    2// file COPYING or http://www.opensource.org/licenses/mit-license.php.
    

    marcofleon commented at 8:09 pm on June 6, 2024:
    Good catch, thanks. Fixed.
  5. in src/test/fuzz/i2p.cpp:54 in b9a754f24c outdated
    49+    }
    50+
    51+    const CService to;
    52+    bool proxy_error;
    53+
    54+    if (session.Connect(to, conn, proxy_error)) {
    


    brunoerg commented at 3:00 pm on June 5, 2024:
    Why is it calling Connect without specifying to?

    marcofleon commented at 7:52 pm on June 6, 2024:
    SAM 3.1 doesn’t use ports so there’s a check in Connect that to.GetPort() equals 0. For the purposes of this fuzz test it doesn’t seem to really matter whether to is specified. But I agree it’s not best practice to have it be uninitialized. I think changing this to if (session.Connect(CService{}, conn, proxy_error)) and getting rid of the declared variable is probably better.
  6. DrahtBot added the label CI failed on Jun 5, 2024
  7. DrahtBot commented at 3:17 pm on June 5, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/25842896976

  8. DrahtBot removed the label CI failed on Jun 5, 2024
  9. fuzz: add I2P harness 193c748e44
  10. marcofleon force-pushed on Jun 6, 2024
  11. dergoegge approved
  12. dergoegge commented at 8:40 am on June 7, 2024: member

    tACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49

    Thanks for picking this up!

    I fuzzed the i2p harrness for a few hundred CPU hours and even without the dictionary it reaches almost everything. The dictionary will still help though, so adding it to qa-assets later would be good.

    My only note: I think it would be worthwhile to point out in the PR description what changes you have made to the original harness and why they were necessary.

  13. marcofleon commented at 4:21 pm on June 9, 2024: contributor

    My only note: I think it would be worthwhile to point out in the PR description what changes you have made to the original harness and why they were necessary.

    Done. Thanks for all the help @dergoegge.

  14. in src/test/fuzz/i2p.cpp:26 in 193c748e44
    21+
    22+FUZZ_TARGET(i2p, .init = initialize_i2p)
    23+{
    24+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
    25+
    26+    SetMockTime(ConsumeTime(fuzzed_data_provider));
    


    vasild commented at 1:16 pm on June 10, 2024:

    Does this mean that the time will be frozen for the duration of the test?

    From the OP:

    GetTime is called at points in i2p

    Where is GetTime() called in i2p?


    brunoerg commented at 2:36 pm on June 11, 2024:

    Where is GetTime() called in i2p?

    It’s used in Sock (e.g. RecvUntilTerminator).


    marcofleon commented at 5:20 pm on June 11, 2024:

    Yes, GetTime() is also called in SendComplete.

    Does this mean that the time will be frozen for the duration of the test?

    I believe so, unless I’m missing something. For each iteration, g_mock_time will be set to an integer within the range specified in ConsumeTime.


    vasild commented at 7:28 am on June 12, 2024:

    The frozen time will defeat FUZZED_SOCKET_FAKE_LATENCY:

    https://github.com/bitcoin/bitcoin/blob/91e0beede2859dea987ba6db746e95dca0ceb024/src/test/fuzz/util/net.cpp#L229-L231

    RecvUntilTerminator() and SendComplete() will never timeout. Is this acceptable trade off in order to have the fuzz test “more stable”? If it is “unstable” without freezing the time, does that mean that some of those methods actually timed out, resulting in a wider coverage? In this aspect, it seems that stable = only covering the non-timeout-codepath = narrower coverage = worse.

    Would it make sense to use mock time, but advance it a little in each Recv() or Send() method?


    marcofleon commented at 12:45 pm on June 12, 2024:

    I think it’s an acceptable tradeoff, as using mock time makes the test more determinstic. I don’t think timeouts are too critical a part of the I2P code, and this harness as is reaches almost all of the code paths in i2p.

    But I see what you’re saying in that advancing time would be a more accurate simulation. Maybe we leave this as a potential follow up on FuzzedSock?


    vasild commented at 2:06 pm on June 12, 2024:

    I think it’s an acceptable tradeoff …

    Ok.

    … advancing time would be a more accurate simulation. Maybe we leave this as a potential follow up …

    Right, further changes in FuzzedSock itself are kind of out of scope for this PR which resurrects the previously deleted test.

    Thanks!

  15. in src/test/fuzz/i2p.cpp:39 in 193c748e44
    34+    const fs::path private_key_path = gArgs.GetDataDirNet() / "fuzzed_i2p_private_key";
    35+    const CService addr{in6_addr(IN6ADDR_LOOPBACK_INIT), 7656};
    36+    const Proxy sam_proxy{addr, false};
    37+    CThreadInterrupt interrupt;
    38+
    39+    i2p::sam::Session session{private_key_path, sam_proxy, &interrupt};
    


    vasild commented at 1:40 pm on June 10, 2024:

    Can be simplified:

    0    CThreadInterrupt interrupt;
    1
    2    i2p::sam::Session session{private_key_path, Proxy{CService{}, false}, &interrupt};
    

    marcofleon commented at 5:21 pm on June 11, 2024:

    This doesn’t seem to work when I test it. I think this results in an unspecifed IPv6 address, so in Hello(), the sock returned from m_control_host.Connect() is always invalid. https://github.com/bitcoin/bitcoin/blob/337f9d44c28b1d3563a0063a8805b418d506698d/src/netaddress.cpp#L425-L431

    Another option would be to use ConsumeService here instead of explicitly setting addr. I haven’t tested it yet but would probably work.


    vasild commented at 7:07 am on June 12, 2024:
    Alright, I did not realize this. (can resolve this)
  16. brunoerg approved
  17. brunoerg commented at 9:12 pm on June 11, 2024: contributor
    ACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
  18. vasild approved
  19. vasild commented at 2:03 pm on June 12, 2024: contributor
    ACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
  20. fanquake merged this on Jun 12, 2024
  21. fanquake closed this on Jun 12, 2024

  22. hebasto added the label Needs CMake port on Aug 22, 2024
  23. maflcko removed the label Needs CMake port on Aug 29, 2024
  24. marcofleon deleted the branch on Nov 19, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me