fuzz: split FuzzedSock interface and implementation #21630

vasild · 2021-04-07T09:12:16Z

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)

practicalswift · 2021-04-07T09:29:43Z

Concept ACK

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

maflcko · 2021-04-07T09:31:41Z

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

maflcko · 2021-04-07T09:32:17Z

review-only ACK d2e8d12 📧

Show signature and timestamp

Signature:

Timestamp of file with hash fbf1a942dcb87778554e8487c05545ff298d95af1816d6f90d53986f457f41d2 -

src/test/fuzz/util.cpp

vasild · 2021-04-07T12:20:50Z

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

Right! Addressed in #21631.

practicalswift · 2021-04-07T21:00:33Z

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

maflcko · 2021-04-08T07:18:22Z

review ACK 841905b 🍰

Show signature and timestamp

Signature:

Timestamp of file with hash 3f87a5b288e93fbd4eb780ede4aca90eea4b994dd7a1d9c2be7101c3fbf5173f -

vasild · 2021-04-09T12:17:41Z

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

practicalswift · 2021-04-09T14:56:18Z

src/test/fuzz/util.cpp

 {
-    if (!m_fuzzed_data_provider.ConsumeBool()) {
+    m_socket = fuzzed_data_provider.ConsumeIntegral<SOCKET>();


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.

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.

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

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

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.

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

Low fd values avoided in #21677.

Add wrapper methods for ...

Done in #21700

vasild · 2021-04-12T13:53:40Z

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

1c1467f i2p: cancel the Accept() method if waiting on the socket errors (Vasil Dimov) Pull request description: If `Sock::Wait()` fails, then cancel the `Accept()` method. Not checking the return value may cause an uninitialized read a few lines below when we read the `occurred` variable. [Spotted](bitcoin/bitcoin#21630 (comment)) by MarcoFalke, thanks! ACKs for top commit: laanwj: Code review ACK 1c1467f practicalswift: cr ACK 1c1467f: patch looks correct and agree with laanwj that `[[nodiscard]]` can be taken in a follow-up PR :) Tree-SHA512: 57fa8a03a4e055999e23121cd9ed1566a585ece0cf68b74223d8c902804cb6890218c9356d60e0560ccacc6c8542a526356c226ebd48e7b299b4572be312d49b

1c1467f i2p: cancel the Accept() method if waiting on the socket errors (Vasil Dimov) Pull request description: If `Sock::Wait()` fails, then cancel the `Accept()` method. Not checking the return value may cause an uninitialized read a few lines below when we read the `occurred` variable. [Spotted](bitcoin#21630 (comment)) by MarcoFalke, thanks! ACKs for top commit: laanwj: Code review ACK 1c1467f practicalswift: cr ACK 1c1467f: patch looks correct and agree with laanwj that `[[nodiscard]]` can be taken in a follow-up PR :) Tree-SHA512: 57fa8a03a4e055999e23121cd9ed1566a585ece0cf68b74223d8c902804cb6890218c9356d60e0560ccacc6c8542a526356c226ebd48e7b299b4572be312d49b

DrahtBot · 2021-04-14T11:59:16Z

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

Conflicts

No conflicts as of last run.

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.

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

vasild · 2021-04-15T06:53:23Z

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

practicalswift · 2021-04-15T07:12:22Z

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

maflcko · 2021-04-15T08:46:28Z

re-ACK 549c82a only change is rebase 🎬

Show signature and timestamp

Signature:

Timestamp of file with hash fefa9f6ff7ab6d5c5d493fb447f3ba78a5d161e01bf1afb703b033184420ad98 -

vasild mentioned this pull request Apr 7, 2021

fuzz: Fix uninitialized read in i2p test #21617

Merged

fanquake added the Tests label Apr 7, 2021

maflcko reviewed Apr 7, 2021

View reviewed changes

src/test/fuzz/util.cpp Outdated Show resolved Hide resolved

vasild mentioned this pull request Apr 7, 2021

i2p: always check the return value of Sock::Wait() #21631

Merged

bitcoin deleted a comment from xzavjierr Apr 8, 2021

bitcoin deleted a comment from Nadtasticle Apr 8, 2021

practicalswift reviewed Apr 9, 2021

View reviewed changes

DrahtBot mentioned this pull request Apr 14, 2021

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

Merged

DrahtBot added the Needs rebase label Apr 15, 2021

vasild added 5 commits April 15, 2021 08:19

style: remove extra white space

5198a02

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

0c90ff1

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

9668e43

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

549c82a

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

vasild force-pushed the FuzzedSock_move branch from 841905b to 549c82a Compare April 15, 2021 06:53

DrahtBot removed the Needs rebase label Apr 15, 2021

maflcko merged commit c6b30cc into bitcoin:master Apr 15, 2021

vasild deleted the FuzzedSock_move branch April 15, 2021 08:51

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 15, 2021

Merge bitcoin#21630: fuzz: split FuzzedSock interface and implementation

c0fea96

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fuzz: split FuzzedSock interface and implementation #21630

fuzz: split FuzzedSock interface and implementation #21630

vasild commented Apr 7, 2021

practicalswift commented Apr 7, 2021 •

edited

Loading

maflcko commented Apr 7, 2021

maflcko commented Apr 7, 2021

vasild commented Apr 7, 2021

practicalswift commented Apr 7, 2021 •

edited

Loading

maflcko commented Apr 8, 2021

vasild commented Apr 9, 2021

practicalswift Apr 9, 2021

vasild Apr 9, 2021

practicalswift Apr 9, 2021 •

edited

Loading

practicalswift Apr 9, 2021

vasild Apr 12, 2021

practicalswift Apr 12, 2021 •

edited

Loading

vasild Apr 15, 2021 •

edited

Loading

vasild Apr 15, 2021

vasild commented Apr 12, 2021

DrahtBot commented Apr 14, 2021 •

edited

Loading

vasild commented Apr 15, 2021

practicalswift commented Apr 15, 2021

maflcko commented Apr 15, 2021

fuzz: split FuzzedSock interface and implementation #21630

fuzz: split FuzzedSock interface and implementation #21630

Conversation

vasild commented Apr 7, 2021

practicalswift commented Apr 7, 2021 • edited Loading

maflcko commented Apr 7, 2021

maflcko commented Apr 7, 2021

vasild commented Apr 7, 2021

practicalswift commented Apr 7, 2021 • edited Loading

maflcko commented Apr 8, 2021

vasild commented Apr 9, 2021

practicalswift Apr 9, 2021

Choose a reason for hiding this comment

vasild Apr 9, 2021

Choose a reason for hiding this comment

practicalswift Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

practicalswift Apr 9, 2021

Choose a reason for hiding this comment

vasild Apr 12, 2021

Choose a reason for hiding this comment

practicalswift Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

vasild Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

vasild Apr 15, 2021

Choose a reason for hiding this comment

vasild commented Apr 12, 2021

DrahtBot commented Apr 14, 2021 • edited Loading

Conflicts

vasild commented Apr 15, 2021

practicalswift commented Apr 15, 2021

maflcko commented Apr 15, 2021

practicalswift commented Apr 7, 2021 •

edited

Loading

practicalswift commented Apr 7, 2021 •

edited

Loading

practicalswift Apr 9, 2021 •

edited

Loading

practicalswift Apr 12, 2021 •

edited

Loading

vasild Apr 15, 2021 •

edited

Loading

DrahtBot commented Apr 14, 2021 •

edited

Loading