When building #23152 with g++ 11.1 a few -Wmaybe-uninitialized warnings are shown. The warning in FastRandomContext seems legit, and this PR fixes that.
Note that there are more warnings in leveldb code which I did not investigate. There are warnings in fuzz/float.cpp and fuzz/string.cpp which are also left untouched (see #23169 (review))
18 | @@ -19,7 +19,7 @@ FUZZ_TARGET(float) 19 | 20 | { 21 | const double d{[&] { 22 | - double tmp; 23 | + double tmp{0};
The goal of leaving this uninitialized is to have memory sanitizers yell. By initializing you disable memory sanitizers.
One workaround would be to make this optional, then return *Assert(tmp); (or tmp.value()). But I am not sure if this is worth it.
ah I didn't think about that. I'll remove this commit
231 | - uint8_t u8; 232 | + int32_t i32{0}; 233 | + int64_t i64{0}; 234 | + uint32_t u32{0}; 235 | + uint64_t u64{0}; 236 | + uint8_t u8{0};
Same
Concept ACK on fixing the unitialized read (bitbuf) which can be tested with Valgrind using the following patch:
diff --git a/src/random.cpp b/src/random.cpp
index 174f4cef3..73e946783 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -693,13 +693,30 @@ FastRandomContext::FastRandomContext(bool fDeterministic) noexcept : requires_se
rng.SetKey(seed.begin(), 32);
}
+// Force Valgrind use-of-uninitialized memory (UUM) violation if `o` is uninitialized.
+//
+// As suggested by [@guidovranken](/bitcoin-bitcoin/contributor/guidovranken/) in [#22064](/bitcoin-bitcoin/22064/)
+template<typename T>
+void ForceValgrindWarningIfUninitialized(const T& o) {
+ static_assert(std::is_trivially_copyable<T>::value);
+ FILE* f = fopen("/dev/null", "wb");
+ fwrite(&o, sizeof(o), 1, f);
+ fclose(f);
+}
+
FastRandomContext& FastRandomContext::operator=(FastRandomContext&& from) noexcept
{
+ ForceValgrindWarningIfUninitialized(from.requires_seed);
requires_seed = from.requires_seed;
+ ForceValgrindWarningIfUninitialized(from.rng);
rng = from.rng;
+ ForceValgrindWarningIfUninitialized(from.bytebuf);
std::copy(std::begin(from.bytebuf), std::end(from.bytebuf), std::begin(bytebuf));
+ ForceValgrindWarningIfUninitialized(from.bytebuf_size);
bytebuf_size = from.bytebuf_size;
+ ForceValgrindWarningIfUninitialized(from.bitbuf);
bitbuf = from.bitbuf;
+ ForceValgrindWarningIfUninitialized(from.bitbuf_size);
bitbuf_size = from.bitbuf_size;
from.requires_seed = true;
from.bytebuf_size = 0;
And running valgrind src/test/test_bitcoin -t addrman_tests/addrman_simple:
==22209== Memcheck, a memory error detector
…
Running 1 test case...
==22209== Syscall param write(buf) points to uninitialised byte(s)
==22209== at 0x736A264: write (write.c:27)
==22209== by 0x72E522C: _IO_file_write@@GLIBC_2.2.5 (fileops.c:1203)
==22209== by 0x72E6FC0: new_do_write (fileops.c:457)
==22209== by 0x72E6FC0: _IO_do_write@@GLIBC_2.2.5 (fileops.c:433)
==22209== by 0x72E637F: _IO_file_close_it@@GLIBC_2.2.5 (fileops.c:136)
==22209== by 0x72D83F6: fclose@@GLIBC_2.2.5 (iofclose.c:53)
==22209== by 0xDA86B7: ForceValgrindWarningIfUninitialized<unsigned char [64]> (random.cpp:704)
==22209== by 0xDA86B7: FastRandomContext::operator=(FastRandomContext&&) (random.cpp:713)
==22209== by 0x9CFD67: Seed(FastRandomContext&) (setup_common.cpp:66)
==22209== by 0x9D066B: SeedInsecureRand (setup_common.h:61)
==22209== by 0x9D066B: BasicTestingSetup::BasicTestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&) (setup_common.cpp:105)
==22209== by 0x3AB6BE: addrman_simple (addrman_tests.cpp:166)
…
As it has historically been a source of some very serious vulnerabilities I'm somewhat concerned around fixing uninitialized value warnings in random functionality. Please review this carefully. Concept ACK of course.
124 | + bool requires_seed{true}; 125 | + ChaCha20 rng{}; 126 | 127 | - unsigned char bytebuf[64]; 128 | - int bytebuf_size; 129 | + unsigned char bytebuf[64] = {};
shouldn't this be left uninitialized? With this patch memory sanitizer will fail to detect logic errors.
I'm not sure msan would catch any error here. If I understand it correctly, msan can only catch uninitialized memory when a branch is involved. I played a bit with this sample code which loops over uninitialized memory and prints a result:
#include <iostream>
struct Foo {
unsigned char bytebuf[1024 * 1024];
};
int main() {
Foo f; // uninitialized
uint64_t x = 0;
for (size_t i = 0; i < 1024 * 1024; ++i) {
// if (f.bytebuf[i]) {
x += f.bytebuf[i];
//}
}
std::cout << x << std::endl;
}
I compiled with clang++ -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls test.cpp && ./a.out.
if, clang's memory sanitizer does not detect any uninitialized memory and prints a random number. Even when compiling with -O0.if commented in, clang only detect an uninitialized memory error when compiled with -O0.So I think msan can't really catch much here
I don't thinks the shortcomings of one memory sanitizer tool should be used as an argument to disable other sanitizer tools as well. valgrind detects this (with or without the if).
Not a comment on these specific changes, but as a more general comment: note that VALGRIND_MAKE_MEM_UNDEFINED (Valgrind) and __msan_poison (MSAN) can be used to mark memory regions as uninitialized from a tooling perspective (regardless of actual status).
Example use: https://github.com/kriskwiatkowski/pqc/blob/main/src/common/ct_check.h
I've removed the initialization of bytebuf in f46225ea5cadcb94cbaf7b77346bfef622d35eed
Code review ACK f46225ea5cadcb94cbaf7b77346bfef622d35eed
I still don't understand this.
Reading #23169 (comment) tells me that the issue is (also) bytebuf, but you are fixing bitbuf and leave bytebuf an uninitialized read?
Building with `--enable-lto`in #23152 produced a `-Wmaybe-uninitialized`
warning in FastRandomContext. This commit fixes the move constructor:
There was undefined behavior due to uninitialized copy, and
uninitialized reads in bytebuf.
The `FastRandomContext(const uint256& seed)` constructor did not
initialize the `uint64_t bitbuf` member, and in
`FastRandomContext& operator=(FastRandomContext&& from) noexcept;` the
uninitialized variable was copied with `bitbuf = from.bitbuf;` which
technically is undefined behavior. This happens in `setup_common.cpp`
in the line `ctx = FastRandomContext(seed);`.
My change was only about the undefined behavior issue, because assigning from an uninitialized variable that's not a char is undefined behavior. As far as I understand it with bytebuf that's not undefined behavior but just reading uninitialized memory: https://eel.is/c++draft/basic.indet#example-1
I've rebased and updated the PR though to get rid of reading uninitialized memory as well in 10aeb6e
If the buffer sizes are initialized, I'm pretty sure the uninitialized area of the byte buffer itself will never be read from (because it will be filled before any read happens). So I'm in favor of not initializing the byte buffer, giving analysis tools a chance to detect bugs with it.
Jup, I agree, see #23169 (review).
Now that you removed the initialization, the pull request title etc should be updated? (The current patch does not change initialization)
Right, I've updated the title
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | sipa, MarcoFalke |
| Concept ACK | practicalswift |
| Stale ACK | laanwj |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
utACK 10aeb6e6484048b061620d484dbb10dee0ead386
696 | @@ -697,9 +697,13 @@ FastRandomContext& FastRandomContext::operator=(FastRandomContext&& from) noexce 697 | { 698 | requires_seed = from.requires_seed; 699 | rng = from.rng; 700 | - std::copy(std::begin(from.bytebuf), std::end(from.bytebuf), std::begin(bytebuf)); 701 | + if (from.bytebuf_size != 0) { 702 | + std::copy(std::begin(from.bytebuf), std::end(from.bytebuf), std::begin(bytebuf));
This may still be UB if bytebuf_size is nonzero, but less than 64 and std::copy does not decay to the "simple assignment operator" (https://eel.is/c++draft/basic.indet#2.2), no?
If yes, I am not sure if it is worth it to fix, but it could be fixed by only copying the initialized part?
Otherwise, the hunk could also be dropped to leave the code in master as-is.
Hmm, it should probably be std::copy(std::begin(from.bytebuf), std::begin(from.bytebuf) + from.bytebuf_size, std::begin(bytebuf));? Then the if can be dropped too.
It probably uses = (https://eel.is/c++draft/alg.copy#3), so I guess any code is fine here. Maybe still revert to the code in master to avoid unnecessary changes and confusion?
Can still reproduce with gcc 12.2
Left a question.
Happy to re-ACK if you decide to change something
ACK 10aeb6e6484048b061620d484dbb10dee0ead386 🌻
<details><summary>Show signature</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 10aeb6e6484048b061620d484dbb10dee0ead386 🌻
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi1fAv/Y3d+GlbG+mm4/OqUCynfgYTZQRKI6YGchUigjXm4pKVT+zMjUZ46r+CK
4ncyI81dSp4XEroQ4Ppstljk+6tYLq/1+E6usfMQnO109oFfTqkYVLBeMdqZBUSH
w7SMoQkEmHEYxrNbx08qAy3uyymnwqkJgS7JUfVsrmdhw7CvyPBICDzGkYrtPCx2
z13VVmQPMHWjDH5700pQp9zbAXBBba+WQBQ0UfsahdPBGTABDQx86x3PtKMyW4kI
LI7LTA1s5F6VCR2W53esqdJGGsn4D03l0LBkOKU5lwXncepfiMmncisOl4y2e2ZQ
J2/TsCyk1PjqitOnHJd8SlDxOP26EqfvtjH6IHK4SE3xTvnZZegj2UDRTXH3zmis
HS4VZBRF+nYWC94Nk8zYi+PwYoreyjTZy0K9YEJcb5T1QswZeyT/vEgm3fEEe85E
a0W0k5C0T27mzcy+W3G0W0h9zot+jjsPwwLQ+HC84QqwS3FtzxlOrtCI7rs1DzgE
mkX4AlCj
=Qwgq
-----END PGP SIGNATURE-----
</details>
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
