It is UB to apply a distance to a pointer or iterator further than the end itself, even if the distance is (partially) revoked later on.
Fix the issue by advancing the data pointer at most to the end.
This fix is required to adopt C++ safe buffers #31272.
Also included is a somewhat unrelated commit.
This is no longer needed after commit
6aa0e70ccbd5491ec9d7c81892820f3341ccd631
It is UB to apply a distance to a pointer or iterator further than the
end itself, even if the distance is (partially) revoked later on.
Fix the issue by advancing the data pointer at most to the end.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31655.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
bitcoind was unchanged, so I tagged this as “refactor”. However, this may not hold for every compiler and build setting.
This can be tested with a diff:
0diff --git a/src/crypto/sha3.cpp b/src/crypto/sha3.cpp
1index 770500bfe2..6eedba3ad6 100644
2--- a/src/crypto/sha3.cpp
3+++ b/src/crypto/sha3.cpp
4@@ -103,8 +103,10 @@ void KeccakF(uint64_t (&st)[25])
5 }
6 }
7
8-SHA3_256& SHA3_256::Write(Span<const unsigned char> data)
9+#include <span>
10+SHA3_256& SHA3_256::Write(Span<const unsigned char> old)
11 {
12+std::span data{old};
13 if (m_bufsize && m_bufsize + data.size() >= sizeof(m_buffer)) {
14 // Fill the buffer and process it.
15 std::copy(data.begin(), data.begin() + sizeof(m_buffer) - m_bufsize, m_buffer + m_bufsize);
On master it fails when running MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh. On this pull, it should pass.
It is UB to apply a distance to a pointer or iterator further than the end itself, even if the distance is (partially) revoked later on.
I’m not arguing with this change, but could someone provide a quote from the C++20 standard that stipulates this? From my reading of the standard, it appears that only dereferencing causes UB in such cases.
It is UB to apply a distance to a pointer or iterator further than the end itself, even if the distance is (partially) revoked later on.
I’m not arguing with this change, but could someone provide a quote from the C++20 standard that stipulates this? From my reading of the standard, it appears that only dereferencing causes UB in such cases.
No, the addition explicitly declares this as UB, see https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to […] an expression P of pointer type, the result has the type of P. … if P points to a (possibly-hypothetical) array element i of an array object x with n elements […] the expressions P + J and J + P (where J has the value j) point to the (possibly-hypothetical) array element i+j of x if 0≤i+j≤n […] Otherwise, the behavior is undefined.
Note: In the context of Span, an iterator is a pointer.
This can be tested with a diff:
0diff --git a/src/crypto/sha3.cpp b/src/crypto/sha3.cpp 1index 770500bfe2..6eedba3ad6 100644 2--- a/src/crypto/sha3.cpp 3+++ b/src/crypto/sha3.cpp 4@@ -103,8 +103,10 @@ void KeccakF(uint64_t (&st)[25]) 5 } 6 } 7 8-SHA3_256& SHA3_256::Write(Span<const unsigned char> data) 9+#include <span> 10+SHA3_256& SHA3_256::Write(Span<const unsigned char> old) 11 { 12+std::span data{old}; 13 if (m_bufsize && m_bufsize + data.size() >= sizeof(m_buffer)) { 14 // Fill the buffer and process it. 15 std::copy(data.begin(), data.begin() + sizeof(m_buffer) - m_bufsize, m_buffer + m_bufsize);On master it fails when running
MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh. On this pull, it should pass.
I can confirm this.