Add regression fuzz harness for CVE-2017-18350. This fuzzing harness would have found CVE-2017-18350 within a minute of fuzzing :)
See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don’t forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.
Happy fuzzing :)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
40@@ -37,6 +41,98 @@ class proxyType
41 bool randomize_credentials;
42 };
43
44+struct timeval MillisToTimeval(int64_t nTimeout);
61+};
62+
63+// Represents a non-owned SOCKET.
64+//
65+// The underlying SOCKET will NOT be closed automatically on object destruction.
66+class SocketImpl : public Socket
It is used in netbase.cpp? :)
0$ git grep SocketImpl
1src/netbase.cpp: SocketImpl socket{hSocket};
2src/netbase.cpp: SocketImpl socket{hSocket};
3src/netbase.h:class SocketImpl : public Socket
4src/netbase.h: explicit SocketImpl(SOCKET socket) : m_socket{socket}
SocketImpl functions. Might be a bug with lcov perhaps.
ConnectThroughProxy which is not fuzzed :)
FuzzedSocket is the one that’s used. Is the idea that they be used together in some manner eventually?
SOCKET as a Socket instance: no further plans beyond that :)
469@@ -470,4 +470,63 @@ void ReadFromStream(FuzzedDataProvider& fuzzed_data_provider, Stream& stream) no
470 }
471 }
472
473+class FuzzedSocket : public Socket
474+{
475+ FuzzedDataProvider& fuzzed_data_provider;
m_fuzzed_data_provider so the reference in the constructor can just be fuzzed_data_provider?
501+ if (len > random_bytes.size()) {
502+ std::memset(buf + random_bytes.size(), 0, len - random_bytes.size());
503+ }
504+ return len;
505+ }
506+ return random_bytes.size();
random_bytes.size() < len since that condition is only checked if ConsumeBool returns true. Shouldn’t the correct number of bytes be returned if possible?
recv can return up to len bytes like the real recv, right? :)
Builds on Ubuntu. Fails to build on macOS with brew clang.
0Making all in src
1 CXX test/fuzz/addition_overflow-addition_overflow.o
2In file included from test/fuzz/addition_overflow.cpp:7:
3./test/fuzz/util.h:335:13: error: no matching function for call to 'AdditionOverflow'
4 if (AdditionOverflow((uint64_t)fuzzed_file->m_offset, random_bytes.size())) {
5 ^~~~~~~~~~~~~~~~
6./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('unsigned long long' vs. 'unsigned long')
7NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
8 ^
9./test/fuzz/util.h:346:13: error: no matching function for call to 'AdditionOverflow'
10 if (AdditionOverflow(fuzzed_file->m_offset, n)) {
11 ^~~~~~~~~~~~~~~~
12./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('long long' vs. 'long')
13NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
14 ^
152 errors generated.
16make[2]: *** [test/fuzz/addition_overflow-addition_overflow.o] Error 1
17make[1]: *** [all-recursive] Error 1
18make: *** [all-recursive] Error 1
507+ }
508+
509+#ifdef USE_POLL
510+ int poll(short events, int timeout_ms) override
511+ {
512+ return m_fuzzed_data_provider.ConsumeIntegralInRange<int>(-1, 1);
Calling this with -1 as the min parameter to ConsumeIntegralInRange will result in a very large range since it will be cast to uint64_t:
https://github.com/bitcoin/bitcoin/blob/48c1083632687a42ac603d4f241e70616a1d3815/src/test/fuzz/FuzzedDataProvider.h#L87
Maybe an alternative version of ConsumeIntegralInRange for just -1,0,1 can be used? Not your fault, it’s the header file’s fault.
513+ }
514+#endif
515+
516+ int select(bool watch_read, bool watch_write, int timeout_ms) override
517+ {
518+ return m_fuzzed_data_provider.ConsumeIntegralInRange<int>(-1, 1);
ACK 2d02f775fa36c59bca5d7ef1a0e702c1bc2de117
Code change looks good to me. I did not try to reintroduce the CVE, but I did read up about it. Ubuntu 18:
./configure --enable-fuzz --with-sanitizers=address,undefined,integer,fuzzer - no errorsmacOS 10.15.4:
./configure --enable-fuzz --with-sanitizers=address,fuzzer --disable-asm - no errors./configure --enable-fuzz --with-sanitizers=undefined,integer,fuzzer - one complaint (suppressed by #19713) 0/usr/local/opt/llvm/bin/../include/c++/v1/memory:1876:35: runtime error: implicit conversion from type 'char' of value -125 (8-bit, signed) to type 'unsigned char' changed the value to 131 (8-bit, unsigned)
1 [#0](/bitcoin-bitcoin/0/) 0x1072b4bd4 in void std::__1::allocator_traits<std::__1::allocator<unsigned char> >::construct<unsigned char, char const&>(std::__1::allocator<unsigned char>&, unsigned char*, char const&) memory:1595
2 [#1](/bitcoin-bitcoin/1/) 0x1072b49f9 in void std::__1::allocator_traits<std::__1::allocator<unsigned char> >::__construct_range_forward<std::__1::__wrap_iter<char const*>, unsigned char*>(std::__1::allocator<unsigned char>&, std::__1::__wrap_iter<char const*>, std::__1::__wrap_iter<char const*>, unsigned char*&) memory:1688
3 [#2](/bitcoin-bitcoin/2/) 0x1072b3598 in std::__1::enable_if<__is_cpp17_forward_iterator<std::__1::__wrap_iter<char const*> >::value, void>::type std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >::__construct_at_end<std::__1::__wrap_iter<char const*> >(std::__1::__wrap_iter<char const*>, std::__1::__wrap_iter<char const*>, unsigned long) vector:1076
4 [#3](/bitcoin-bitcoin/3/) 0x10728e2a2 in std::__1::enable_if<(__is_cpp17_forward_iterator<std::__1::__wrap_iter<char const*> >::value) && (is_constructible<unsigned char, std::__1::iterator_traits<std::__1::__wrap_iter<char const*> >::reference>::value), std::__1::__wrap_iter<unsigned char*> >::type std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >::insert<std::__1::__wrap_iter<char const*> >(std::__1::__wrap_iter<unsigned char const*>, std::__1::__wrap_iter<char const*>, std::__1::__wrap_iter<char const*>) vector:1996
5 [#4](/bitcoin-bitcoin/4/) 0x10728b18c in Socks5(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int, ProxyCredentials const*, Socket&) netbase.cpp:492
6 [#5](/bitcoin-bitcoin/5/) 0x10663a420 in test_one_input(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&) socks5.cpp:30
7 [#6](/bitcoin-bitcoin/6/) 0x107663b28 in LLVMFuzzerTestOneInput fuzz.cpp:45
8 [#7](/bitcoin-bitcoin/7/) 0x107862780 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:556
9 [#8](/bitcoin-bitcoin/8/) 0x107861ec5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) FuzzerLoop.cpp:470
10 [#9](/bitcoin-bitcoin/9/) 0x107863f76 in fuzzer::Fuzzer::MutateAndTestOne() FuzzerLoop.cpp:698
11 [#10](/bitcoin-bitcoin/10/) 0x107864be5 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:830
12 [#11](/bitcoin-bitcoin/11/) 0x107851d8d in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:829
13 [#12](/bitcoin-bitcoin/12/) 0x10787da62 in main FuzzerMain.cpp:19
14 [#13](/bitcoin-bitcoin/13/) 0x7fff73778cc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)
$(FUZZ_SUITE_LDFLAGS_COMMON).
@Crypt-iQ May I ask you to review and hopefully give your second ACK on this PR? :)
Friendly review beg :)
Concept NACK:s are totally fine too: that would allow me to move forward with other fuzzing stuff without having to keep this PR updated :)
@MarcoFalke Ah, got it! Do you know who would be an appropriate reviewer? :)
FWIW I think sipa wrote the Socks5(...) code originally in #1141.
Any review help welcome! Concept NACK is obviously fine too: I just want to move forward towards close (due to NACK or close due to merge) :)
Rebased. Now using the names MockableSocket, Socket and FuzzedSocket.
Socket and FuzzedSocket both implement MockableSocket.
The introduced src/test/fuzz/socks5 would have caught the CVE-2017-18350 buffer overflow (which I caught via code review rather than fuzzing) within literally seconds of fuzzing :)
524+ ssize_t send(const char* buf, size_t len, int flags) override
525+ {
526+ if (m_fuzzed_data_provider.ConsumeBool()) {
527+ return len;
528+ }
529+ return m_fuzzed_data_provider.ConsumeIntegralInRange<ssize_t>(-1, len);
Just a note: this could return 0 from send(). I am not sure if send(2) could ever return 0:
On success, these calls return the number of characters sent. On error, -1 is returned
anyway, I think it is ok to return 0 - to check that our code does not get bricked by that (in case send(2) actually returns 0).
errno if we are going to return an error (-1). Application reaction on error should differ depending on the value of errno (should retry on “temporary” errors).
529+ return m_fuzzed_data_provider.ConsumeIntegralInRange<ssize_t>(-1, len);
530+ }
531+
532+ ssize_t recv(char* buf, size_t len, int flags) override
533+ {
534+ if (buf == nullptr || len == 0 || m_fuzzed_data_provider.ConsumeBool()) {
buf is nullptr then len is 0 before this if and then remove buf == nullptr from here.
534+ if (buf == nullptr || len == 0 || m_fuzzed_data_provider.ConsumeBool()) {
535+ return m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
536+ }
537+ const std::vector<uint8_t> random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(len);
538+ if (random_bytes.empty()) {
539+ return m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
errno to some “random” value if we are going to return -1. Maybe the “random” value should have higher probablitity of being one of EAGAIN, EWOULDBLOCK, EINTR or EINPROGRESS.
539+ return m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
540+ }
541+ std::memcpy(buf, random_bytes.data(), random_bytes.size());
542+ if (m_fuzzed_data_provider.ConsumeBool()) {
543+ if (len > random_bytes.size()) {
544+ std::memset(buf + random_bytes.size(), 0, len - random_bytes.size());
recv(2) would never set the remaining buffer to 0x0 bytes.
This is intentional:
If m_fuzzed_data_provider.ConsumeBool() we pretend to read len bytes but we only consume random_bytes.size() from the fuzzing input and let the remaining bytes be 0x0.
That way we can satisfy a read of say 4096 bytes even if we only have say 20 bytes of input left: 20 bytes will be real data from the input and 4076 bytes will be 0x0 bytes.
Makes sense? :)
len. Sorry for the noise.
532+ ssize_t recv(char* buf, size_t len, int flags) override
533+ {
534+ if (buf == nullptr || len == 0 || m_fuzzed_data_provider.ConsumeBool()) {
535+ return m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
536+ }
537+ const std::vector<uint8_t> random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(len);
What is the likelihood of this returning less than len? The ConsumeBytes() comment says:
… If fewer than // |num_bytes| of data remain, returns a shorter std::vector containing all // of the data that’s left.
I think we should make it more likely that this fuzzed recv() returns a partial read because this is when the “interesting” things happen in the app code as it should retry.
85+ ssize_t recv(char* buf, size_t len, int flags) override
86+ {
87+ return ::recv(m_socket, buf, len, flags);
88+ }
89+
90+#ifdef USE_POLL
poll.h here, I think.
I am considering how to make a RAII socket class…
What about something like this: https://github.com/vasild/bitcoin/commit/bf3fa6a10a69f3d0dbd84188e3ba089a19248b20?
96+#endif
97+
98+ // Simplified select(2) interface: Wait until the socket becomes ready for
99+ // reading (if watch_read) and/or writing (if watch_write). Timeout after
100+ // timeout_ms milliseconds. See select(2) for return values.
101+ int select(bool watch_read, bool watch_write, int timeout_ms) override;
#else of USE_POLL?
I still like the concept of this very much, but I just wondered if there isn’t a general way to fuzz network applications that doesn’t require wrapping every BSD socketsI call. I mean it seem like something people would want to commonly do.
Easy fuzzing of networking code is unfortunately an unsolved problem generally AFAIK.
There have been many attempts to solve this by using LD_PRELOAD hacks as @vasild mentions. One notable example is preeny’s desock. My experience is that these LD_PRELOAD hacks are pretty fragile and while they might be good enough for short-term one-off fuzzing jobs (think targeted vulnerability research) they are not suitable for more long-term robustness testing needs like in our case.
The best “off the shelf” solution to this problem I’ve seen is what Honggfuzz is doing with its networking mode libhfnetdriver (see http://blog.swiecki.net/2018/01/fuzzing-tcp-servers.html). Unfortunately that solutions is limited to Honggfuzz and it is more suited for “macro level” networking code fuzzing than the more “micro level” networking code fuzzing this PR targets.
That said: Honggfuzz NetDriver is great too have in the toolbox too: I’ve tried documenting how to fuzz Bitcoin Core using Honggfuzz’s networking mode. See the unmerged PR #20380 (“doc: Add instructions on how to fuzz the P2P layer using Honggfuzz NetDriver”). Review welcome! :)
tl;dr – Mocked networking functions driven by FuzzedDataProvider as suggested in this PR is AFAICT the most robust and appropriate way to solve our low-level network fuzzing needs. That said it would be interesting to know if anyone has any alternative approach which is both robust and fuzzer engine agnostic: automated robustness testing of networking code is an active area of research :)
tl;dr – Mocked networking functions driven by FuzzedDataProvider as suggested in this PR is AFAICT the most robust and appropriate way to solve our low-level network fuzzing needs.
Sure. Fair enough. It’s just that I’m in principle not a fan of wrapping and abstracting basic operating system APIs such as BSD Sockets because anyone contributing to the code has to learn a special interface instead of being up to speed quickly with what they already know. It’s also another moving part that could have bugs itself.
That said, it seems this is useful for other things such as I2P support so that seems enough of a motivation to do it anyway. If we do it, it would be good to do so at a global level as #20788 does.
anyone contributing to the code has to learn a special interface instead of being up to speed quickly with what they already know
Indeed! This is why #20788 mimics the system interface:
0ssize_t ret = recv(socket, data, len, 0);
1// vs (same arguments, return type and semantic)
2ssize_t ret = sock.Recv(data, len, 0);
getaddrinfo (#19415). I think introducing LD_PRELOAD into fuzz builds quickly becomes complicated with more fuzz tests - where do these shared objects go and who maintains them? The tradeoff is more mocking code, so I favor the approach in this PR.
10@@ -11,6 +11,7 @@
11
12 #include <compat.h>
13 #include <netaddress.h>
14+#include <protocol.h>
37@@ -37,6 +38,79 @@ class proxyType
38 bool randomize_credentials;
39 };
40
41+/**
42+ * Convert milliseconds to a struct timeval for e.g. select.
43+ */
44+struct timeval MillisToTimeval(int64_t nTimeout);
647+ {
648+ return m_fuzzed_data_provider.ConsumeBool();
649+ }
650+};
651+
652+FuzzedSocket ConsumeSocket(FuzzedDataProvider& fuzzed_data_provider)
249@@ -250,6 +250,24 @@ template <class T>
250 return false;
251 }
252
253+/**
254+ * Returns a copy of the value selected from the given vector `v`.
255+ */
256+template <typename T>
257+[[nodiscard]] T PickValue(FuzzedDataProvider& fuzzed_data_provider, const std::vector<T> v) noexcept
261+}
262+
263+/**
264+ * Sets errno to a value selected from the given vector `errnos`.
265+ */
266+inline void SetFuzzedErrNo(FuzzedDataProvider& fuzzed_data_provider, const std::vector<int> errnos) noexcept
std::array and PickValueInArray.35+ // will slow down fuzzing.
36+ SOCKS5_RECV_TIMEOUT = (fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) ? 1 : DEFAULT_SOCKS5_RECV_TIMEOUT;
37+ FuzzedSock fuzzed_sock = ConsumeSock(fuzzed_data_provider);
38+ // This Socks5(...) fuzzing harness would have caught CVE-2017-18350 within
39+ // a few seconds of fuzzing.
40+ (void)Socks5(fuzzed_data_provider.ConsumeRandomLengthString(512), fuzzed_data_provider.ConsumeIntegral<int>(), fuzzed_data_provider.ConsumeBool() ? &proxy_credentials : nullptr, fuzzed_sock);
, or ?, or :
632+ if (r == -1) {
633+ SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos);
634+ }
635+ return r;
636+ }
637+ const std::vector<uint8_t> random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(m_fuzzed_data_provider.ConsumeBool() ? len : m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, len));
?, or :
9+
10+#include <cstdint>
11+#include <string>
12+#include <vector>
13+
14+bool Socks5(const std::string& strDest, int port, const ProxyCredentials* auth, const Sock& socket);
nit: since Socks5() is not static now in netbase.cpp, I think it is more natural to put its declaration (prototype) in netbase.h and remove it from here.
Same for SOCKS5_RECV_TIMEOUT.
638+ return r;
639+ }
640+ std::memcpy(buf, random_bytes.data(), random_bytes.size());
641+ if (m_fuzzed_data_provider.ConsumeBool()) {
642+ if (len > random_bytes.size()) {
643+ std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size());
(char*) in memset() should not be needed. Or if it is needed (why?), then it should also be needed for the memcpy() call a few lines earlier since both take void* argument.
re: #19203 (review)
nit: the typecast
(char*)inmemset()should not be needed. Or if it is needed (why?), then it should also be needed for thememcpy()call a few lines earlier since both takevoid*argument.
I think think the compiler won’t allow arithmetic + random_bytes.size() on a void pointer.
643+ std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size());
644+ }
645+ return len;
646+ }
647+ if (m_fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) {
648+ std::this_thread::sleep_for(std::chrono::milliseconds{2});
SOCKS5_RECV_TIMEOUT can be lowered to 1 second for the tests or be the default 20 seconds.
In half of the cases this will sleep for 2 ms. So we need 500 Recv() calls where this sleep is executed in order to trigger a higher level timeout of 1 second.
I think this timeout will practically never be reached. Maybe change this to a few 100s of ms to make a timeout more likely?
re: #19203 (review)
SOCKS5_RECV_TIMEOUTcan be lowered to 1 second for the tests or be the default 20 seconds. In half of the cases this will sleep for 2 ms. So we need 500Recv()calls where this sleep is executed in order to trigger a higher level timeout of 1 second.I think this timeout will practically never be reached. Maybe change this to a few 100s of ms to make a timeout more likely?
SOCKS5_RECV_TIMEOUT looks like milliseconds not seconds so std::chrono::milliseconds{2} should always time out.
Also, I’m assuming this is fake-sleeping not really sleeping in the middle of a fuzz test?
You are right - it can be lowered to 1ms or be the default 20sec. If we want to always produce a timeout, maybe better to sleep for SOCKS5_RECV_TIMEOUT instead of std::chrono::milliseconds{2}?
Why would it be fake sleeping? My understanding is that it will actually sleep in the middle of the fuzz test.
Unfortunately the sleeping required to make the timeout trigger is not mockable: that’s the reason for making the ugly 2 ms real sleep opt-in via the environment variable FUZZED_SOCKET_FAKE_LATENCY. That environment is not assumed to be set except in special circumstances such as when targeted fuzzing is performed by security researchers and others who are willing to pay the 2 ms cost in order to get full coverage including the timeout logic.
Making the sleeping required to trigger the timeout mockable is likely out of scope for this PR (but would be nice to have!), so if the opt-in-to-timeout-via-FUZZED_SOCKET_FAKE_LATENCY workaround is deemed too ugly I could simply drop that commit from this PR, and we’ll get partial instead of full coverage :)
Let me know what you think and I’ll adjust accordingly :)
Running the new fuzz test for a few tens of minutes produces no errors.
Reverting the fix for CVE-2017–18350 like this:
0--- i/src/netbase.cpp
1+++ w/src/netbase.cpp
2@@ -533,13 +533,13 @@ bool Socks5(const std::string& strDest, int port, const ProxyCredentials* auth,
3 case SOCKS5Atyp::DOMAINNAME:
4 {
5 recvr = InterruptibleRecv(pchRet3, 1, SOCKS5_RECV_TIMEOUT, sock);
6 if (recvr != IntrRecvError::OK) {
7 return error("Error reading from proxy");
8 }
9- int nRecv = pchRet3[0];
10+ int nRecv = (char)pchRet3[0];
11 recvr = InterruptibleRecv(pchRet3, nRecv, SOCKS5_RECV_TIMEOUT, sock);
12 break;
13 }
14 default: return error("Error: malformed proxy response");
15 }
16 if (recvr != IntrRecvError::OK) {
crashes the test in less than a minute.
The PR description needs an update since it mentions 4 commits, but there are just 2 now.
Ah yes I had already updated the PR title for it, but you’re right that the description is out of date too.
572+ assert(false && "Not implemented yet.");
573+ }
574+
575+ ssize_t Send(const void* data, size_t len, int flags) const override
576+ {
577+ constexpr std::array send_errnos{
In commit “fuzz: Add fuzzing harness for Socks5(…)” (1d95ff9f731b0c5a317a9c3ce52e37b4a57e99ff)
It it better to make assumptions about return value and errno value with ConsumeIntegralInRange and PickValueInArray instead of just using ConsumeIntegral for these? ConsumeIntegral would definitely make the setup shorter and simpler and seem to make fuzzing more robust. Would it cause fuzzing to be too slow?
I think we want to mimic a conforming OS send() because our code is not prepared to handle a misbehaving send(), e.g.
-1 and does not set errno or-1 and sets errno to 2379392 or-9287 orlen.627+ return r;
628+ }
629+ const std::vector<uint8_t> random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(
630+ m_fuzzed_data_provider.ConsumeBool() ?
631+ len :
632+ m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, len));
In commit “fuzz: Add fuzzing harness for Socks5(…)” (1d95ff9f731b0c5a317a9c3ce52e37b4a57e99ff)
Again I don’t understand why need to bias the choice here instead of just choosing uniformly in the range. I guess I don’t understand enough about fuzzing but it seems annoying if you have to hold the fuzzer’s hand this way.
Good question!
My intuition was that the fuzzer would be helped by being able to distinguish between the expected “success case” (enough to read) and the unexpected “fail” case ((likely) not enough to read) on a single byte input basis. In other words: a single byte input chooses between the “fail path” and the “success path” via ConsumeBool.
After some tinkering/measuring it seems like that effort to help the fuzzer didn’t make any significant difference.
Now skipping the ConsumeBool(…) ? len : ConsumeIntegralInRange(…) in favor of a straight ConsumeIntegralInRange(…) as suggested. Please re-review :)
40@@ -41,7 +41,7 @@ int nConnectTimeout = DEFAULT_CONNECT_TIMEOUT;
41 bool fNameLookup = DEFAULT_NAME_LOOKUP;
42
43 // Need ample time for negotiation for very slow proxies such as Tor (milliseconds)
44-static const int SOCKS5_RECV_TIMEOUT = 20 * 1000;
45+int SOCKS5_RECV_TIMEOUT = 20 * 1000;
In commit “fuzz: Add FUZZED_SOCKET_FAKE_LATENCY mode to FuzzedSock to allow for fuzzing timeout logic” (88af03002c170d95dc04077f3bbcd739e2a9e5b7)
It seems dangerous to disguise a variable as an all caps constant. Best thing would be change this to a SOCKS5_RECV_TIMEOUT_DEFAULT default timeout, and add a runtime parameter to override the default. If that’s too complicated, next best thing would be to s/SOCKS5_RECV_TIMEOUT/g_socks5_recv_timeout/
b62b90fa0e642c89a75afcfeb7f0491fc8e93f00 looks good except that the first commit does not compile:
0test/fuzz/socks5.cpp:23:5: error: use of undeclared identifier 'default_socks5_recv_timeout'
1 default_socks5_recv_timeout = g_socks5_recv_timeout;
2 ^
3test/fuzz/socks5.cpp:23:35: error: use of undeclared identifier 'g_socks5_recv_timeout'
4 default_socks5_recv_timeout = g_socks5_recv_timeout;
5 ^
(the second commit fixes this)