fuzz: don't pass (maybe) nullptr to memcpy() #33644
Conversation
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: bitcoin#33643
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33644. ReviewsSee the guideline for information on the review process. |
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. |
I think this is not so much about the compiler but about the nonnull attribute found in 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 |
According to the C standard (§7.26.1), the behaviour of |
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:
|
| if (!sin_addr_bytes.empty()) { | ||
| memcpy(&addr4->sin_addr, sin_addr_bytes.data(), sin_addr_bytes.size()); | ||
| } |
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
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 #33743
5 participants
If
ConsumeBytes()returns an empty vector, then itsdata()method may or may not return a null pointer [1]. Itssize()method will return 0 andmemcpy(dst, maybenull, 0)will be called fromFuzzedSock::Accept().Given that the
lenargument is 0,memcpy()should not try to dereference the maybenull argument, but nevertheless the sanitizer is upset: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