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. A summary of reviews will appear here.

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)

Contributors
vasild DrahtBot dergoegge brunoerg

Labels
Tests