fuzz: Avoid use of low file descriptor ids (which may be in use) in FuzzedSock #21677

1. 
   practicalswift commented at 8:59 am on April 14, 2021: contributor

   Avoid use of low file descriptor ids (which may be in use) in FuzzedSock.

   Context: https://github.com/bitcoin/bitcoin/pull/21630/files#r610694541

2. DrahtBot added the label Tests on Apr 14, 2021
3. 
   laanwj commented at 10:03 am on April 14, 2021: member

   Concept ACK but this seems a bit… arbitrary. If you want to be 100% certain to not step on an existing one, could you open a (real) file descriptor, connected to something like a pipe() or /dev/null etc, and use that? Or do you use the fact that it needs to be a non-allocated file descriptor somewhere?
4. 
   DrahtBot commented at 11:28 am on April 14, 2021: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #21630 (fuzz: split FuzzedSock interface and implementation by vasild)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

5. in src/test/fuzz/util.h:580 in 6464d10602 outdated

   576@@ -577,15 +577,15 @@ class FuzzedSock : public Sock
   577 public:
   578     explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider) : m_fuzzed_data_provider{fuzzed_data_provider}
   579     {
   580-          m_socket = fuzzed_data_provider.ConsumeIntegral<SOCKET>();
   581+          m_socket = fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET);
   

   ---

   ---

   

   vasild commented at 12:52 pm on April 14, 2021:

   Maybe this should receive similar treatment:

   https://github.com/bitcoin/bitcoin/blob/03ecceedf6f15d2062e95b4533c5cea092c4c696/src/test/util/net.h#L78-L83

   ---

   

   practicalswift commented at 10:24 pm on April 14, 2021:

   Done!
6. 
   vasild commented at 12:55 pm on April 14, 2021: member

   … could you open a (real) file descriptor, connected to …

   Right! It does not need to be connected to anything. Just reuse this:

   https://github.com/bitcoin/bitcoin/blob/03ecceedf6f15d2062e95b4533c5cea092c4c696/src/test/sock_tests.cpp#L30-L35

   plus a close() in the FuzzedSocket destructor.

7. 
   practicalswift commented at 10:20 pm on April 14, 2021: contributor

   Concept ACK but this seems a bit… arbitrary. If you want to be 100% certain to not step on an existing one, could you open a (real) file descriptor, connected to something like a pipe() or /dev/null etc, and use that? Or do you use the fact that it needs to be a non-allocated file descriptor somewhere?

   The nice thing about going with INVALID_SOCKET - 1 is that it doesn’t involve slowdowns due to syscalls, and that reading or writing from fd INVALID_SOCKET - 1 is guaranteed to fail (unless the fuzzing harness has four billion open file descriptors :)).

   Note that INVALID_SOCKET - 1 is 4 294 967 294, and that POSIX requires file descriptor id re-use on close.

   Note that this PR is only a short-term bug fix to avoid file descriptors stdin (0), stdout (1), stderr (2) and other likely to be used file descriptors while awaiting vasild’s suggested cleanup/refactoring.

8. Avoid use of low file descriptor ids (which may be in use) in FuzzedSock and StaticContentsSock 6262182b3f
9. practicalswift force-pushed on Apr 14, 2021
10. vasild approved
11. 
    vasild commented at 5:49 am on April 15, 2021: member

    ACK 6262182b3f1c9540291fb8de3bf7a785e7113c55
12. MarcoFalke merged this on Apr 15, 2021
13. MarcoFalke closed this on Apr 15, 2021

    ---

14. 
    vasild commented at 6:08 am on April 15, 2021: member

    The chosen constant INVALID_SOCKET - 1 is bigger than FD_SETSIZE, so those dummy sockets are not selectable:

    https://github.com/bitcoin/bitcoin/blob/2cd834e6c09dbbb676ecac4a36d8f0f56b4fe4b2/src/compat.h#L100-L106

    I think that’s ok. I am working on a change to convert IsSelectableSocket(bare SOCKET or sock.Get()) to sock.IsSelectable() so that the mocked sockets could override that.

15. 
    vasild commented at 3:18 pm on April 15, 2021: member

    #21700 reduces the usage of Sock::Get() to tests and CreateSockTCP() which is not to be mocked. It also changes IsSelectableSocket() to be a method of Sock() so that FuzzedSock can override it nicely.
16. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift laanwj DrahtBot vasild

Labels
Tests