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.

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

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.

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

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

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.

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

@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 :))

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.

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?

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)

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 :)

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.

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.

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

Echoing the NACK on the asserts :)

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

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.

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

Why? :)

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

1. memory sanity check: = {0}
2. 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).

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.

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.

@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.

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

Conflicts

No conflicts as of last run.

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

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

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).

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).

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.

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

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).

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.