Avoid UB (member call on nullptr) when failing to read randomness in the startup process #15111

Avoid UB (member call on nullptr) when failing to read randomness in the startup process.

RandFailure is called by GetDevURandom, GetOSRand and GetRandBytes when failing to read randomness for some reason.

This is RandFailure:

[[noreturn]] static void RandFailure()
{
    LogPrintf("Failed to read randomness, aborting\n");
    std::abort();
}

This is LogPrintf:

template <typename... Args>
static inline void LogPrintf(const char* fmt, const Args&... args)
{
    if (g_logger->Enabled()) {
        …
    }
}

Compilers are allowed to optimise away calls to RandFailure that are guaranteed to take place when g_logger == nullptr. Please note that such optimisation would remove the crucial std::abort call in RandFailure.

Failing to read randomness before this patch (under UBSan):

$ src/bitcoind
logging.h:135:19: runtime error: member call on null pointer of type 'BCLog::Logger'
<undefined behaviour>

Failing to read randomness after this patch (under UBSan):

$ src/bitcoind
Failed to read randomness, aborting
$

I fail to see how g_logger could be a nullptr. It is well-defined for the whole duration of main.

About the randomness failure: We should probably not consume bytes before we did a RandomInit()?

Could you clarify how this is happening and maybe include steps to reproduce?

@MarcoFalke After #14955 the initialization of the RNG happens on first use, even before calling RandomInit().

@MarcoFalke Consider the case when the PRNG has not been seeded with enough randomness to ensure an unpredictable byte sequence. RAND_bytes() will then return 0.

Give me a few minutes I'll get back with the exact steps to reproduce.

@MarcoFalke

Steps to reproduce:

$ git clone https://github.com/bitcoin/bitcoin
$ cd bitcoin
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
$ git apply
diff --git a/src/random.cpp b/src/random.cpp
index f8ffda136..a3f84798d 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -274,7 +274,7 @@ void GetOSRand(unsigned char *ent32)

 void GetRandBytes(unsigned char* buf, int num)
 {
-    if (RAND_bytes(buf, num) != 1) {
+    if (true) {
         RandFailure();
     }
 }
$ make
$ UBSAN_OPTIONS="halt_on_error=1:print_stacktrace=1" src/bitcoind
logging.h:135:19: runtime error: member call on null pointer of type 'BCLog::Logger'
    [#0](/bitcoin-bitcoin/0/) 0x564eac67cff2 in void LogPrintf<>(char const*) /.../bitcoin/src/./logging.h:135:19
    [#1](/bitcoin-bitcoin/1/) 0x564eac67c054 in RandFailure() /.../bitcoin/src/random.cpp:53:5
    [#2](/bitcoin-bitcoin/2/) 0x564eac67c07d in GetRandBytes(unsigned char*, int) /.../bitcoin/src/random.cpp:278:9
    [#3](/bitcoin-bitcoin/3/) 0x564eabc15be7 in (anonymous namespace)::CSignatureCache::CSignatureCache() /.../bitcoin/src/script/sigcache.cpp:35:9
    [#4](/bitcoin-bitcoin/4/) 0x564eabc15be7 in __cxx_global_var_init.8 /.../bitcoin/src/script/sigcache.cpp:68
    [#5](/bitcoin-bitcoin/5/) 0x564eabc15be7 in _GLOBAL__sub_I_sigcache.cpp /.../bitcoin/src/script/sigcache.cpp
    [#6](/bitcoin-bitcoin/6/) 0x564eac8125cc in __libc_csu_init (/.../bitcoin/src/bitcoind+0x22185cc)
    [#7](/bitcoin-bitcoin/7/) 0x7f522eaa8b27 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:266
    [#8](/bitcoin-bitcoin/8/) 0x564eabc201d9 in _start (/.../bitcoin/src/bitcoind+0x16261d9)

Please note: before main.

That answers the raised questions?

@MarcoFalke @sipa These are places I've identified (so far) where randomness is requested before main:

1. CSignatureCache ctor (src/script/sigcache.cpp): Calls GetRandBytes(nonce.begin(), 32)

2. SaltedTxidHasher ctor (src/txmempool.cpp): Calls GetRand(std::numeric_limits<uint64_t>::{min,max}()) which calls GetRandBytes(…)

3. static uint256 scriptExecutionCacheNonce(GetRandHash()); (src/validation.cpp): Calls GetRandHash() which calls GetRandBytes((…)

It seems odd that we need those globals (e.g. the mempool) before main even starts. They should probably be initialized later.

@MarcoFalke That would be nice. And LogPrintf(…) should be robust and handle also callers who happen to call it before main, right?

There are generally two ways of dealing with initialization order of globals. One is to make all globals unique_ptrs or otherwise non-instantiated at startup and explicitly create all. Another approach is using std::call_once or static locals like #14955 does.

Since C++11 the second can be done with pretty low overhead and without too big hoops, so that's my preference over explicit initialization as it's less fragile.

@sipa Do you agree that LogPrintf(…) should be robust and handle also callers who happen to call it before main?

What is even the point of g_logger in the first place? It seems to serve no purpose except creating UB when logging is called by global constructors. I propose instead PR#12954 be reverted: It served no obvious purpose and instead introduced a potentially serious misbehaviour.

@MarcoFalke Could this one get a release milestone? :-)

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #15266 (memory: Construct globals on first use by MarcoFalke)

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.

Closing in favour of @MarcoFalke's better fix in #15266. Thanks!