fuzz: set the output argument of FuzzedSock::Accept() #31316

pull vasild wants to merge 1 commits into bitcoin:master from vasild:fuzzedsock_accept changing 1 files +27 −0
  1. vasild commented at 1:25 pm on November 18, 2024: contributor

    FuzzedSock::Accept() properly returns a new socket, but it forgot to set the output argument addr, like accept(2) is expected to.

    This could lead to reading uninitialized data during testing when we read it, e.g. from CService::SetSockAddr() which reads the sa_family member.

    Set addr to a fuzzed IPv4 or IPv6 address.

  2. fuzz: set the output argument of FuzzedSock::Accept()
    `FuzzedSock::Accept()` properly returns a new socket, but it forgot to
    set the output argument `addr`, like `accept(2)` is expected to.
    
    This could lead to reading uninitialized data during testing when we
    read it, e.g. from `CService::SetSockAddr()` which reads the `sa_family`
    member.
    
    Set `addr` to a fuzzed IPv4 or IPv6 address.
    7e28f6d0dd
  3. DrahtBot commented at 1:25 pm on November 18, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31316.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

  4. DrahtBot added the label Tests on Nov 18, 2024
  5. vasild commented at 1:26 pm on November 18, 2024: contributor
    Found in another mocked socket implementation (not FuzzedSock) in https://github.com/Sjors/bitcoin/pull/50#issuecomment-2478468037, but FuzzedSock has the same problem.
  6. dergoegge commented at 1:18 pm on November 20, 2024: member
    Perhaps this makes more sense in combination with a test that uses it? Currently FuzzedSock::Accept seems entirely unused: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/test/fuzz/util/net.cpp.gcov.html
  7. brunoerg commented at 2:39 pm on November 21, 2024: contributor

    Perhaps this makes more sense in combination with a test that uses it? Currently FuzzedSock::Accept seems entirely unused: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/test/fuzz/util/net.cpp.gcov.html

    I agree. I was going to ask steps to reproduce then I noticed this is unused.

  8. vasild commented at 1:37 pm on November 22, 2024: contributor

    Such a test is in #28584 which adds a call to CConnman::SocketHandler() and CConnman::InitBinds() during fuzzing. CConnman::InitBinds() will add some listening sockets and from there:

    CConnman::SocketHandler() calls CConnman::SocketHandlerListening() calls CConnman::AcceptConnection() calls FuzzedSock::Accept().

  9. brunoerg commented at 1:32 pm on November 25, 2024: contributor
    So, it would be better to cherry-pick this into #28584?
  10. vasild commented at 11:07 am on November 26, 2024: contributor
    Hmm, to have it included in #28584 - yes, I should do that. But also to have it standalone here because this is fixing a bug in master and shouldn’t be tied to the fate of #28584.
  11. bitcoin deleted a comment on Nov 26, 2024
  12. vasild commented at 2:33 pm on November 28, 2024: contributor

    Included this in #28584.

    Leaving this open for an independent review/merge because it is smaller change and fixes a problem in master.


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-12-03 18:12 UTC

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