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

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. 
   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.

   Type	Reviewers
   ACK	jonatack

   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 Nov 18, 2024
4. 
   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.
5. 
   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
6. 
   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.

7. 
   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().

8. 
   brunoerg commented at 1:32 pm on November 25, 2024: contributor

   So, it would be better to cherry-pick this into #28584?
9. 
   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.
10. bitcoin deleted a comment on Nov 26, 2024
11. 
    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.

12. 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.

    83199523c9
13. vasild force-pushed on Dec 10, 2024
14. 
    vasild commented at 10:14 am on December 10, 2024: contributor

    7e28f6d0dd...83199523c9: copy less bytes from the returned vector if ConsumeBytes() returns less, ie don’t copy from past the end of the returned vector. It is ok to leave some of addr4->sin_addr during this copy because we have memset the entire to 0 earlier.

    Thanks to #28584 (comment)

15. fanquake requested review from marcofleon on Feb 21, 2025
16. 
    jonatack commented at 5:08 pm on June 2, 2025: member

    utACK 83199523c90591d57cd5046212c878a4d54d621d

    This commit was also pulled into #28584 as https://github.com/bitcoin/bitcoin/pull/28584/commits/cf0a49723bbbbb22d47ccb8f0e030a51e6757609 and is identical at the time of review.

17. DrahtBot added the label CI failed on Jun 15, 2025
18. DrahtBot removed the label CI failed on Jun 16, 2025
19. 
    vasild commented at 9:52 am on October 1, 2025: contributor

    Closing this because #28584 was merged which includes the commit in this PR.

    This PR: 83199523c90591d57cd5046212c878a4d54d621d #28584 contains: e883b37768812d96feec207a37202c7d1b603c1f

    0$ git range-diff e883b37768~..e883b37768 83199523c9~..83199523c9
    11:  e883b37768 = 1:  83199523c9 fuzz: set the output argument of FuzzedSock::Accept()
    2$
    

20. vasild closed this on Oct 1, 2025

    ---

Contributors
vasild DrahtBot dergoegge brunoerg jonatack

Review Requested
marcofleon

Labels
Tests