fuzz: Fix uninitialized read in i2p test #21617

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2104-fuzzValgrind changing 2 files +11 −5
  1. MarcoFalke commented at 10:45 am on April 6, 2021: member

    Can be tested with:

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

    0==22582== Conditional jump or move depends on uninitialised value(s)
1==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)
2==22582==    by 0xB305DB: ConnectSocketDirectly(CService const&, Sock const&, int, bool) (netbase.cpp:570)
3==22582==    by 0x8AAA5D: i2p::sam::Session::Hello() const (i2p.cpp:284)
4==22582==    by 0x8A6FA0: i2p::sam::Session::CreateIfNotCreatedAlready() (i2p.cpp:352)
5==22582==    by 0x8A6742: i2p::sam::Session::Listen(i2p::Connection&) (i2p.cpp:134)
6==22582==    by 0x7A6C42: i2p_fuzz_target(Span<unsigned char const>) (i2p.cpp:37)
  2. fuzz: Fix uninitialized read in test 33333755f2
  3. MarcoFalke renamed this:
    fuzz: Fix uninitialized read in test
    fuzz: Fix uninitialized read in i2p test
    on Apr 6, 2021
  4. fanquake added the label Tests on Apr 6, 2021
  5. jonatack commented at 10:53 am on April 6, 2021: member
    Concept ACK
  6. sipa commented at 9:56 pm on April 6, 2021: member
    utACK 33333755f2edcbe88fcd136f6fef81f94819002e
  7. fanquake requested review from vasild on Apr 6, 2021
  8. in src/test/fuzz/util.cpp:10 in 33333755f2 
     6@@ -7,6 +7,14 @@
 7 #include <util/rbf.h>
 8 #include <version.h>
 9 
10+bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred ) const
    vasild commented at 7:52 am on April 7, 2021:
    nit: extra white space near the end: occurred )
    MarcoFalke commented at 8:39 am on April 7, 2021:
    Good catch. Someone forgot to run clang-format
  9. in src/test/fuzz/util.cpp:17 in 33333755f2 
    12+    if (!m_fuzzed_data_provider.ConsumeBool()) {
13+        return false;
14+    }
15+    if (occurred) *occurred = 0;
16+    return true;
17+}
    vasild commented at 8:02 am on April 7, 2021:
    • 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()
     0{
 1    constexpr std::array wait_errnos{
 2        EBADF,
 3        EINTR,
 4        EINVAL,
 5    };
 6    if (m_fuzzed_data_provider.ConsumeBool()) { 
 7        SetFuzzedErrNo(m_fuzzed_data_provider, wait_errnos);
 8        return false;
 9    } 
10    if (occurred) { 
11        if (m_fuzzed_data_provider.ConsumeBool()) { 
12            *occurred = requested;
13        } else { 
14            *occurred = 0;
15        } 
16    } 
17    return true;
18}
    MarcoFalke commented at 8:41 am on April 7, 2021:

    !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?

  10. in src/test/fuzz/util.h:741 in 33333755f2 
    737@@ -738,12 +738,10 @@ class FuzzedSock : public Sock
738         return 0;
739     }
740 
741-    bool Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred = nullptr) const override
742-    {
743-        return m_fuzzed_data_provider.ConsumeBool();
744-    }
745+    bool Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred = nullptr) const override;
    vasild commented at 8:05 am on April 7, 2021:

    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:

    https://github.com/vasild/bitcoin/commit/33c203d6878d9b6adee046f36311c58a13e79941

      0commit 33c203d6878d9b6adee046f36311c58a13e79941 (HEAD -> pull/21617_1617705813_33333755f__2104-fuzzValgrind, vasild/2104-fuzzValgrind)
  1Parent: 33333755f2edcbe88fcd136f6fef81f94819002e
  2Author:     Vasil Dimov <vd@FreeBSD.org>
  3AuthorDate: Wed Apr 7 10:18:39 2021 +0200
  4Commit:     Vasil Dimov <vd@FreeBSD.org>
  5CommitDate: Wed Apr 7 10:18:39 2021 +0200
  6gpg: Signature made Wed Apr  7 10:18:59 2021 CEST
  7gpg:                using RSA key E64D8D45614DB07545D9CCC154DF06F64B55CBBF
  8gpg: Good signature from "Vasil Dimov <vd@myforest.net>" [ultimate]
  9gpg:                 aka "Vasil Dimov <vd@FreeBSD.org>" [ultimate]
 10gpg:                 aka "Vasil Dimov <vasild@gmail.com>" [ultimate]
 11
 12
 13    fuzz: split FuzzedSock interface and implementation
 14
 15diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
 16index cf5244e31..2ab227e29 100644
 17--- a/src/test/fuzz/util.cpp
 18+++ b/src/test/fuzz/util.cpp
 19@@ -4,21 +4,194 @@
 20 
 21 #include <test/fuzz/util.h>
 22 #include <test/util/script.h>
 23 #include <util/rbf.h>
 24 #include <version.h>
 25 
 26+FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
 27+    : m_fuzzed_data_provider{fuzzed_data_provider}
 28+{
 29+    m_socket = fuzzed_data_provider.ConsumeIntegral<SOCKET>();
 30+}
 31+
 32+FuzzedSock::~FuzzedSock()
 33+{
 34+    // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call
 35+    // Sock::Reset() (not FuzzedSock::Reset()!) which will call CloseSocket(m_socket).
 36+    // Avoid closing an arbitrary file descriptor (m_socket is just a random number which
 37+    // may concide with a real opened file descriptor).
 38+    Reset();
 39+}
 40+
 41+FuzzedSock& FuzzedSock::operator=(Sock&& other)
 42+{
 43+    assert(false && "Move of Sock into FuzzedSock not allowed.");
 44+    return *this;
 45+}
 46+
 47+void FuzzedSock::Reset()
 48+{
 49+    m_socket = INVALID_SOCKET;
 50+}
 51+
 52+ssize_t FuzzedSock::Send(const void* data, size_t len, int flags) const
 53+{
 54+    constexpr std::array send_errnos{
 55+        EACCES,
 56+        EAGAIN,
 57+        EALREADY,
 58+        EBADF,
 59+        ECONNRESET,
 60+        EDESTADDRREQ,
 61+        EFAULT,
 62+        EINTR,
 63+        EINVAL,
 64+        EISCONN,
 65+        EMSGSIZE,
 66+        ENOBUFS,
 67+        ENOMEM,
 68+        ENOTCONN,
 69+        ENOTSOCK,
 70+        EOPNOTSUPP,
 71+        EPIPE,
 72+        EWOULDBLOCK,
 73+    };
 74+    if (m_fuzzed_data_provider.ConsumeBool()) {
 75+        return len;
 76+    }
 77+    const ssize_t r = m_fuzzed_data_provider.ConsumeIntegralInRange<ssize_t>(-1, len);
 78+    if (r == -1) {
 79+        SetFuzzedErrNo(m_fuzzed_data_provider, send_errnos);
 80+    }
 81+    return r;
 82+}
 83+
 84+ssize_t FuzzedSock::Recv(void* buf, size_t len, int flags) const
 85+{
 86+    // Have a permanent error at recv_errnos[0] because when the fuzzed data is exhausted
 87+    // SetFuzzedErrNo() will always return the first element and we want to avoid Recv()
 88+    // returning -1 and setting errno to EAGAIN repeatedly.
 89+    constexpr std::array recv_errnos{
 90+        ECONNREFUSED,
 91+        EAGAIN,
 92+        EBADF,
 93+        EFAULT,
 94+        EINTR,
 95+        EINVAL,
 96+        ENOMEM,
 97+        ENOTCONN,
 98+        ENOTSOCK,
 99+        EWOULDBLOCK,
100+    };
101+    assert(buf != nullptr || len == 0);
102+    if (len == 0 || m_fuzzed_data_provider.ConsumeBool()) {
103+        const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
104+        if (r == -1) {
105+            SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos);
106+        }
107+        return r;
108+    }
109+    std::vector<uint8_t> random_bytes;
110+    bool pad_to_len_bytes{m_fuzzed_data_provider.ConsumeBool()};
111+    if (m_peek_data.has_value()) {
112+        // `MSG_PEEK` was used in the preceding `Recv()` call, return `m_peek_data`.
113+        random_bytes.assign({m_peek_data.value()});
114+        if ((flags & MSG_PEEK) == 0) {
115+            m_peek_data.reset();
116+        }
117+        pad_to_len_bytes = false;
118+    } else if ((flags & MSG_PEEK) != 0) {
119+        // New call with `MSG_PEEK`.
120+        random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(1);
121+        if (!random_bytes.empty()) {
122+            m_peek_data = random_bytes[0];
123+            pad_to_len_bytes = false;
124+        }
125+    } else {
126+        random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(
127+            m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, len));
128+    }
129+    if (random_bytes.empty()) {
130+        const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
131+        if (r == -1) {
132+            SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos);
133+        }
134+        return r;
135+    }
136+    std::memcpy(buf, random_bytes.data(), random_bytes.size());
137+    if (pad_to_len_bytes) {
138+        if (len > random_bytes.size()) {
139+            std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size());
140+        }
141+        return len;
142+    }
143+    if (m_fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) {
144+        std::this_thread::sleep_for(std::chrono::milliseconds{2});
145+    }
146+    return random_bytes.size();
147+}
148+
149+int FuzzedSock::Connect(const sockaddr*, socklen_t) const
150+{
151+    // Have a permanent error at connect_errnos[0] because when the fuzzed data is exhausted
152+    // SetFuzzedErrNo() will always return the first element and we want to avoid Connect()
153+    // returning -1 and setting errno to EAGAIN repeatedly.
154+    constexpr std::array connect_errnos{
155+        ECONNREFUSED,
156+        EAGAIN,
157+        ECONNRESET,
158+        EHOSTUNREACH,
159+        EINPROGRESS,
160+        EINTR,
161+        ENETUNREACH,
162+        ETIMEDOUT,
163+    };
164+    if (m_fuzzed_data_provider.ConsumeBool()) {
165+        SetFuzzedErrNo(m_fuzzed_data_provider, connect_errnos);
166+        return -1;
167+    }
168+    return 0;
169+}
170+
171+int FuzzedSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const
172+{
173+    constexpr std::array getsockopt_errnos{
174+        ENOMEM,
175+        ENOBUFS,
176+    };
177+    if (m_fuzzed_data_provider.ConsumeBool()) {
178+        SetFuzzedErrNo(m_fuzzed_data_provider, getsockopt_errnos);
179+        return -1;
180+    }
181+    if (opt_val == nullptr) {
182+        return 0;
183+    }
184+    std::memcpy(opt_val,
185+                ConsumeFixedLengthByteVector(m_fuzzed_data_provider, *opt_len).data(),
186+                *opt_len);
187+    return 0;
188+}
189+
190 bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred ) const
191 {
192     if (!m_fuzzed_data_provider.ConsumeBool()) {
193         return false;
194     }
195     if (occurred) *occurred = 0;
196     return true;
197 }
198 
199+bool FuzzedSock::IsConnected(std::string& errmsg) const
200+{
201+    if (m_fuzzed_data_provider.ConsumeBool()) {
202+        return true;
203+    }
204+    errmsg = "disconnected at random by the fuzzer";
205+    return false;
206+}
207+
208 void FillNode(FuzzedDataProvider& fuzzed_data_provider, CNode& node, bool init_version) noexcept
209 {
210     const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
211     const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
212     const int32_t version = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max());
213     const bool filter_txs = fuzzed_data_provider.ConsumeBool();
214diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
215index adcdd7174..8f4f87fbd 100644
216--- a/src/test/fuzz/util.h
217+++ b/src/test/fuzz/util.h
218@@ -572,185 +572,31 @@ class FuzzedSock : public Sock
219      * If `MSG_PEEK` is used, then our `Recv()` returns some random data as usual, but on the next
220      * `Recv()` call we must return the same data, thus we remember it here.
221      */
222     mutable std::optional<uint8_t> m_peek_data;
223 
224 public:
225-    explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider) : m_fuzzed_data_provider{fuzzed_data_provider}
226-    {
227-          m_socket = fuzzed_data_provider.ConsumeIntegral<SOCKET>();
228-    }
229+    explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider);
230 
231-    ~FuzzedSock() override
232-    {
233-        // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call
234-        // Sock::Reset() (not FuzzedSock::Reset()!) which will call CloseSocket(m_socket).
235-        // Avoid closing an arbitrary file descriptor (m_socket is just a random number which
236-        // may concide with a real opened file descriptor).
237-        Reset();
238-    }
239+    ~FuzzedSock() override;
240 
241-    FuzzedSock& operator=(Sock&& other) override
242-    {
243-        assert(false && "Move of Sock into FuzzedSock not allowed.");
244-        return *this;
245-    }
246+    FuzzedSock& operator=(Sock&& other) override;
247 
248-    void Reset() override
249-    {
250-        m_socket = INVALID_SOCKET;
251-    }
252+    void Reset() override;
253 
254-    ssize_t Send(const void* data, size_t len, int flags) const override
255-    {
256-        constexpr std::array send_errnos{
257-            EACCES,
258-            EAGAIN,
259-            EALREADY,
260-            EBADF,
261-            ECONNRESET,
262-            EDESTADDRREQ,
263-            EFAULT,
264-            EINTR,
265-            EINVAL,
266-            EISCONN,
267-            EMSGSIZE,
268-            ENOBUFS,
269-            ENOMEM,
270-            ENOTCONN,
271-            ENOTSOCK,
272-            EOPNOTSUPP,
273-            EPIPE,
274-            EWOULDBLOCK,
275-        };
276-        if (m_fuzzed_data_provider.ConsumeBool()) {
277-            return len;
278-        }
279-        const ssize_t r = m_fuzzed_data_provider.ConsumeIntegralInRange<ssize_t>(-1, len);
280-        if (r == -1) {
281-            SetFuzzedErrNo(m_fuzzed_data_provider, send_errnos);
282-        }
283-        return r;
284-    }
285+    ssize_t Send(const void* data, size_t len, int flags) const override;
286 
287-    ssize_t Recv(void* buf, size_t len, int flags) const override
288-    {
289-        // Have a permanent error at recv_errnos[0] because when the fuzzed data is exhausted
290-        // SetFuzzedErrNo() will always return the first element and we want to avoid Recv()
291-        // returning -1 and setting errno to EAGAIN repeatedly.
292-        constexpr std::array recv_errnos{
293-            ECONNREFUSED,
294-            EAGAIN,
295-            EBADF,
296-            EFAULT,
297-            EINTR,
298-            EINVAL,
299-            ENOMEM,
300-            ENOTCONN,
301-            ENOTSOCK,
302-            EWOULDBLOCK,
303-        };
304-        assert(buf != nullptr || len == 0);
305-        if (len == 0 || m_fuzzed_data_provider.ConsumeBool()) {
306-            const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
307-            if (r == -1) {
308-                SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos);
309-            }
310-            return r;
311-        }
312-        std::vector<uint8_t> random_bytes;
313-        bool pad_to_len_bytes{m_fuzzed_data_provider.ConsumeBool()};
314-        if (m_peek_data.has_value()) {
315-            // `MSG_PEEK` was used in the preceding `Recv()` call, return `m_peek_data`.
316-            random_bytes.assign({m_peek_data.value()});
317-            if ((flags & MSG_PEEK) == 0) {
318-                m_peek_data.reset();
319-            }
320-            pad_to_len_bytes = false;
321-        } else if ((flags & MSG_PEEK) != 0) {
322-            // New call with `MSG_PEEK`.
323-            random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(1);
324-            if (!random_bytes.empty()) {
325-                m_peek_data = random_bytes[0];
326-                pad_to_len_bytes = false;
327-            }
328-        } else {
329-            random_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(
330-                m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, len));
331-        }
332-        if (random_bytes.empty()) {
333-            const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1;
334-            if (r == -1) {
335-                SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos);
336-            }
337-            return r;
338-        }
339-        std::memcpy(buf, random_bytes.data(), random_bytes.size());
340-        if (pad_to_len_bytes) {
341-            if (len > random_bytes.size()) {
342-                std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size());
343-            }
344-            return len;
345-        }
346-        if (m_fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) {
347-            std::this_thread::sleep_for(std::chrono::milliseconds{2});
348-        }
349-        return random_bytes.size();
350-    }
351+    ssize_t Recv(void* buf, size_t len, int flags) const override;
352 
353-    int Connect(const sockaddr*, socklen_t) const override
354-    {
355-        // Have a permanent error at connect_errnos[0] because when the fuzzed data is exhausted
356-        // SetFuzzedErrNo() will always return the first element and we want to avoid Connect()
357-        // returning -1 and setting errno to EAGAIN repeatedly.
358-        constexpr std::array connect_errnos{
359-            ECONNREFUSED,
360-            EAGAIN,
361-            ECONNRESET,
362-            EHOSTUNREACH,
363-            EINPROGRESS,
364-            EINTR,
365-            ENETUNREACH,
366-            ETIMEDOUT,
367-        };
368-        if (m_fuzzed_data_provider.ConsumeBool()) {
369-            SetFuzzedErrNo(m_fuzzed_data_provider, connect_errnos);
370-            return -1;
371-        }
372-        return 0;
373-    }
374+    int Connect(const sockaddr*, socklen_t) const override;
375 
376-    int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override
377-    {
378-        constexpr std::array getsockopt_errnos{
379-            ENOMEM,
380-            ENOBUFS,
381-        };
382-        if (m_fuzzed_data_provider.ConsumeBool()) {
383-            SetFuzzedErrNo(m_fuzzed_data_provider, getsockopt_errnos);
384-            return -1;
385-        }
386-        if (opt_val == nullptr) {
387-            return 0;
388-        }
389-        std::memcpy(opt_val,
390-                    ConsumeFixedLengthByteVector(m_fuzzed_data_provider, *opt_len).data(),
391-                    *opt_len);
392-        return 0;
393-    }
394+    int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override;
395 
396     bool Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred = nullptr) const override;
397 
398-    bool IsConnected(std::string& errmsg) const override
399-    {
400-        if (m_fuzzed_data_provider.ConsumeBool()) {
401-            return true;
402-        }
403-        errmsg = "disconnected at random by the fuzzer";
404-        return false;
405-    }
406+    bool IsConnected(std::string& errmsg) const override;
407 };
408 
409 [[nodiscard]] inline FuzzedSock ConsumeSock(FuzzedDataProvider& fuzzed_data_provider)
410 {
411     return FuzzedSock{fuzzed_data_provider};
412 }

    (feel free to ignore)

    MarcoFalke commented at 8:41 am on April 7, 2021:
    Concept ACK on the commit. Please open a new pr and ping me for review.
  11. vasild approved
  12. vasild commented at 8:32 am on April 7, 2021: member

    ACK 33333755f2edcbe88fcd136f6fef81f94819002e

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

  13. MarcoFalke merged this on Apr 7, 2021
  14. MarcoFalke closed this on Apr 7, 2021
  15. MarcoFalke deleted the branch on Apr 7, 2021
  16. MarcoFalke commented at 8:42 am on April 7, 2021: member
    I merged this to unbreak our valgrind fuzzer. Happy to review the proposed changes in a follow-up pr.
  17. vasild commented at 9:12 am on April 7, 2021: member
    Followup in https://github.com/bitcoin/bitcoin/pull/21630
  18. sidhujag referenced this in commit e1c3fd05be on Apr 7, 2021
  19. MarcoFalke referenced this in commit c6b30ccb2e on Apr 15, 2021
  20. DrahtBot locked this on Aug 16, 2022


MarcoFalke jonatack sipa vasild

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-05 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me