Move events_hasher into RNGState() #17670

sipa · 2019-12-04T23:18:14Z

This moves events_hasher and events_mutex into RNGState() in random.cpp. This guarantees (through the existing GetRNGState() function) that the mutex is always created before any events are added, even when that happens inside global initializers.

Fixes the issue reported here: #17573 (comment), and includes the annotation from #17666).

maflcko

ACK 7f83c58

src/random.cpp

maflcko · 2019-12-05T03:02:48Z

ACK fbf5ec5 🌵

Show signature and timestamp

Signature:

Timestamp of file with hash 2f8761f11943984d0b6cc42d906e741a8d4112ed5add619f327346cd68d07a42 -

sipsorcery

ACK fbf5ec5.

Fixes the access violation exceptions on Windows.

DrahtBot · 2019-12-05T05:05:15Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17627 (Suppress false positive warning about uninitialized entropy buffers by Sjors)
#17507 (random: mark RandAddPeriodic and SeedPeriodic as noexcept by fanquake)

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.

laanwj · 2019-12-05T08:41:54Z

ACK fbf5ec5

How can we avoid these kind of things from creeping in in the future, is there something like a initialization-order-sanitizer?

practicalswift · 2019-12-05T09:02:08Z

@laanwj AddressSanitizer has ASAN_OPTIONS=check_initialization_order=true which I believe we're not using (yet) :) There is also ASAN_OPTIONS=strict_init_order=true.

Another case where better use of available tooling perhaps could help us avoid C++ sharp edges? :)

laanwj · 2019-12-05T14:15:42Z

Needs rebase due to #17507 (as expected)

DrahtBot · 2019-12-05T14:29:32Z

Needs rebase

promag · 2019-12-05T14:35:37Z

ACK after rebase.

This guarantees (through the existing GetRNGState() function

Useful comment GetRNGState() regarding thread safety:

    // This C++11 idiom relies on the guarantee that static variable are initialized
    // on first call, even when multiple parallel calls are permitted.

Tested that this removes the ASan error:

==3770==ERROR: AddressSanitizer: initialization-order-fiasco on address 0x0001058f70a0 at pc 0x000105542404 bp 0x7ffeeb534ef0 sp 0x7ffeeb534ee8
READ of size 8 at 0x0001058f70a0 thread T0
    #0 0x105542403 in CSHA256::Finalize(unsigned char*) sha256.cpp:667
    #1 0x105433ca0 in SeedEvents(CSHA512&) random.cpp:462
    #2 0x1054300f6 in ProcRand(unsigned char*, int, RNGLevel) random.cpp:482
    #3 0x10542f99a in GetRandBytes(unsigned char*, int) random.cpp:576
    #4 0x104bfcce5 in _GLOBAL__sub_I_sigcache.cpp sigcache.cpp:34

sipa · 2019-12-05T17:52:07Z

Rebased.

maflcko · 2019-12-05T18:19:03Z

re-ACK 8bda096 🥈

Show signature and timestamp

Signature:

Timestamp of file with hash e2bfe4a67aa719d2065d58a1574be2aaf95e2d291d18d60bb78e1c080e12e664 -

sipsorcery · 2019-12-05T20:36:15Z

re-ACK 8bda096.

8bda096 Move events_hasher into RNGState() (Pieter Wuille) Pull request description: This moves `events_hasher` and `events_mutex` into `RNGState()` in random.cpp. This guarantees (through the existing `GetRNGState()` function) that the mutex is always created before any events are added, even when that happens inside global initializers. Fixes the issue reported here: #17573 (comment), and includes the annotation from #17666). ACKs for top commit: MarcoFalke: re-ACK 8bda096 🥈 sipsorcery: re-ACK 8bda096. Tree-SHA512: 78702d668764df19e9d61d87d82eca71cceca87d5351b740e13e732a1c18a3d53d7fbaaf63245266da597370bfebec9fa6a4749c15ec5a78dcfe6122c33553ed

1f9d5af tests: Add initialization order fiasco detection in Travis (practicalswift) Pull request description: Add initialization order fiasco detection in Travis :) Context: #17670 (comment) This would have caught the `events_hasher` initialization order issue introduced in #17573 and fixed in #17670. Output in case of an initialization order fiasco: ``` ==7934==ERROR: AddressSanitizer: initialization-order-fiasco on address 0x557098d79200 at pc 0x55709796b9a3 bp 0x7ffde524dc30 sp 0x7ffde524dc28 READ of size 8 at 0x557098d79200 thread T0 #0 0x55709796b9a2 in CSHA256::Finalize(unsigned char*) src/crypto/sha256.cpp:667:25 #1 0x5570978150e9 in SeedEvents(CSHA512&) src/random.cpp:462:19 #2 0x5570978145e1 in SeedSlow(CSHA512&) src/random.cpp:482:5 #3 0x5570978149a3 in SeedStartup(CSHA512&, (anonymous namespace)::RNGState&) src/random.cpp:527:5 #4 0x55709781102d in ProcRand(unsigned char*, int, RNGLevel) src/random.cpp:571:9 #5 0x557097810d19 in GetRandBytes(unsigned char*, int) src/random.cpp:576:59 #6 0x557096c2f9d5 in (anonymous namespace)::CSignatureCache::CSignatureCache() src/script/sigcache.cpp:34:9 #7 0x557096511977 in __cxx_global_var_init.7 src/script/sigcache.cpp:67:24 #8 0x5570965119f8 in _GLOBAL__sub_I_sigcache.cpp src/script/sigcache.cpp #9 0x557097bba4ac in __libc_csu_init (src/bitcoind+0x18554ac) #10 0x7f214b1c2b27 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:266 #11 0x5570965347d9 in _start (src/bitcoind+0x1cf7d9) 0x557098d79200 is located 96 bytes inside of global variable 'events_hasher' defined in 'random.cpp:456:16' (0x557098d791a0) of size 104 registered at: #0 0x557096545dfd in __asan_register_globals compiler-rt/lib/asan/asan_globals.cpp:360:3 #1 0x557097817f8b in asan.module_ctor (src/bitcoind+0x14b2f8b) SUMMARY: AddressSanitizer: initialization-order-fiasco src/crypto/sha256.cpp:667:25 in CSHA256::Finalize(unsigned char*) ``` ACKs for top commit: promag: Tested ACK 1f9d5af, got MarcoFalke: ACK 1f9d5af 👔 Tree-SHA512: f24ac0a313df7549193bd7f4fcfdf9b72bdfc6a6ee31d0b08e6d0752e5108fbd532106b6c86377ae0641258c9adb4921872e5d9a0154c0284e03315e0777102c

Summary: [[bitcoin/bitcoin#17573 | PR17573]]: > Exposes a generic dead-simple "SeedEvent" interface, but currently just used for net messages. This PR caused CI failures on Windows, and was patched the day after its merge with the following: [[bitcoin/bitcoin#17670 | PR17670]] > This moves events_hasher and events_mutex into RNGState() in random.cpp. This guarantees (through the existing GetRNGState() function) that the mutex is always created before any events are added, even when that happens inside global initializers. > > Fixes the issue reported here: [[bitcoin/bitcoin#17573 (comment) | #17573 (comment)]], and includes the annotation from #17666). This is a backport of Core [[bitcoin/bitcoin#17573 | PR17573]] and [[bitcoin/bitcoin#17670 | PR17670]] Test Plan: ``` cmake .. -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_BUILD_TYPE=Debug ninja all check-all ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8287

…Travis Summary: 1f9d5af4f197e7cc0469a0bb25dcbc51dfa537f4 tests: Add initialization order fiasco detection in Travis (practicalswift) Pull request description: Add initialization order fiasco detection in Travis :) Context: bitcoin/bitcoin#17670 (comment) This would have caught the `events_hasher` initialization order issue introduced in #17573 and fixed in #17670. Output in case of an initialization order fiasco: ``` ==7934==ERROR: AddressSanitizer: initialization-order-fiasco on address 0x557098d79200 at pc 0x55709796b9a3 bp 0x7ffde524dc30 sp 0x7ffde524dc28 READ of size 8 at 0x557098d79200 thread T0 #0 0x55709796b9a2 in CSHA256::Finalize(unsigned char*) src/crypto/sha256.cpp:667:25 #1 0x5570978150e9 in SeedEvents(CSHA512&) src/random.cpp:462:19 #2 0x5570978145e1 in SeedSlow(CSHA512&) src/random.cpp:482:5 #3 0x5570978149a3 in SeedStartup(CSHA512&, (anonymous namespace)::RNGState&) src/random.cpp:527:5 #4 0x55709781102d in ProcRand(unsigned char*, int, RNGLevel) src/random.cpp:571:9 #5 0x557097810d19 in GetRandBytes(unsigned char*, int) src/random.cpp:576:59 #6 0x557096c2f9d5 in (anonymous namespace)::CSignatureCache::CSignatureCache() src/script/sigcache.cpp:34:9 #7 0x557096511977 in __cxx_global_var_init.7 src/script/sigcache.cpp:67:24 #8 0x5570965119f8 in _GLOBAL__sub_I_sigcache.cpp src/script/sigcache.cpp #9 0x557097bba4ac in __libc_csu_init (src/bitcoind+0x18554ac) #10 0x7f214b1c2b27 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:266 #11 0x5570965347d9 in _start (src/bitcoind+0x1cf7d9) 0x557098d79200 is located 96 bytes inside of global variable 'events_hasher' defined in 'random.cpp:456:16' (0x557098d791a0) of size 104 registered at: #0 0x557096545dfd in __asan_register_globals compiler-rt/lib/asan/asan_globals.cpp:360:3 #1 0x557097817f8b in asan.module_ctor (src/bitcoind+0x14b2f8b) SUMMARY: AddressSanitizer: initialization-order-fiasco src/crypto/sha256.cpp:667:25 in CSHA256::Finalize(unsigned char*) ``` --- Backport of Core [[bitcoin/bitcoin#17674 | PR17674]] Depends on D8756 Test Plan: cmake -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DENABLE_SANITIZERS=address ninja all check check-functional Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8757

cecbf6c Use secure.h header for secure allocators (Fuzzbawls) d9f67da net: add ifaddrs.h include (fanquake) e906436 build: check if -lsocket is required with *ifaddrs (fanquake) 414f405 rand: only try and use freeifaddrs if available (fanquake) 3a039d6 build: avoid getifaddrs when unavailable (Cory Fields) 77bddd7 Use GetStrongRandBytes in gmp bignum initialization (Fuzzbawls) b70b26f Fix typo in comment in randomenv.cpp (Fuzzbawls) fec460c Put bounds on the number of CPUID leaves explored (Pieter Wuille) 41ab1ff Fix CPUID subleaf iteration (Pieter Wuille) 8a9bbb1 Move events_hasher into RNGState() (Pieter Wuille) 88c2ae5 random: mark RandAddPeriodic and SeedPeriodic as noexcept (fanquake) 81d382f doc: correct random.h docs after bitcoin#17270 (fanquake) f363ea9 Seed RNG with precision timestamps on receipt of net messages. (Matt Corallo) 7d6ddcb Run background seeding periodically instead of unpredictably (Pieter Wuille) 4679181 Add information gathered through getauxval() (Pieter Wuille) 88d97d0 Feed CPUID data into RNG (Pieter Wuille) 8f5b9c9 Use sysctl for seeding on MacOS/BSD (Pieter Wuille) 67de246 Gather additional entropy from the environment (Pieter Wuille) 6142e1f Seed randomness with process id / thread id / various clocks (Pieter Wuille) 7bde8b7 [MOVEONLY] Move cpuid code from random to compat/cpuid (Fuzzbawls) 52b5336 [MOVEONLY] Move perfmon data gathering to new randomenv module (Pieter Wuille) 27cf995 doc: minor corrections in random.cpp (fanquake) fccd2b8 doc: correct function name in ReportHardwareRand() (fanquake) 909473e Fix FreeBSD build by including utilstrencodings.h (Fuzzbawls) 630931f break circular dependency: random/sync -> util -> random/sync (Fuzzbawls) 5eed08c random: remove call to RAND_screen() (Windows only) (fanquake) ada9868 gui: remove OpenSSL PRNG seeding (Windows, Qt only) (fanquake) 22a7121 Fix non-deterministic coverage of test DoS_mapOrphans (Fuzzbawls) 79e7fd3 Add ChaCha20 bench (Jonas Schnelli) 6966aa9 Add ChaCha20 encryption option (XOR) (Jonas Schnelli) 28c9cdb tests: Add script checking for deterministic line coverage (practicalswift) c82e359 test: Make bloom tests deterministic (MarcoFalke) 7b33223 Document strenghtening (Pieter Wuille) 0190dec Add hash strengthening to the RNG (Pieter Wuille) 67e336d Use RdSeed when available, and reduce RdRand load (Pieter Wuille) 4ffda1f Document RNG design in random.h (Pieter Wuille) 2b6381e Use secure allocator for RNG state (Pieter Wuille) 080deb3 Encapsulate RNGState better (Pieter Wuille) 787d72f DRY: Implement GetRand using FastRandomContext::randrange (Pieter Wuille) 5bc2583 Sprinkle some sweet noexcepts over the RNG code (Pieter Wuille) 774899f Remove hwrand_initialized. (Pieter Wuille) 698d133 Switch all RNG code to the built-in PRNG. (Pieter Wuille) 038a45a Integrate util/system's CInit into RNGState (Fuzzbawls) 5f20e62 Abstract out seeding/extracting entropy into RNGState::MixExtract (Pieter Wuille) 298f97c Add thread safety annotations to RNG state (Pieter Wuille) 2326535 Rename some hardware RNG related functions (Pieter Wuille) d76ee83 Automatically initialize RNG on first use. (Pieter Wuille) 1a5dbc5 Don't log RandAddSeedPerfmon details (Pieter Wuille) 32e6c42 Simplify testing RNG code (Fuzzbawls) 972effa Make unit tests use the insecure_rand_ctx exclusively (Fuzzbawls) af52bf5 Use a FastRandomContext in LimitOrphanTxSize (Fuzzbawls) 746d466 Introduce a Shuffle for FastRandomContext and use it in wallet (Fuzzbawls) 1cdf124 Use a local FastRandomContext in a few more places in net (Fuzzbawls) e862564 Make addrman use its local RNG exclusively (Fuzzbawls) 94b2ead Make FastRandomContext support standard C++11 RNG interface (Pieter Wuille) Pull request description: This is a collection of upstream PRs that have been backported to bring our RNG (`src/random`) code more up-to-date. The following upstream PRs have been included here: - bitcoin#12742 - bitcoin#14624 - some of this had already been merged previously - bitcoin#14955 - bitcoin#15250 - bitcoin#15224 - bitcoin#15324 - bitcoin#15296 - bitcoin#15512 - bitcoin#16878 - bitcoin#17151 - bitcoin#17191 - bitcoin#13236 - bitcoin#13314 - bitcoin#17169 - bitcoin#17270 - omitted last commit as our testing framework doesn't support it currently - omitted bitcoin@64e1e02, to be pulled in after our time utility is updated in a separate PR - bitcoin#17573 - bitcoin#17507 - bitcoin#17670 - bitcoin#17527 - bitcoin#14127 - bitcoin#21486 ACKs for top commit: furszy: ACK cecbf6c with a minor nit that can be easily tackled later. random-zebra: rebase utACK cecbf6c and merging... Tree-SHA512: 3463b693cc9bddc1ec15228d264a794f5c2f159073fafa2ccf6e2563abfeb4369e49505f97ca84f2478ca792bd07b66d2cd83c58044d6a0cae6af42d22f5784b

fanquake added the Utils/log/libs label Dec 4, 2019

sipa mentioned this pull request Dec 4, 2019

Seed RNG with precision timestamps on receipt of net messages. #17573

Merged

sipa requested review from TheBlueMatt and sipsorcery December 5, 2019 00:35

sipa added the Bug label Dec 5, 2019

maflcko approved these changes Dec 5, 2019

View reviewed changes

src/random.cpp Outdated Show resolved Hide resolved

sipsorcery approved these changes Dec 5, 2019

View reviewed changes

practicalswift mentioned this pull request Dec 5, 2019

random: Add missing thread-safety annotations for events_hasher #17666

Closed

sipa mentioned this pull request Dec 5, 2019

random: mark RandAddPeriodic and SeedPeriodic as noexcept #17507

Merged

practicalswift mentioned this pull request Dec 5, 2019

tests: Add initialization order fiasco detection in Travis #17674

Merged

DrahtBot added the Needs rebase label Dec 5, 2019

Move events_hasher into RNGState()

8bda096

sipa force-pushed the 201912_events_rngstate branch from fbf5ec5 to 8bda096 Compare December 5, 2019 17:50

fanquake removed the Needs rebase label Dec 5, 2019

maflcko merged commit 8bda096 into bitcoin:master Dec 5, 2019

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

Naviabheeman mentioned this pull request Apr 8, 2020

Remove Openssl dependency from Tapyrus chaintope/tapyrus-core#61

Closed

Fuzzbawls mentioned this pull request Mar 31, 2021

[Refactor] Update RNG code from upstream PIVX-Project/PIVX#2278

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Move events_hasher into RNGState() #17670

Move events_hasher into RNGState() #17670

Uh oh!

sipa commented Dec 4, 2019

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

maflcko commented Dec 5, 2019

Uh oh!

sipsorcery left a comment

Uh oh!

DrahtBot commented Dec 5, 2019 •

edited

Loading

Uh oh!

laanwj commented Dec 5, 2019

Uh oh!

practicalswift commented Dec 5, 2019 •

edited

Loading

Uh oh!

laanwj commented Dec 5, 2019

Uh oh!

DrahtBot commented Dec 5, 2019

Uh oh!

promag commented Dec 5, 2019 •

edited

Loading

Uh oh!

sipa commented Dec 5, 2019

Uh oh!

maflcko commented Dec 5, 2019

Uh oh!

sipsorcery commented Dec 5, 2019

Uh oh!

Uh oh!

Move events_hasher into RNGState() #17670

Move events_hasher into RNGState() #17670

Uh oh!

Conversation

sipa commented Dec 4, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Dec 5, 2019

Uh oh!

sipsorcery left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Dec 5, 2019

Uh oh!

practicalswift commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Dec 5, 2019

Uh oh!

DrahtBot commented Dec 5, 2019

Uh oh!

promag commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 5, 2019

Uh oh!

maflcko commented Dec 5, 2019

Uh oh!

sipsorcery commented Dec 5, 2019

Uh oh!

Uh oh!

DrahtBot commented Dec 5, 2019 •

edited

Loading

practicalswift commented Dec 5, 2019 •

edited

Loading

promag commented Dec 5, 2019 •

edited

Loading