fuzz: split FuzzedSock interface and implementation

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):

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

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

review-only ACK d2e8d121eff98f9bbc3f15977bfb1dda9a27245a 📧

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review-only ACK d2e8d121eff98f9bbc3f15977bfb1dda9a27245a 📧
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUg1aQv/aeRus5hrRnP5M5V3tAv4J17uP4l0h7ApjymlEHRgJvNLlBTDAjOKdji/
 8u3CYy9mEuxrLn1tv5TGqBMXy1YQLrj2x1R0ExBp96LRalfRPVyTpocx/yOhokfj0
 9sgy5MyPOpNg4WbcxMU6z0Mli2mg0TBg9pii/b5+KTQg7SdnSsjMXkWYoBYIBvcLk
10g/Ra92zzNjYcUGYLM5zDbTu3oU/wbhV7M1H5xapm29heIhtJ3FfSGo5zlA5BynVO
11nMNjq9+I04e3qpPopH0wnsSaApnqlFqzRdUNADTMdkyB+tqolDxet3G4GRR0SPGL
128TR7BXBUqAKNfx37x2+4hM5cUnTh+eByyemrRXlyMwAGWzo6QBmINbmD6dbPLT/t
13KZUAiKPN7ppA0nsSBdmejAX37ZyVKLFjtC0Nd2XT6XNKXCaYA27lL7VNbQJ2wpOq
14fayqkcxeAre8F+5rFq7/oIWSU8ArIXM9ruWhXO6OOFB0MHgcN23CgQ58jfvGLIoM
156nO8IrZq
16=aNPu
17-----END PGP SIGNATURE-----

Timestamp of file with hash fbf1a942dcb87778554e8487c05545ff298d95af1816d6f90d53986f457f41d2 -

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 🍰

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 841905bb61e34faa97b34f4f0c97f7581092a988 🍰
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhL0Av/boScIgVINXDYMJ262ndgfw3HxUyP9JYZLZ/LFDjDhnuLCoBYqfnGvs63
 8u8DZKbJoFtxjyHkasgOMgWindvj9ae/qzTNkLSBoT8Z6oWoooRhmjawvWXCj2OhO
 9TxfqVpY9FtcpnXMHZUWN3nx8qbGeAkhdoQ8dB3V0tCa1N+74taaLB1GmgcaJzy/8
10Mftf7AymvV/lgWSvZjcIOv1JoXKqw2vEYEkDUoXJuVyiVeqZXzYPeT/ZAgFrlQed
11ui3KjNa8RT/VGa4+rQee6rl76oSUcjBBRiAw+nRGpi4J3cxu15yHoD/ETR0OVow3
12z0d//mhKHbn6Pyn4tqJ23ple4/JlFRgAuZBEBPYcfxVUYBfPNHgOiomoIKA5fLnK
13ByJcIa0hHsPVMy54bzXHMrPyDaFeXw/563/lPsJt9IgJ3o73B3hQ0PGbtvmZe3FH
14K4lSSuxOWBNdowECW0qMweSH36kzrB9KQPvF2ov+KGNbd5j8ErB8M5GtpLcMy4mq
15RBX26q3v
16=eXO6
17-----END PGP SIGNATURE-----

Timestamp of file with hash 3f87a5b288e93fbd4eb780ede4aca90eea4b994dd7a1d9c2be7101c3fbf5173f -

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 🎬

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 549c82ad3a34a885ecca37a5f04c36dfbaa95d17 only change is rebase 🎬
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjNUwv5AZxj+qkRP+IARG87hMf1liImC+MpuCNqfr2qE2faCtn9+lGilUiSYIMo
 8Tmf4R06QIevh+jlAAbXazLwB7eLrSrFX25dIgQ7f6ijCMXEl8ezcic58WY1K7gF4
 9OhWm6bjDHYOH6b70fIpAHIXxsZViIJD1N1COqoD999rvFkTkCkREQXIsk1LkHxAJ
10Cr45GFJ7zSo55WIxV8NSfvUFDftENjlgu0/YB4n6wraIaQ3sNiRe7n45eqVt5Zc2
11Z0lnE3+9JwbCZKpsNcJ9C5G24KcIxyc/hbB5qwP+uF58b2ANGry5KJjMEyGJ5TWp
12hWWxcjZQq1ZhaEoCOpapKKyTDPckTLfE4TfpJz5h3YbVzEXezT+nUskZoCHTc9b7
13Oldfw9BF7foMyhS83QOfjDd4POw7rtPaWT7PR0lRAKbXGHjy9PYHpocXoLlvXJPz
14asKm+tfedIyfIVO24aRPMnrW22KUFX9Hrd56lqeo21SVHXT22zyz+bcDyNSV8541
15zTOdMYVQ
16=0nmO
17-----END PGP SIGNATURE-----

Timestamp of file with hash fefa9f6ff7ab6d5c5d493fb447f3ba78a5d161e01bf1afb703b033184420ad98 -

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

fuzz: split FuzzedSock interface and implementation #21630

Conflicts