fix initialization in FastRandomContext: removes undefined behavior & uninitialized read #23169
Conversation
749e88e to
98d1d7a
Compare
-Wmaybe-uninitialized warnings found by building with -flto|
Concept ACK on fixing the unitialized read ( 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 in https://github.com/bitcoin/bitcoin/issues/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 |
|
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. |
|
Yeah, would have been good if |
98d1d7a to
f46225e
Compare
|
Code review ACK f46225e |
|
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 bitcoin#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);`.
f46225e to
10aeb6e
Compare
|
My change was only about the undefined behavior issue, because assigning from an uninitialized variable that's not a 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 (comment). 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 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee 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. ConflictsReviewers, 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 10aeb6e |
| @@ -697,9 +697,13 @@ FastRandomContext& FastRandomContext::operator=(FastRandomContext&& from) noexce | |||
| { | |||
| requires_seed = from.requires_seed; | |||
| rng = from.rng; | |||
| std::copy(std::begin(from.bytebuf), std::end(from.bytebuf), std::begin(bytebuf)); | |||
| if (from.bytebuf_size != 0) { | |||
| std::copy(std::begin(from.bytebuf), std::end(from.bytebuf), std::begin(bytebuf)); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Happy to re-ACK if you decide to change something
ACK 10aeb6e 🌻
Show signature
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-----
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Now that #26153 was merged this is not relevant any more / was fixed (as far as I can say) |
There are still some |
When building #23152 with g++ 11.1 a few
-Wmaybe-uninitializedwarnings are shown. The warning inFastRandomContextseems 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 (comment))