fuzz: don't pass (maybe) nullptr to memcpy() #33644

vasild commented at 10:25 AM on October 17, 2025: contributor

If ConsumeBytes() returns an empty vector, then its data() method may or may not return a null pointer [1]. Its size() method will return 0 and memcpy(dst, maybenull, 0) will be called from FuzzedSock::Accept().

Given that the len argument is 0, memcpy() should not try to dereference the maybenull argument, but nevertheless the sanitizer is upset:

./src/test/fuzz/util/net.cpp:337:43: runtime error: null pointer passed
as argument 2, which is declared to never be null

Fix this by avoiding the call to memcpy() if an empty vector is returned. The full target buffer is zeroed upfront.

[1] https://en.cppreference.com/w/cpp/container/vector/data

Resolves: #33643

fuzz: don't pass (maybe) nullptr to memcpy()

If `ConsumeBytes()` returns an empty vector, then its `data()` method
may or may not return a null pointer [1]. Its `size()` method will
return 0 and `memcpy(dst, maybenull, 0)` will be called from
`FuzzedSock::Accept()`.

Given that the `len` argument is 0, `memcpy()` should not try to
dereference the maybenull argument, but nevertheless the sanitizer is
upset:

```
./src/test/fuzz/util/net.cpp:337:43: runtime error: null pointer passed
as argument 2, which is declared to never be null
```

Fix this by avoiding the call to `memcpy()` if an empty vector is
returned. The full target buffer is zeroed upfront.

[1] https://en.cppreference.com/w/cpp/container/vector/data

Resolves: https://github.com/bitcoin/bitcoin/issues/33643

9de18cc262

DrahtBot added the label Tests on Oct 17, 2025

DrahtBot commented at 10:25 AM on October 17, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33644.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

maflcko commented at 11:20 AM on October 17, 2025: member

Given that the len argument is 0, memcpy() should not try to dereference the maybenull argument

That is right and it was already fixed upstream, see also #32016 (comment).

Historically this was a GCC bug, but I was using clang, so I wonder why this somehow changed in the wrong direction. I'll try to take a closer look next week.

vasild commented at 12:08 PM on October 17, 2025: contributor

Historically this was a GCC bug, but I was using clang, so I wonder why this somehow changed in the wrong direction.

I think this is not so much about the compiler but about the nonnull attribute found in /usr/include/string.h, which is used by both gcc and clang:

/* Copy N bytes of SRC to DEST.  */
extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
                     size_t __n) __THROW __nonnull ((1, 2));

FreeBSD does not have such a nonnull attribute, so this could explain why I did not reproduce it on FreeBSD.

Loong time ago I had a very frustrating and unproductive bug-chase in another project. It boiled down to this nonnull attribute. It turned out that the compiler, seeing the nonnull attribute, removed (optimized away) parts of the function body that contained if (arg == nullptr) { handle and return from the function; } because it assumed that the argument will never be null.

purpleKarrot commented at 6:39 AM on October 18, 2025: contributor

Given that the len argument is 0, memcpy() should not try to dereference the maybenull argument

According to the C standard (§7.26.1), the behaviour of memcpyis undefined if either srcor dstare null. So it is not a bug.

maflcko commented at 7:49 AM on October 22, 2025: member

Given that the len argument is 0, memcpy() should not try to dereference the maybenull argument

According to the C standard (§7.26.1), the behaviour of memcpyis undefined if either srcor dstare null. So it is not a bug.

Apart from https://www.open-std.org/JTC1/SC22/WG14/www/docs/n3466.pdf , see also https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3322.pdf , which says:

Modify 7.26.1p3: Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall ... may be null pointers.

in src/test/fuzz/util/net.cpp:328 in 9de18cc262

 322 | @@ -323,7 +323,9 @@ std::unique_ptr<Sock> FuzzedSock::Accept(sockaddr* addr, socklen_t* addr_len) co
 323 |                  auto addr4 = reinterpret_cast<sockaddr_in*>(addr);
 324 |                  addr4->sin_family = AF_INET;
 325 |                  const auto sin_addr_bytes = m_fuzzed_data_provider.ConsumeBytes<uint8_t>(sizeof(addr4->sin_addr));
 326 | -                memcpy(&addr4->sin_addr, sin_addr_bytes.data(), sin_addr_bytes.size());
 327 | +                if (!sin_addr_bytes.empty()) {
 328 | +                    memcpy(&addr4->sin_addr, sin_addr_bytes.data(), sin_addr_bytes.size());
 329 | +                }

maflcko commented at 7:57 AM on October 22, 2025:

Instead of using C functions in a c++ codebase and then working around C quirks temporarily, it seems better to just use the c++ algorithms. The have a few benefits:

The compiler will optimize them to the same asm, but performance doesn't really matter here anyway
They work around the uban bug
they have the additional benefit of being a bit more type safe and document the byte cast explicitly
The resulting code and logic is shorter

std::ranges::copy(sin_addr_bytes, reinterpret_cast<uint8_t*>(&addr4->sin_addr));

fanquake commented at 5:11 PM on October 29, 2025:

@vasild did you want to follow up here?

maflcko commented at 10:10 AM on October 30, 2025:

Looks like this fell off the radar. It is a fuzz blocker for 31.0, so I went ahead and pushed my alternative fix in https://github.com/bitcoin/bitcoin/pull/33743

fanquake commented at 10:11 AM on October 30, 2025:

Ok, closing this for now.

maflcko added this to the milestone 31.0 on Oct 25, 2025

fanquake closed this on Oct 30, 2025

fanquake removed this from the milestone 31.0 on Oct 30, 2025