Suppress false positive warning about uninitialized entropy buffers

Sjors commented at 9:37 am on November 28, 2019: member

The memory sanitizer cannot see through syscall (see https://github.com/google/sanitizers/issues/852). This caused a false positive use-of-uninitialized-value error for SYS_getrandom.

Given recent problematic real issues with uninitialized variables, I think it’s worth getting rid of the false positives so developers, and maybe Travis, can run the sanitizer.

 0==18287==WARNING: MemorySanitizer: use-of-uninitialized-value
 1    [#0](/bitcoin-bitcoin/0/) 0x561f1c40bfc8 in ReadBE64(unsigned char const*) /home/dev/bitcoin/src/./crypto/common.h:67:12
 2    [#1](/bitcoin-bitcoin/1/) 0x561f1c3fe1d3 in (anonymous namespace)::sha512::Transform(unsigned long*, unsigned char const*) /home/dev/bitcoin/src/crypto/sha512.cpp:59:63
 3    [#2](/bitcoin-bitcoin/2/) 0x561f1c3fcb77 in CSHA512::Write(unsigned char const*, unsigned long) /home/dev/bitcoin/src/crypto/sha512.cpp:168:9
 4    [#3](/bitcoin-bitcoin/3/) 0x561f1c2d102d in CSHA512& (anonymous namespace)::operator<<<timespec>(CSHA512&, timespec const&) /home/dev/bitcoin/src/randomenv.cpp:119:12
 5    [#4](/bitcoin-bitcoin/4/) 0x561f1c2d07e8 in RandAddDynamicEnv(CSHA512&) /home/dev/bitcoin/src/randomenv.cpp:249:12
 6    [#5](/bitcoin-bitcoin/5/) 0x561f1c2cbfb9 in SeedStartup(CSHA512&, (anonymous namespace)::RNGState&) /home/dev/bitcoin/src/random.cpp:513:5
 7    [#6](/bitcoin-bitcoin/6/) 0x561f1c2c933e in ProcRand(unsigned char*, int, RNGLevel) /home/dev/bitcoin/src/random.cpp:553:9
 8    [#7](/bitcoin-bitcoin/7/) 0x561f1c2c8f98 in GetRandBytes(unsigned char*, int) /home/dev/bitcoin/src/random.cpp:558:59
 9    [#8](/bitcoin-bitcoin/8/) 0x561f1baa521e in (anonymous namespace)::CSignatureCache::CSignatureCache() /home/dev/bitcoin/src/script/sigcache.cpp:34:9
10    [#9](/bitcoin-bitcoin/9/) 0x561f1b3bcdf9 in __cxx_global_var_init.3 /home/dev/bitcoin/src/script/sigcache.cpp:67:24
11    [#10](/bitcoin-bitcoin/10/) 0x561f1b3bce88 in _GLOBAL__sub_I_sigcache.cpp /home/dev/bitcoin/src/script/sigcache.cpp
12    [#11](/bitcoin-bitcoin/11/) 0x561f1c60107c in __libc_csu_init (/home/dev/bitcoin/src/bitcoind+0x129507c)
13    [#12](/bitcoin-bitcoin/12/) 0x7fc924a8fb27 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:266
14    [#13](/bitcoin-bitcoin/13/) 0x561f1b3c3659 in _start (/home/dev/bitcoin/src/bitcoind+0x57659)
15
16  Uninitialized value was stored to memory at
17    [#0](/bitcoin-bitcoin/0/) 0x561f1b3c9316 in __msan_memcpy (/home/dev/bitcoin/src/bitcoind+0x5d316)
18    [#1](/bitcoin-bitcoin/1/) 0x561f1c3fcc8c in CSHA512::Write(unsigned char const*, unsigned long) /home/dev/bitcoin/src/crypto/sha512.cpp:179:9
19    [#2](/bitcoin-bitcoin/2/) 0x561f1c2cbc50 in SeedSlow(CSHA512&) /home/dev/bitcoin/src/random.cpp:467:12
20    [#3](/bitcoin-bitcoin/3/) 0x561f1c2cbf65 in SeedStartup(CSHA512&, (anonymous namespace)::RNGState&) /home/dev/bitcoin/src/random.cpp:509:5
21    [#4](/bitcoin-bitcoin/4/) 0x561f1c2c933e in ProcRand(unsigned char*, int, RNGLevel) /home/dev/bitcoin/src/random.cpp:553:9
22    [#5](/bitcoin-bitcoin/5/) 0x561f1c2c8f98 in GetRandBytes(unsigned char*, int) /home/dev/bitcoin/src/random.cpp:558:59
23    [#6](/bitcoin-bitcoin/6/) 0x561f1baa521e in (anonymous namespace)::CSignatureCache::CSignatureCache() /home/dev/bitcoin/src/script/sigcache.cpp:34:9
24    [#7](/bitcoin-bitcoin/7/) 0x561f1b3bcdf9 in __cxx_global_var_init.3 /home/dev/bitcoin/src/script/sigcache.cpp:67:24
25    [#8](/bitcoin-bitcoin/8/) 0x561f1b3bce88 in _GLOBAL__sub_I_sigcache.cpp /home/dev/bitcoin/src/script/sigcache.cpp
26    [#9](/bitcoin-bitcoin/9/) 0x561f1c60107c in __libc_csu_init (/home/dev/bitcoin/src/bitcoind+0x129507c)
27
28  Uninitialized value was created by an allocation of 'buffer' in the stack frame of function '_ZL8SeedSlowR7CSHA512'
29    [#0](/bitcoin-bitcoin/0/) 0x561f1c2cbb80 in SeedSlow(CSHA512&) /home/dev/bitcoin/src/random.cpp:459
30
31SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/dev/bitcoin/src/./crypto/common.h:67:12 in ReadBE64(unsigned char const*)
32Exiting

See #17620 for how to use the memory sanitizer.

laanwj commented at 9:48 am on November 28, 2019: member

I think in general, initializing everything unless it’s deliberately not done for performance reasons, is a good thing. One company I’ve worked for even had a strict “initialize everything” policy, no exceptions.

Fixing random buffers because of valgrind false positives has some very bad precedent though, please review carefully.

practicalswift commented at 9:53 am on November 28, 2019: contributor

@laanwj To be fair this is not a Valgrind false positive? :) Valgrind false positives are very rare in my experience.

laanwj commented at 10:00 am on November 28, 2019: member

To be fair this is not a Valgrind false positive? :) Valgrind false positives are very rare in my experience.

It’s a false positive because the buffer is actually filled in by the syscall but ~~valgrind~~ msan doesn’t notice.

The memory sanitizer cannot see through syscall (see google/sanitizers#852). This caused a false positive use-of-uninitialized-value error for SYS_getrandom.

Oh you’re right, wrong tool.

practicalswift commented at 10:02 am on November 28, 2019: contributor

@laanwj Yes, but the false positive is in MemorySanitizer – not Valgrind :)

elichai commented at 10:09 am on November 28, 2019: contributor

Concept ACK. From a quick glance it seems like the only change is initializing the value, and because unlike the openssl/debian fiasco, we don’t use unintialized memory for generating randomness then there’s no logical reason why this can make anything that currently works break (unless it was already broken).

Still requires close up review but I think it’s good to keep that in mind

Sjors commented at 10:17 am on November 28, 2019: member

Agree that it needs careful review. I can replace the {0} with “// intentionally not initialized” where needed.

Also note that the code this touches is relatively new (added by @sipa in #15250 and #17270), and deserves extra scrutiny anyway now that we’ve dropped the OpenSSL training wheels.

The actual initialization before this commit took place in GetOSRand, which is different depending on the operating system details. That’s probably another good reason to make sure stuff is initialized before it goes in there.

practicalswift commented at 10:29 am on November 28, 2019: contributor

Concept ACK

I think in general, initializing everything unless it’s deliberately not done for performance reasons, is a good thing. One company I’ve worked for even had a strict “initialize everything” policy, no exceptions.

Agree. I think it is worth doing.

Some background reading that might be of interest regarding the pros/cons of default initialization in general:

C++ Core Guidelines (Sutter & Stroustrup), “Always initialize an object”: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es20-always-initialize-an-object
Clang automatic variable initialization: https://reviews.llvm.org/D54604 + https://twitter.com/jfbastien/status/1063203517102415872
Apple has started using automatic variable initialization in iOS kernel builds: https://twitter.com/josephbialek/status/1168930302036762625
Microsoft has started using MSVC InitAll in Windows kernel builds: https://twitter.com/JosephBialek/status/1062774315098112001
Talk “Making C Less Dangerous in the Linux kernel”: https://www.youtube.com/watch?v=FY9SbqTO5GQ&feature=youtu.be&t=742
PDF: “Fighting Uninitialized Memory in the Kernel”: https://linuxplumbersconf.org/event/4/contributions/426/attachments/338/564/KMSAN_for_LPC_2019

Some quotes:

Linus Torvalds: “So I’d like the zeroing of local variables to be a native compiler option, so that we can (eventually - these things take a long time) just start saying “ok, we simply consider stack variables to be always initialized”.
Linus Torvalds: “Again - I don’t think we want a world where everything is force-initialized. There are going to be situations where that just hurts too much. But if we get to a place where we are zero-initialized by default, and have to explicitly mark the unsafe things (and we’ll have comments not just about how they get initialized, but also about why that particular thing is so performance-critical), that would be a good place to be.”

Note that there is a clang-tidy fixup that can rewrite this for us if we want (applies only to integers, booleans, floats, doubles and pointers unfortunately): https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-init-variables.html

practicalswift commented at 10:33 am on November 28, 2019: contributor

@Sjors Which subset of these changes are required to make MemorySanitizer happy? Not all these are technically needed, right? (I’m not opposing fixing it generally, but asking to get the full picture :))

in src/random.cpp:454 in ccfed8a225 outdated

450@@ -451,7 +451,7 @@ static void SeedFast(CSHA512& hasher) noexcept
451 
452 static void SeedSlow(CSHA512& hasher) noexcept
453 {
454-    unsigned char buffer[32];
455+    unsigned char buffer[32] = {0};

Sjors commented at 12:30 pm on November 28, 2019:

@practicalswift once I fixed this one, it ran into unrelated use-of-uninitialized-value problems. That could mean this is the only problem, or the other ones just didn’t get a chance to get called before the sanitizer bailed out.

jonatack commented at 2:02 pm on November 28, 2019: member

Concept ACK. I feel unqualified to ACK here, but I read the changes and the first 2 links provided by @practicalswift, built ccfed8a without sanitizer (–enable-debug only) with gcc version 8.3.0 (Debian 8.3.0-6) on Debian 4.19.37-5+deb10u2 (2019-08-08), ran tests, and am running bitcoind without issues and seeing the Feeding n bytes of dynamic environment data into RNG logs with typical values for n (~16700) on this installation.

gmaxwell commented at 3:01 pm on November 28, 2019: contributor

Initializing this would conceal from valgrind if changes in the code cased the random variable to not get filled which would be a moderately serious problem: Zero is not an acceptable random value. :)

Since msan is actually incorrect due to a known and documented limitation in msan, might it not be better to use a suppression? I’m not a fan of suppressing actual errors– but this isn’t an error, it’s a tool limit, and suppressing-by-initializing will reduce the effectiveness other tools without the same limit.

elichai commented at 3:09 pm on November 28, 2019: contributor

Initializing this would conceal from valgrind if changes in the code cased the random variable to not get filled which would be a moderately serious problem: Zero is not an acceptable random value. :)

Since msan is actually incorrect due to a known and documented limitation in msan, might it not be better to use a suppression? I’m not a fan of suppressing actual errors– but this isn’t an error, it’s a tool limit, and suppressing-by-initializing will reduce the effectiveness other tools without the same limit.

That’s a good point. does valgrind support syscalls?

Sjors commented at 3:51 pm on November 28, 2019: member

Maybe I can add assert statements directly after any buffers are filled to check that they’re not still 0? (though I suppose that could be forgotten in new places)

practicalswift commented at 9:11 pm on November 28, 2019: contributor

Clarification regarding my Concept ACK: a suppression would be totally fine too.

As long as we make it easier for everyone to use MemorySanitizer I’m happy: we need more MSAN testing :)

Sjors force-pushed on Nov 29, 2019

Sjors commented at 8:44 am on November 29, 2019: member

I added asserts to ensure these buffers are actually populated on first use. I also added @gmaxwell’s comment about valgrind. I’m not very familiar with how valgrind works. If it substitutes calls to hasher.Write(), hasher.Finalize(), GetOSRand(), etc with no-op, then this won’t work.

I could pick a magic value instead, initialize with that and use == in the assert.

laanwj commented at 8:47 am on November 29, 2019: member

That’s a good point. does valgrind support syscalls?

Yes, valgrind doesn’t have this issue.

Maybe I can add assert statements directly after any buffers are filled to check that they’re not still 0? (though I suppose that could be forgotten in new places)

But what if the input is legitly 0? These are not random values being fed in, but just some OS data, which may or may not consist of zeroes.

So NACK on the asserts.

in src/random.cpp:395 in a69bb009d0 outdated

390@@ -389,6 +391,8 @@ class RNGState {
391             ++m_counter;
392             // Finalize the hasher
393             hasher.Finalize(buf);
394+            // Ensure Finalize() populated buf
395+            assert(buf);

laanwj commented at 8:48 am on November 29, 2019:

FWIW: asserting the address of the buffer, on the stack, like you do here does nothing.

Sjors commented at 9:52 am on November 29, 2019:

For future reference, something like this works (?), though it’s hideous, probably needs a macro, which adds even more complexity:

0unsigned char zeros[sizeof(buf)] = {0};
1assert(!memcmp(fbuf, zeros, sizeof(buf)));

Sjors commented at 9:53 am on November 29, 2019:

Funny who you run into while googling that: https://rusty.ozlabs.org/?p=560 :-)

Sjors force-pushed on Nov 29, 2019

laanwj commented at 9:15 am on November 29, 2019: member

I’ve thought about it a bit and I’m principally opposed to adding asserts here. Now you’re changing run-time behavior of the application to work around a problem in an analysis tool.

I’m sorry to say this but : please fix the tool instead

practicalswift commented at 9:26 am on November 29, 2019: contributor

Echoing the NACK on the asserts :)

Sjors force-pushed on Nov 29, 2019

practicalswift commented at 9:30 am on November 29, 2019: contributor

@Sjors Have you considered taking the suppressions route instead? Perhaps that is less intrusive.

Sjors commented at 9:31 am on November 29, 2019: member

Reverted to the original approach, which just initializes to zero. I’m not sure if intentially leaving them uninitialized and having the warnings suppressed if the right approach either.

Sjors force-pushed on Nov 29, 2019

practicalswift commented at 9:34 am on November 29, 2019: contributor

I’m not sure if intentially leaving them uninitialized and having the warnings suppressed if the right approach either.

Why? :)

Sjors commented at 9:51 am on November 29, 2019: member

It seems like we’re choosing between two tools here:

memory sanity check: = {0}
valgrind: // intentionally undefined to allow valgrind to catch issues

There’s gotta be a solution that involves defined behavior. That would make the code easier to reason about, and prevents folks on different operating systems and compiler optimization flags not noticing bugs (as happened with #17568 and #17449).

laanwj commented at 10:00 am on November 29, 2019: member

In that case, let’s at least not penalize the tool that actually gets things right (valgrind).

So I’d vote for adding a MSAN surpression and just be done with it. It can be removed when the tool is fixed.

Sjors commented at 10:04 am on November 29, 2019: member

Wouldn’t initializing to a magic value (e.g sha256("bitcoin")) solve both valgrind and sanity check issue?

practicalswift commented at 3:26 pm on November 29, 2019: contributor

@Sjors Why not doing a suppression? That seems to be the easiest way to solve the problem this PR is meant to solve :)

There are two ways to do MemorySanitizer suppresions: you can do the suppression in the code using the annotation __attribute__((no_sanitize("memory"))) for the functions you want MSAN-suppressed, or you can use a sanitizer special case blacklist which is perhaps slightly more involved. See https://clang.llvm.org/docs/MemorySanitizer.html#usage for details :)

MarcoFalke commented at 7:03 pm on December 3, 2019: member

NACK. We don’t generally change code to please buggy tool or compiler warnings, especially if the “fix” hides actual bugs in our code base.

Sjors commented at 9:10 am on December 4, 2019: member

@MarcoFalke it’s about more than making tools happy. The tool pointed us to these uninitialized variables, which have caused pretty serious bugs in the recent past and are bad practice. I don’t think we should just ignore this.

practicalswift commented at 9:20 am on December 4, 2019: contributor

@Sjors You haven’t addressed the question about suppressions. Why not suppress this case? :)

laanwj commented at 9:22 am on December 4, 2019: member

I agree with @MarcoFalke and @practicalswift here.

As I said before, if we do anything here it should be a tool-specific suppression, until the tool is fixed.

Sjors commented at 9:36 am on December 4, 2019: member

I’m not against suppressing the warning, but I don’t think that’s enough by itself. It doesn’t make the undefined behavior go away. Perhaps one day the address sanitizer can reason about undefined behavior even through syscall. That’s great, but still not a good reason to keep the undefined behavior.

laanwj commented at 9:37 am on December 4, 2019: member

But it is not undefined behavior, this specific tool is unable to reason through this. If it was actual undefined behavior, we’d respond very differently.

Sjors referenced this in commit 4777647055 on Dec 4, 2019

Sjors force-pushed on Dec 4, 2019

Sjors referenced this in commit 53460b42d6 on Dec 4, 2019

Sjors commented at 12:48 pm on December 4, 2019: member

I added the suppression, as narrowly as possible. I also added comments to the uninitialized variables, pointing back to this discussion so it doesn’t get repeated.

Sjors referenced this in commit 527f3ec989 on Dec 4, 2019

Sjors force-pushed on Dec 4, 2019

MarcoFalke commented at 2:50 pm on December 4, 2019: member

@MarcoFalke it’s about more than making tools happy. The tool pointed us to these uninitialized variables, which have caused pretty serious bugs in the recent past and are bad practice. I don’t think we should just ignore this.

Again, this is a bug in the tool. These variables are not uninitialized when read. Initializing them with the wrong value will cause bugs in the future because tools that properly detect uninitialized reads will no longer yell at you.

This point is repeated every couple of months. Might be a good candidate to add to the dev notes.

Sjors renamed this:
~~Initialize entropy buffers to suppress false positive warning~~
Suppress false positive warning about uninitialized entropy buffers
on Dec 4, 2019

in src/random.cpp:481 in 527f3ec989 outdated

471+// Suppress memory sanitizer because it can't see that syscall initializes this in GetOSRand().
472+#if defined(__has_feature) && defined(HAVE_SYS_GETRANDOM)
473+#  if __has_feature(memory_sanitizer)
474+__attribute__((no_sanitize("memory")))
475+#  endif
476+#endif

MarcoFalke commented at 8:45 pm on December 4, 2019:

This is a clang bug, right? Should link to the commit that fixed it. And since you are compiling clang from source you might as well use that commit and then remove this code blob here.

Sjors commented at 8:57 pm on December 4, 2019:

The memory sanitizer appears to be a Google project, and they don’t expect syscall support to be fixed anytime soon. Only a more narrow case of getrandom was fixed, which is of no use to us: https://github.com/google/sanitizers/issues/852#issuecomment-327305535

MarcoFalke commented at 6:24 pm on December 5, 2019:

Ok, fine. I’d still prefer a suppression in a suppressions file if possible. Modifying the code like this makes it harder to read and potentially impossible to compile on some environments.

Sjors commented at 11:10 am on December 6, 2019:

@practicalswift any idea how to achieve such a suppressions file? I could also put it in a macro, or we can do that if we ever need it again. The outer #if defined(__has_feature) is there to make sure other environments don’t trip over it.

practicalswift commented at 2:02 pm on December 6, 2019:

@Sjors Sure! :) You need to use the sanitizer special case list functionality by passing -fsanitize-blacklist=test/sanitizer_suppressions/msan as part of CFLAGS/CXXFLAGS. You need to create the file test/sanitizer_suppressions/msan following the file format specified here: https://clang.llvm.org/docs/SanitizerSpecialCaseList.html#format

Unfortunately MemorySanitizer only supports suppression lists supplied at compile-time. Some of the other sanitizers allow suppression lists supplied at run-time.

Sjors commented at 2:33 pm on December 6, 2019:

I prefer to punt on this until we need it in more places…

laanwj commented at 11:34 am on December 9, 2019:

I prefer to punt on this until we need it in more places…

So this is the only place where MSAN special treatment is needed? That’s pretty good, I had assumed it would be the first of many.

Sjors commented at 4:07 pm on December 9, 2019:

I don’t know yet. The binary exits once it finds a single problem. The next problem in line is in a different file, but who know what happens after that.

There’s only one place in our codebase where we use syscall, so that’s a reason for optimism.

DrahtBot commented at 5:25 am on December 5, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

practicalswift commented at 8:13 am on December 5, 2019: contributor

@Sjors Thanks for using a suppression instead. Is this the only suppression needed to make MSAN happy in your setup?

Sjors commented at 8:56 am on December 5, 2019: member

@practicalswift no, it fails on other things, but those appear unrelated. I’ll update #17620 later.

DrahtBot added the label Needs rebase on Dec 5, 2019

gmaxwell commented at 1:06 am on December 6, 2019: contributor

It’s not clear to me that everyone understands what’s going on here. And maybe I caused some confusion by say that zero is not an acceptable random value. No constant is an acceptable random value.

The variable is not undefined. It gets defined by the SYS_getrandom. Memcheck has a flaw/shortcoming where it doesn’t know or assume that sys_getrandom initializes it so it falsely reports it as uninitialized.

If it were actually undefined then that would mean that the getrandom call wasn’t doing anything which would be a really serious problem because it would mean that the software getting random numbers it was expecting. Ignoring the other belt-and-suspenders randomness inputs, a failure to read random data into that variable would be a total break resulting in predictable keys.

With the code as it currently is, should some bug be added so that the variable is not initialized at least valgrind will start yelling. If the code is changed to initialize the value with anything except a truly random value (such a zero or some random hash) then if a flaw that skips the randomness call is introduced the software will be just as vulnerable but in that case the flaw will be undetectable by valgrind. Similarly, compilers can often warn when they’re certain a value will be undefined, and pre-emptive dummy initialization will break those warnings.

An assertion would not be appropriate because any value is acceptable random value, so long as it’s actually random and not some predictable constant.

Another potential tool in the debate between “pre-initilize variables and avoid the risk of undefined values” and “don’t pre-init and preserve valgrind/memcheck sensitivity to coding errors that cause the required “real initialization” is that valgrind.h has special macros that can be used to mark memory as undefined in valgrind’s tracking although they’re defined for language purposes. (VALGRIND_MAKE_MEM_UNDEFINED(x,y), see secp256k1/tests.c). I’m not sure if anything similar exists for memcheck.

In some rare cases pre-initilization is a performance concern. But in most– where the variable isn’t in some inner loop and doesn’t involve a bunch of malloc– it’s not. It is, however, very often a testing sensitivity concern. Of course, in some cases the code can be re-factored to do the proper ‘final’ initialization at declaration time then that’s best.

If there were a big swing in the project towards performing dummy initialization I think it would be really useful if it were always done via a macro that would allow disabling it for testing (or valgrind annotating it).

practicalswift commented at 7:31 am on December 6, 2019: contributor

Memcheck has a flaw/shortcoming where it doesn’t know or assume that sys_getrandom initializes it so it falsely reports it as uninitialized.

You mean MemorySanitizer (MSAN) and not Memcheck (Valgrind), right? :)

In some rare cases pre-initilization is a performance concern. But in most– where the variable isn’t in some inner loop and doesn’t involve a bunch of malloc– it’s not. It is, however, very often a testing sensitivity concern.

From a security perspective doesn’t it boil down to a trade-off between maximising testing sensitivity (achieved by not pre-initializing) vs minimising the risk of leaking non-dummy memory content (achieved by pre-initializing)?

Is that a fair summary? :)

Background context for others:

Depending on code generation, a programmer who runs into undefined behavior by using an uninialized automatic variable may observe any previous value (including program secrets), or any value which the compiler saw fit to materialize on the stack or in a register (this could be to synthesize an immediate, to refer to code or data locations, to generate cookies, etc).

build: skip memory sanitizer for SeedSlow

Memory sanitizer cannot see through syscall. This caused a false
positive use-of-uninitialized-value error for SYS_getrandom.

After discussion in #17627 it was decided not to initialize
these variables to 0, but instead to suppress the warning.

31408edeff

Sjors force-pushed on Dec 6, 2019

DrahtBot removed the label Needs rebase on Dec 6, 2019

laanwj added the label Tests on Dec 10, 2019

gmaxwell commented at 12:10 pm on December 21, 2019: contributor

minimising the risk of leaking non-dummy memory content (achieved by pre-initializing)

There is no risk of leaking anything here, and of the randomness is hashed before being used.

The difference between initializing and not is that if a bug is introduced: With initializing a bug that hobbles the random input will not be detectable (except via review), without initializing the bug would be detectable by memcheck but there would also be undefined behaviour (however, on currently supported compilers/systems the undefined behaviour would be harmless esp compared to the no-randomness bug) until someone noticed the error report in memcheck and fixed it.

jonatack commented at 12:34 pm on December 21, 2019: member

FWIW I attempted to summarise this discussion in the review club notes as a resource and will probably update it with the points made here. Don’t hesitate to suggest corrections or improvements in the repository or to me via IRC.

https://bitcoincore.reviews/17639.html#what-can-we-do-to-mitigate-uninitialized-variables

practicalswift commented at 5:26 pm on December 21, 2019: contributor

From a security perspective doesn’t it boil down to a trade-off between maximising testing sensitivity (achieved by not pre-initializing) vs minimising the risk of leaking non-dummy memory content (achieved by pre-initializing)?

There is no risk of leaking anything here, and of the randomness is hashed before being used.

I was talking about the pros and cons of pre-initializing generally: in this specific case there is obviously no risk of leaking sensitive data :)

Recent discussion on Twitter about pre-initialization in general with some people from Apple working on iPhone security and LLVM respectively that might be of interest for readers of this thread: “If you’ve got a security-sensitive codebase, you should be using -ftrivial-auto-init=pattern in Clang. In 2020, there’s no good reason for uninitialized variables to be exploitable.”

I would assume that they do developer builds without -ftrivial-auto-init=pattern (no protection against exploitability but with unchanged testing sensitivity) and release builds with -ftrivial-auto-init=pattern (guards against exploitability but loses testing sensitivity).

Sjors commented at 1:17 pm on March 12, 2020: member

Closing in favor of the more precise fix in #18288. We should still document a policy around initialisation, perhaps requiring either initialisation or a comment that’s intentionally not initialised.

Sjors closed this on Mar 12, 2020

DrahtBot locked this on Feb 15, 2022

Suppress false positive warning about uninitialized entropy buffers #17627

Conflicts