fuzz: split FuzzedSock interface and implementation #21630

vasild commented at 9:12 AM on April 7, 2021: member

split FuzzedSock interface and implementation
make FuzzedSock::Wait() sometimes simulate an occurred event
set errno from FuzzedSock::Wait() if it simulates a failure

(this is a followup from #21617)

fanquake added the label Tests on Apr 7, 2021

practicalswift commented at 9:29 AM on April 7, 2021: contributor

Concept ACK

Thanks for making src/test/fuzz/ an even better place!

MarcoFalke commented at 9:31 AM on April 7, 2021: member

Can the uninitialized read also happen in i2p? (The Wait return code is ignored here):

src/i2p.cpp=bool Session::Accept(Connection& conn)
src/i2p.cpp-{
src/i2p.cpp-    try {
src/i2p.cpp-        while (!*m_interrupt) {
src/i2p.cpp-            Sock::Event occurred;
src/i2p.cpp:            conn.sock->Wait(MAX_WAIT_FOR_IO, Sock::RECV, &occurred);
src/i2p.cpp-
src/i2p.cpp-            if ((occurred & Sock::RECV) == 0) {

MarcoFalke commented at 9:32 AM on April 7, 2021: member

review-only ACK d2e8d121eff98f9bbc3f15977bfb1dda9a27245a 📧

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash fbf1a942dcb87778554e8487c05545ff298d95af1816d6f90d53986f457f41d2 -

</details>

in src/test/fuzz/util.cpp:181 in d2e8d121ef outdated

 177 | +    constexpr std::array wait_errnos{
 178 | +        EBADF,
 179 | +        EINTR,
 180 | +        EINVAL,
 181 | +    };
 182 |      if (!m_fuzzed_data_provider.ConsumeBool()) {

MarcoFalke commented at 9:32 AM on April 7, 2021:

If you want you can remove the !, now that you are breaking the input format anyway

vasild commented at 9:43 AM on April 7, 2021:

Done.

vasild commented at 12:20 PM on April 7, 2021: member

Can the uninitialized read also happen in i2p? (The Wait return code is ignored here):

Right! Addressed in #21631.

practicalswift commented at 9:00 PM on April 7, 2021: contributor

Consider adding [[nodiscard]] to Wait: such an annotation would likely have saved us from the uninitialized read reported in #21617.

[[nodiscard]] is great for functions where an ignored return value is likely to be unintentional (like in this case!). I think we underuse [[nodiscard]] :)

MarcoFalke commented at 7:18 AM on April 8, 2021: member

review ACK 841905bb61e34faa97b34f4f0c97f7581092a988 🍰

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 3f87a5b288e93fbd4eb780ede4aca90eea4b994dd7a1d9c2be7101c3fbf5173f -

</details>

fanquake deleted a comment on Apr 8, 2021

vasild commented at 12:17 PM on April 9, 2021: member

@practicalswift, I will do [[nodiscard]] in a separate PR, once #21631 is merged.

in src/test/fuzz/util.cpp:13 in 841905bb61 outdated

   6 | @@ -7,15 +7,196 @@
   7 |  #include <util/rbf.h>
   8 |  #include <version.h>
   9 |  
  10 | -bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred ) const
  11 | +FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
  12 | +    : m_fuzzed_data_provider{fuzzed_data_provider}
  13 |  {
  14 | -    if (!m_fuzzed_data_provider.ConsumeBool()) {
  15 | +    m_socket = fuzzed_data_provider.ConsumeIntegral<SOCKET>();

practicalswift commented at 2:56 PM on April 9, 2021:

Not changed in this PR, but is there any scenario where we want m_socket != INVALID_SOCKET here?

Otherwise I'd prefer going with INVALID_SOCKET. Perhaps Sock::m_socket could be default initialized to INVALID_SOCKET. That should be a safe default.

vasild commented at 3:23 PM on April 9, 2021:

Normally we want the fuzzed socket's m_socket to be != INVALID_SOCKET, otherwise a code that does if (sock.Get() == INVALID_SOCKET) will "misbehave" when mocked.

practicalswift commented at 3:50 PM on April 9, 2021:

@vasild Oh, I didn't think about the possibility to peek at m_socket via Get(). But then I guess we want two cases: 1.) m_socket == INVALID_SOCKET, and 2.) m_socket != INVALID_SOCKET where m_socket is a very high number which is unlikely to coincide with a real opened file descriptor?

The current code allows m_socket to be set to low numbers such as 0, 1, 2, 3, etc which may be problematic :)

practicalswift commented at 3:56 PM on April 9, 2021:

Perhaps m_socket = fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET)?

vasild commented at 8:09 AM on April 12, 2021:

Right! And we can do even better:

add a method to check if the Sock object "owns" a socket, similar to std::unique_ptr::operator bool() so that callers don't do Get() == INVALID_SOCKET.
Add wrapper methods for getsockname(), setsockopt(), bind() and listen(). That way more code will be mockable, but more importantly - then nobody would need to call Sock::Get() anymore so the value of m_socket will remain "hidden" within the Sock class.

practicalswift commented at 9:09 AM on April 12, 2021:

@vasild Sounds good! Until we have that in place I think it makes sense to set m_socket to very high numbers here to avoid nasty surprises :) The thought of random reads/writes to existing open file descriptors is a bit scary even if only from fuzzing code.

vasild commented at 6:10 AM on April 15, 2021:

Low fd values avoided in #21677.

vasild commented at 3:19 PM on April 15, 2021:

Add wrapper methods for ...

Done in https://github.com/bitcoin/bitcoin/pull/21700

vasild commented at 1:53 PM on April 12, 2021: member

Sock methods flagged with [[nodiscard]] in #21659.

MarcoFalke referenced this in commit 1f50f0bb38 on Apr 13, 2021

sidhujag referenced this in commit 02aa08ea92 on Apr 13, 2021

DrahtBot commented at 11:59 AM on April 14, 2021: member

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Apr 15, 2021

style: remove extra white space 5198a02de4

fuzz: set errno from FuzzedSock::Wait() if it simulates a failure 0c90ff1429

fuzz: make FuzzedSock::Wait() sometimes simulate an occurred event 9668e43d8e

fuzz: split FuzzedSock interface and implementation

Move the `FuzzedSock`'s implementation from `src/test/fuzz/util.h` to
`src/test/fuzz/util.cpp`.

A separate interface and implementation make the code more readable for
consumers who don't need to (better not) know the implementation
details.

29ae1c13a5

fuzz: use ConsumeBool() instead of !ConsumeBool()

The former is shorter and ends up with a "random" bool anyway.

549c82ad3a

vasild force-pushed on Apr 15, 2021

vasild commented at 6:53 AM on April 15, 2021: member

841905bb6...549c82ad3: rebase due to conflicts

practicalswift commented at 7:12 AM on April 15, 2021: contributor

cr ACK 549c82ad3a34a885ecca37a5f04c36dfbaa95d17: patch looks correct and touches only src/test/fuzz/

DrahtBot removed the label Needs rebase on Apr 15, 2021

MarcoFalke commented at 8:46 AM on April 15, 2021: member

re-ACK 549c82ad3a34a885ecca37a5f04c36dfbaa95d17 only change is rebase 🎬

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash fefa9f6ff7ab6d5c5d493fb447f3ba78a5d161e01bf1afb703b033184420ad98 -

</details>

MarcoFalke merged this on Apr 15, 2021

MarcoFalke closed this on Apr 15, 2021

vasild deleted the branch on Apr 15, 2021

sidhujag referenced this in commit c0fea96357 on Apr 15, 2021

DrahtBot locked this on Aug 16, 2022