fuzz: Fix uninitialized read in i2p test #21617

maflcko · 2021-04-06T10:45:31Z

Can be tested with:

./test/fuzz/test_runner.py -l DEBUG --valgrind ../btc_qa_assets/fuzz_seed_corpus/ i2p

==22582== Conditional jump or move depends on uninitialised value(s)
==22582==    at 0x6BB2D8: __sanitizer_cov_trace_const_cmp1 (in /tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz)
==22582==    by 0xB305DB: ConnectSocketDirectly(CService const&, Sock const&, int, bool) (netbase.cpp:570)
==22582==    by 0x8AAA5D: i2p::sam::Session::Hello() const (i2p.cpp:284)
==22582==    by 0x8A6FA0: i2p::sam::Session::CreateIfNotCreatedAlready() (i2p.cpp:352)
==22582==    by 0x8A6742: i2p::sam::Session::Listen(i2p::Connection&) (i2p.cpp:134)
==22582==    by 0x7A6C42: i2p_fuzz_target(Span<unsigned char const>) (i2p.cpp:37)

jonatack · 2021-04-06T10:53:51Z

Concept ACK

sipa · 2021-04-06T21:56:36Z

utACK 3333375

vasild · 2021-04-07T07:52:03Z

src/test/fuzz/util.cpp

@@ -7,6 +7,14 @@
 #include <util/rbf.h>
 #include <version.h>

+bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred ) const


nit: extra white space near the end: occurred )

Good catch. Someone forgot to run clang-format

vasild · 2021-04-07T08:02:29Z

src/test/fuzz/util.cpp

+{
+    if (!m_fuzzed_data_provider.ConsumeBool()) {
+        return false;
+    }
+    if (occurred) *occurred = 0;
+    return true;
+}


This would signal a failure without setting errno (it was like this even before this PR, but better fix it)

Would result in either an error or timeout, but never "the requested event occurred"

!ConsumeBool() is the same as ConsumeBool()

Suggested change

{

if (!m_fuzzed_data_provider.ConsumeBool()) {

return false;

}

if (occurred) *occurred = 0;

return true;

}

{

constexpr std::array wait_errnos{

EBADF,

EINTR,

EINVAL,

};

if (m_fuzzed_data_provider.ConsumeBool()) {

SetFuzzedErrNo(m_fuzzed_data_provider, wait_errnos);

return false;

}

if (occurred) {

if (m_fuzzed_data_provider.ConsumeBool()) {

*occurred = requested;

} else {

*occurred = 0;

}

}

return true;

}

!ConsumeBool() is the same as ConsumeBool()

I had a slight preference to not invalidate the existing fuzz inputs by using the ! operator on the bool

Mind taking the other changes to a new pr?

vasild · 2021-04-07T08:05:25Z

src/test/fuzz/util.h

-    {
-        return m_fuzzed_data_provider.ConsumeBool();
-    }
+    bool Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred = nullptr) const override;


I like separating the interface from the implementation - it makes it more readable for consumers who want to use it without being bothered with implementation details (actually it is better to not see the implementation so that one does not accidentally start relying on undocumented implementation details).

Here is an extra commit that moves the rest of the FuzzedSock's implementation to util.cpp:

vasild@33c203d

fuzz: split FuzzedSock interface and implementation

commit 33c203d6878d9b6adee046f36311c58a13e79941 (HEAD -> pull/21617_1617705813_33333755f__2104-fuzzValgrind, vasild/2104-fuzzValgrind) Parent: 33333755f2edcbe88fcd136f6fef81f94819002e Author: Vasil Dimov <vd@FreeBSD.org> AuthorDate: Wed Apr 7 10:18:39 2021 +0200 Commit: Vasil Dimov <vd@FreeBSD.org> CommitDate: Wed Apr 7 10:18:39 2021 +0200 gpg: Signature made Wed Apr 7 10:18:59 2021 CEST gpg: using RSA key E64D8D45614DB07545D9CCC154DF06F64B55CBBF gpg: Good signature from "Vasil Dimov <vd@myforest.net>" [ultimate] gpg: aka "Vasil Dimov <vd@FreeBSD.org>" [ultimate] gpg: aka "Vasil Dimov <vasild@gmail.com>" [ultimate] fuzz: split FuzzedSock interface and implementation diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index cf5244e31..2ab227e29 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -4,21 +4,194 @@ #include <test/fuzz/util.h> #include <test/util/script.h> #include <util/rbf.h> #include <version.h> +FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider) + : m_fuzzed_data_provider{fuzzed_data_provider} +{ + m_socket = fuzzed_data_provider.ConsumeIntegral<SOCKET>(); +} + +FuzzedSock::~FuzzedSock() +{ + // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call + // Sock::Reset() (not FuzzedSock::Reset()!) which will call CloseSocket(m_socket). + // Avoid closing an arbitrary file descriptor (m_socket is just a random number which + // may concide with a real opened file descriptor). + Reset(); +} + +FuzzedSock& FuzzedSock::operator=(Sock&& other) +{ + assert(false && "Move of Sock into FuzzedSock not allowed."); + return *this; +} + +void FuzzedSock::Reset() +{ + m_socket = INVALID_SOCKET; +} + +ssize_t FuzzedSock::Send(const void* data, size_t len, int flags) const +{ + constexpr std::array send_errnos{ + EACCES, + EAGAIN, + EALREADY, + EBADF, + ECONNRESET, + EDESTADDRREQ, + EFAULT, + EINTR, + EINVAL, + EISCONN, + EMSGSIZE, + ENOBUFS, + ENOMEM, + ENOTCONN, + ENOTSOCK, + EOPNOTSUPP, + EPIPE, + EWOULDBLOCK, + }; + if (m_fuzzed_data_provider.ConsumeBool()) { + return len; + } + const ssize_t r = m_fuzzed_data_provider.ConsumeIntegralInRange<ssize_t>(-1, len); + if (r == -1) { + SetFuzzedErrNo(m_fuzzed_data_provider, send_errnos); + } + return r; +} + +ssize_t FuzzedSock::Recv(void* buf, size_t len, int flags) const +{ + // Have a permanent error at recv_errnos[0] because when the fuzzed data is exhausted + // SetFuzzedErrNo() will always return the first element and we want to avoid Recv() + // returning -1 and setting errno to EAGAIN repeatedly. + constexpr std::array recv_errnos{ + ECONNREFUSED, + EAGAIN, + EBADF, + EFAULT, + EINTR, + EINVAL, + ENOMEM, + ENOTCONN, + ENOTSOCK, + EWOULDBLOCK, + }; + assert(buf != nullptr || len == 0); + if (len == 0 || m_fuzzed_data_provider.ConsumeBool()) { + const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; + if (r == -1) { + SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos); + } + return r; + } + std::vector<uint8_t> random_bytes; + bool pad_to_len_bytes{m_fuzzed_data_provider.ConsumeBool()}; + if (m_peek_data.has_value()) { + // `MSG_PEEK` was used in the preceding `Recv()` call, return `m_peek_data`. + random_bytes.assign({m_peek_data.value()}); + if ((flags & MSG_PEEK) == 0) { + m_peek_data.reset(); + } + pad_to_len_bytes = false; + } else if ((flags & MSG_PEEK) != 0) { + // New call with `MSG_PEEK`. + random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(1); + if (!random_bytes.empty()) { + m_peek_data = random_bytes[0]; + pad_to_len_bytes = false; + } + } else { + random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>( + m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, len)); + } + if (random_bytes.empty()) { + const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; + if (r == -1) { + SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos); + } + return r; + } + std::memcpy(buf, random_bytes.data(), random_bytes.size()); + if (pad_to_len_bytes) { + if (len > random_bytes.size()) { + std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size()); + } + return len; + } + if (m_fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) { + std::this_thread::sleep_for(std::chrono::milliseconds{2}); + } + return random_bytes.size(); +} + +int FuzzedSock::Connect(const sockaddr*, socklen_t) const +{ + // Have a permanent error at connect_errnos[0] because when the fuzzed data is exhausted + // SetFuzzedErrNo() will always return the first element and we want to avoid Connect() + // returning -1 and setting errno to EAGAIN repeatedly. + constexpr std::array connect_errnos{ + ECONNREFUSED, + EAGAIN, + ECONNRESET, + EHOSTUNREACH, + EINPROGRESS, + EINTR, + ENETUNREACH, + ETIMEDOUT, + }; + if (m_fuzzed_data_provider.ConsumeBool()) { + SetFuzzedErrNo(m_fuzzed_data_provider, connect_errnos); + return -1; + } + return 0; +} + +int FuzzedSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const +{ + constexpr std::array getsockopt_errnos{ + ENOMEM, + ENOBUFS, + }; + if (m_fuzzed_data_provider.ConsumeBool()) { + SetFuzzedErrNo(m_fuzzed_data_provider, getsockopt_errnos); + return -1; + } + if (opt_val == nullptr) { + return 0; + } + std::memcpy(opt_val, + ConsumeFixedLengthByteVector(m_fuzzed_data_provider, *opt_len).data(), + *opt_len); + return 0; +} + bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred ) const { if (!m_fuzzed_data_provider.ConsumeBool()) { return false; } if (occurred) *occurred = 0; return true; } +bool FuzzedSock::IsConnected(std::string& errmsg) const +{ + if (m_fuzzed_data_provider.ConsumeBool()) { + return true; + } + errmsg = "disconnected at random by the fuzzer"; + return false; +} + void FillNode(FuzzedDataProvider& fuzzed_data_provider, CNode& node, bool init_version) noexcept { const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS); const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS); const int32_t version = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max()); const bool filter_txs = fuzzed_data_provider.ConsumeBool(); diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index adcdd7174..8f4f87fbd 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -572,185 +572,31 @@ class FuzzedSock : public Sock * If `MSG_PEEK` is used, then our `Recv()` returns some random data as usual, but on the next * `Recv()` call we must return the same data, thus we remember it here. */ mutable std::optional<uint8_t> m_peek_data; public: - explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider) : m_fuzzed_data_provider{fuzzed_data_provider} - { - m_socket = fuzzed_data_provider.ConsumeIntegral<SOCKET>(); - } + explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider); - ~FuzzedSock() override - { - // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call - // Sock::Reset() (not FuzzedSock::Reset()!) which will call CloseSocket(m_socket). - // Avoid closing an arbitrary file descriptor (m_socket is just a random number which - // may concide with a real opened file descriptor). - Reset(); - } + ~FuzzedSock() override; - FuzzedSock& operator=(Sock&& other) override - { - assert(false && "Move of Sock into FuzzedSock not allowed."); - return *this; - } + FuzzedSock& operator=(Sock&& other) override; - void Reset() override - { - m_socket = INVALID_SOCKET; - } + void Reset() override; - ssize_t Send(const void* data, size_t len, int flags) const override - { - constexpr std::array send_errnos{ - EACCES, - EAGAIN, - EALREADY, - EBADF, - ECONNRESET, - EDESTADDRREQ, - EFAULT, - EINTR, - EINVAL, - EISCONN, - EMSGSIZE, - ENOBUFS, - ENOMEM, - ENOTCONN, - ENOTSOCK, - EOPNOTSUPP, - EPIPE, - EWOULDBLOCK, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - return len; - } - const ssize_t r = m_fuzzed_data_provider.ConsumeIntegralInRange<ssize_t>(-1, len); - if (r == -1) { - SetFuzzedErrNo(m_fuzzed_data_provider, send_errnos); - } - return r; - } + ssize_t Send(const void* data, size_t len, int flags) const override; - ssize_t Recv(void* buf, size_t len, int flags) const override - { - // Have a permanent error at recv_errnos[0] because when the fuzzed data is exhausted - // SetFuzzedErrNo() will always return the first element and we want to avoid Recv() - // returning -1 and setting errno to EAGAIN repeatedly. - constexpr std::array recv_errnos{ - ECONNREFUSED, - EAGAIN, - EBADF, - EFAULT, - EINTR, - EINVAL, - ENOMEM, - ENOTCONN, - ENOTSOCK, - EWOULDBLOCK, - }; - assert(buf != nullptr || len == 0); - if (len == 0 || m_fuzzed_data_provider.ConsumeBool()) { - const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; - if (r == -1) { - SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos); - } - return r; - } - std::vector<uint8_t> random_bytes; - bool pad_to_len_bytes{m_fuzzed_data_provider.ConsumeBool()}; - if (m_peek_data.has_value()) { - // `MSG_PEEK` was used in the preceding `Recv()` call, return `m_peek_data`. - random_bytes.assign({m_peek_data.value()}); - if ((flags & MSG_PEEK) == 0) { - m_peek_data.reset(); - } - pad_to_len_bytes = false; - } else if ((flags & MSG_PEEK) != 0) { - // New call with `MSG_PEEK`. - random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(1); - if (!random_bytes.empty()) { - m_peek_data = random_bytes[0]; - pad_to_len_bytes = false; - } - } else { - random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>( - m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, len)); - } - if (random_bytes.empty()) { - const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; - if (r == -1) { - SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos); - } - return r; - } - std::memcpy(buf, random_bytes.data(), random_bytes.size()); - if (pad_to_len_bytes) { - if (len > random_bytes.size()) { - std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size()); - } - return len; - } - if (m_fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) { - std::this_thread::sleep_for(std::chrono::milliseconds{2}); - } - return random_bytes.size(); - } + ssize_t Recv(void* buf, size_t len, int flags) const override; - int Connect(const sockaddr*, socklen_t) const override - { - // Have a permanent error at connect_errnos[0] because when the fuzzed data is exhausted - // SetFuzzedErrNo() will always return the first element and we want to avoid Connect() - // returning -1 and setting errno to EAGAIN repeatedly. - constexpr std::array connect_errnos{ - ECONNREFUSED, - EAGAIN, - ECONNRESET, - EHOSTUNREACH, - EINPROGRESS, - EINTR, - ENETUNREACH, - ETIMEDOUT, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, connect_errnos); - return -1; - } - return 0; - } + int Connect(const sockaddr*, socklen_t) const override; - int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override - { - constexpr std::array getsockopt_errnos{ - ENOMEM, - ENOBUFS, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, getsockopt_errnos); - return -1; - } - if (opt_val == nullptr) { - return 0; - } - std::memcpy(opt_val, - ConsumeFixedLengthByteVector(m_fuzzed_data_provider, *opt_len).data(), - *opt_len); - return 0; - } + int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override; bool Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred = nullptr) const override; - bool IsConnected(std::string& errmsg) const override - { - if (m_fuzzed_data_provider.ConsumeBool()) { - return true; - } - errmsg = "disconnected at random by the fuzzer"; - return false; - } + bool IsConnected(std::string& errmsg) const override; }; [[nodiscard]] inline FuzzedSock ConsumeSock(FuzzedDataProvider& fuzzed_data_provider) { return FuzzedSock{fuzzed_data_provider}; }

(feel free to ignore)

Concept ACK on the commit. Please open a new pr and ping me for review.

vasild

ACK 3333375

Even without taking the suggestions above, this would fix the uninitialized read - the main purpose of this PR.

maflcko · 2021-04-07T08:42:23Z

I merged this to unbreak our valgrind fuzzer. Happy to review the proposed changes in a follow-up pr.

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

Followup in #21630

3333375 fuzz: Fix uninitialized read in test (MarcoFalke) Pull request description: Can be tested with: ``` ./test/fuzz/test_runner.py -l DEBUG --valgrind ../btc_qa_assets/fuzz_seed_corpus/ i2p ``` ``` ==22582== Conditional jump or move depends on uninitialised value(s) ==22582== at 0x6BB2D8: __sanitizer_cov_trace_const_cmp1 (in /tmp/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz) ==22582== by 0xB305DB: ConnectSocketDirectly(CService const&, Sock const&, int, bool) (netbase.cpp:570) ==22582== by 0x8AAA5D: i2p::sam::Session::Hello() const (i2p.cpp:284) ==22582== by 0x8A6FA0: i2p::sam::Session::CreateIfNotCreatedAlready() (i2p.cpp:352) ==22582== by 0x8A6742: i2p::sam::Session::Listen(i2p::Connection&) (i2p.cpp:134) ==22582== by 0x7A6C42: i2p_fuzz_target(Span<unsigned char const>) (i2p.cpp:37) ACKs for top commit: sipa: utACK 3333375 vasild: ACK 3333375 Tree-SHA512: 36073582b26b541324b3e55f3fd4a44abf89cb3081f36d361525daf8c27602fbc25f736510ec30df7cb4ca0c4e395e8d8a60f531bf6af358b5a3e65dbabf72c0

549c82a fuzz: use ConsumeBool() instead of !ConsumeBool() (Vasil Dimov) 29ae1c1 fuzz: split FuzzedSock interface and implementation (Vasil Dimov) 9668e43 fuzz: make FuzzedSock::Wait() sometimes simulate an occurred event (Vasil Dimov) 0c90ff1 fuzz: set errno from FuzzedSock::Wait() if it simulates a failure (Vasil Dimov) 5198a02 style: remove extra white space (Vasil Dimov) Pull request description: * 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 bitcoin/bitcoin#21617) ACKs for top commit: practicalswift: cr ACK 549c82a: patch looks correct and touches only `src/test/fuzz/` MarcoFalke: re-ACK 549c82a only change is rebase 🎬 Tree-SHA512: 8ba965a8319074ad2ef840219c35c77e37cc79f00fb3926f20ccbf5f58e9616f5a3ac96434ad33996b47d292fa760d5d00a529001ac0d1d254262e5df93f616f

fuzz: Fix uninitialized read in test

3333375

maflcko changed the title ~~fuzz: Fix uninitialized read in test~~ fuzz: Fix uninitialized read in i2p test Apr 6, 2021

fanquake added the Tests label Apr 6, 2021

fanquake requested a review from vasild April 6, 2021 23:39

vasild reviewed Apr 7, 2021

View reviewed changes

vasild approved these changes Apr 7, 2021

View reviewed changes

maflcko merged commit 6154291 into bitcoin:master Apr 7, 2021

maflcko deleted the 2104-fuzzValgrind branch April 7, 2021 08:41

vasild mentioned this pull request Apr 7, 2021

fuzz: split FuzzedSock interface and implementation #21630

Merged

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: Fix uninitialized read in i2p test #21617

fuzz: Fix uninitialized read in i2p test #21617

maflcko commented Apr 6, 2021

jonatack commented Apr 6, 2021

sipa commented Apr 6, 2021

vasild Apr 7, 2021

maflcko Apr 7, 2021

vasild Apr 7, 2021

maflcko Apr 7, 2021

vasild Apr 7, 2021

maflcko Apr 7, 2021

vasild left a comment

maflcko commented Apr 7, 2021

vasild commented Apr 7, 2021

fuzz: Fix uninitialized read in i2p test #21617

fuzz: Fix uninitialized read in i2p test #21617

Conversation

maflcko commented Apr 6, 2021

jonatack commented Apr 6, 2021

sipa commented Apr 6, 2021

vasild Apr 7, 2021

Choose a reason for hiding this comment

maflcko Apr 7, 2021

Choose a reason for hiding this comment

vasild Apr 7, 2021

Choose a reason for hiding this comment

maflcko Apr 7, 2021

Choose a reason for hiding this comment

vasild Apr 7, 2021

Choose a reason for hiding this comment

maflcko Apr 7, 2021

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

maflcko commented Apr 7, 2021

vasild commented Apr 7, 2021