refactor: memcpy to std::ranges::copy to work around ubsan warn - #305
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste |
d4b028b to
fac7b9b
Compare
|
Failure should look roughly like: |
|
Concept ACK on replacing |
|
ACK fac7b9b I was also able to reproduce the error that the new test triggers. |
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK fac7b9b. Thanks for the fix!
It's still not completely clear to me if this purely a ubsan issue or if there could have been real problems here with compiler optimizations. For example https://developers.redhat.com/articles/2024/12/11/making-memcpynull-null-0-well-defined would seem to suggest that compilers could have used this memcpy to falsely assume result.begin() and data.init() are never null and skip null checks in nearby code. But avoiding memcpy is an improvement in any case.
Possible followups to this change:
- There are also other uses of memcpy in the codebase that could be replaced
- Code would be simpler without the
resultvariable, just usingcopy(value, output.init(value.size())
I am sure the second arg is the beginning of the destination range, so it would have been: std::ranges::copy(value, output.init(value.size()).begin());
In the past, this was certainly UB in theory (not sure if a compiler exists in practise to exploit this). I think it is fixed in C29, but it is not clear how fast compilers will apply this fix. (Ref: bitcoin/bitcoin#32016 (comment))
Also checked them, but I think they are fine (no promise though). There is https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-ranges.html that can be used? Happy to take a look there, if you find it worthwhile. |
Actually, this is unrelated. That check is about moving iterator-based algos to range-based ones. For the memcpy replacement, there is llvm/llvm-project#22583 |
e8de5c7b68 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn 9307e68e5a Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13 fac7b9b7f6 refactor: memcpy to std::ranges::copy to work around ubsan warn 1bd7025609 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage 438fdd243d doc: Bump version 12 > 13 463d073cb8 test: rename vBool to vector_bool 85df233845 test: add mapStringInt to foo.capnp to cover map serialization and deserialization git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: e8de5c7b68e0ae21c94ae92aa22e5c3b213f9c12
This avoids a std::span ctor. Recommended in bitcoin-core#305 (review)
I think there is no issue of passing a nullptr here, but it makes sense to use the std-lib copy for consistency. Recommended in bitcoin-core#305 (review)
e8de5c7b68 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn 9307e68e5a Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13 fac7b9b7f6 refactor: memcpy to std::ranges::copy to work around ubsan warn 1bd7025609 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage 438fdd243d doc: Bump version 12 > 13 28e056576a Merge bitcoin-core/libmultiprocess#269: proxy: add local connection limit to ListenConnections 39a10ce895 proxy: add local connection limit to ListenConnections() 43172f52d9 test: add dedicated ListenConnections coverage 033f812195 doc/version: Bump version 11 > 12 463d073cb8 test: rename vBool to vector_bool 16bf05dea0 Merge bitcoin-core/libmultiprocess#302: refactor: rename EventLoop::m_num_clients to m_num_refs dd537da9e4 Merge bitcoin-core/libmultiprocess#301: test: recursive async IPC calls and cleanups 400291de00 Merge bitcoin-core/libmultiprocess#299: ci: remove libevent from Core CIs 092be515ad Merge bitcoin-core/libmultiprocess#285: Add ReadList helper 5b617880c5 Merge bitcoin-core/libmultiprocess#283: Add `makePool` method on `ThreadMap` d499830415 refactor: rename EventLoop::m_num_clients to m_num_refs 6450345c98 type: reserve first when reading std::unordered_set 4d0f8db5f9 proxy: add ReadList helper and dedup map/set/vector read handlers 0e49d91186 Add `makePool` method on `ThreadMap` 5519f7f948 test: recursive async IPC calls a29ceff40b ci: remove libevent from Core CIs 85df233845 test: add mapStringInt to foo.capnp to cover map serialization and deserialization git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: e8de5c7b68e0ae21c94ae92aa22e5c3b213f9c12
e8de5c7b68 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn 9307e68e5a Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13 fac7b9b7f6 refactor: memcpy to std::ranges::copy to work around ubsan warn 1bd7025609 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage 438fdd243d doc: Bump version 12 > 13 28e056576a Merge bitcoin-core/libmultiprocess#269: proxy: add local connection limit to ListenConnections 39a10ce895 proxy: add local connection limit to ListenConnections() 43172f52d9 test: add dedicated ListenConnections coverage 033f812195 doc/version: Bump version 11 > 12 463d073cb8 test: rename vBool to vector_bool 16bf05dea0 Merge bitcoin-core/libmultiprocess#302: refactor: rename EventLoop::m_num_clients to m_num_refs dd537da9e4 Merge bitcoin-core/libmultiprocess#301: test: recursive async IPC calls and cleanups 400291de00 Merge bitcoin-core/libmultiprocess#299: ci: remove libevent from Core CIs 092be515ad Merge bitcoin-core/libmultiprocess#285: Add ReadList helper 5b617880c5 Merge bitcoin-core/libmultiprocess#283: Add `makePool` method on `ThreadMap` d499830415 refactor: rename EventLoop::m_num_clients to m_num_refs 6450345c98 type: reserve first when reading std::unordered_set 4d0f8db5f9 proxy: add ReadList helper and dedup map/set/vector read handlers 0e49d91186 Add `makePool` method on `ThreadMap` 5519f7f948 test: recursive async IPC calls a29ceff40b ci: remove libevent from Core CIs 85df233845 test: add mapStringInt to foo.capnp to cover map serialization and deserialization git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: e8de5c7b68e0ae21c94ae92aa22e5c3b213f9c12
I think there is no issue of passing a nullptr to memcpy here, but it makes sense to use the std-lib copy for consistency. Recommended in bitcoin-core#305 (review) Also, add a size sanity check when reading a char array.
26452e0 refactor: memcpy -> std::ranges::copy (marco) 17eab90 test: Fix typo in listen_tests.cpp (marco) ce865a9 refactor: Directly use value in CustomBuildField (marco) Pull request description: Some follow-ups to #305 (review) ACKs for top commit: ryanofsky: Code review ACK 26452e0. Thanks for the updates! Tree-SHA512: 84f07ed0583fac4a05da890808509ea695db3de4e17ed5497e9612306b9d3821167d5d2500381b218a42a5b7858016da659bd3df3e0458962253a8e3ff81fbae
Using std::ranges::copy from the C++ standard library has a few benefits here:
This should also fix bitcoin/bitcoin#35118 (comment)