Move events_hasher into RNGState()

sipa commented at 11:18 pm on December 4, 2019: member

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

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

sipa requested review from TheBlueMatt on Dec 5, 2019

sipa requested review from sipsorcery on Dec 5, 2019

sipa added the label Bug on Dec 5, 2019

in src/random.cpp:595 in 7f83c581fb outdated

597-    events_hasher.Write((const unsigned char *)&event_info, sizeof(event_info));
598-    // Get the low four bytes of the performance counter. This translates to roughly the
599-    // subsecond part.
600-    uint32_t perfcounter = (GetPerformanceCounter() & 0xffffffff);
601-    events_hasher.Write((const unsigned char*)&perfcounter, sizeof(perfcounter));
602+void RandAddEvent(const uint32_t event_info)

MarcoFalke commented at 2:07 am on December 5, 2019:

style-nit:

0void RandAddEvent(const uint32_t event_info) noexcept

sipa commented at 2:20 am on December 5, 2019:

In that case we’d also want it in the .h definition.

sipa commented at 2:30 am on December 5, 2019:

Added, also to a few other functions.

MarcoFalke approved

MarcoFalke commented at 2:07 am on December 5, 2019: member

ACK 7f83c581fbb56a582aa147ed50d2f13e0132607e

MarcoFalke commented at 3:02 am on December 5, 2019: member

ACK fbf5ec5d41cbc1ea05f6baa7d1b32f30ff964301 🌵

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK fbf5ec5d41cbc1ea05f6baa7d1b32f30ff964301 🌵
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUghXQwAoCDLTgpi3i37G/cRRPYrnTZM1tqOGrFIIiUTl3D3hzpfAApUR3DuWLGa
 8niSAYB8j7iLvKYhlOj4cMqP7eFr9b/l0UqbWx+9PBJcGrEf/n7yQ5ktoa22pDlRE
 9bRib6SkWck2b9SOJKA9n8wBWfsOwQloIDbrI8l6ioXpZGogziyOY9aWaxlmMtIyq
10PTwk5lr85D8KF85eWmvXZWNOMTjP6cZkUKuiT582D4MskKnUzFvkCgGXk4PMjM/V
11FHFJncj/yCQpZSVqGxceQFku4K4e9kvI51hbkvRbHwifjB+9JcIZnPPIrXxGc/yS
12VT3xivoh6/QAGdly0tpmOYnQJlEcEqZO96iaI65gAsjX/ryUAn24XJwyemRHVA5v
130103ksv3cZNongFQEhiWwX4B5w+1QkRFascafx0KIULcIFsISOyahn23/BuuZs4I
14MO6rBvBmtduiMTS9Y0LXapdUBj5u4mKOYzD8oOVgN5LvnhUAF4zNGeAmw1depmqw
15TJMbk1e+
16=XQf+
17-----END PGP SIGNATURE-----

Timestamp of file with hash 2f8761f11943984d0b6cc42d906e741a8d4112ed5add619f327346cd68d07a42 -

sipsorcery approved

sipsorcery commented at 4:15 am on December 5, 2019: member

ACK fbf5ec5d41cbc1ea05f6baa7d1b32f30ff964301.

Fixes the access violation exceptions on Windows.

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

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 commented at 8:41 am on December 5, 2019: member

ACK fbf5ec5d41cbc1ea05f6baa7d1b32f30ff964301

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

practicalswift commented at 9:02 am on December 5, 2019: contributor

@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 commented at 2:15 pm on December 5, 2019: member

Needs rebase due to #17507 (as expected)

DrahtBot added the label Needs rebase on Dec 5, 2019

DrahtBot commented at 2:29 pm on December 5, 2019: member

promag commented at 2:35 pm on December 5, 2019: member

ACK after rebase.

This guarantees (through the existing GetRNGState() function

Useful comment GetRNGState() regarding thread safety:

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

Tested that this removes the ASan error:

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

Move events_hasher into RNGState() 8bda0960f9

sipa force-pushed on Dec 5, 2019

sipa commented at 5:52 pm on December 5, 2019: member

Rebased.

fanquake removed the label Needs rebase on Dec 5, 2019

MarcoFalke commented at 6:19 pm on December 5, 2019: member

re-ACK 8bda0960f94dfb6462fc810cd61a8a065730eb79 🥈

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 8bda0960f94dfb6462fc810cd61a8a065730eb79 🥈
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUj2gwv/V0fADNVUtMFbOxW1ejThYoP5Ir6sken6DMGwd6DF80Dn1KrQuQa1Dpbr
 8g8wEzcR4pR8/28CZngNLhRa/T0EkPGfVudoATPDiRYnas27nfOBcHzHE3z0hF3ef
 9g+hA9KQb+MJQVFiavjZj/iz/AOSfiNOpB94k7dl6+XrpFtaz5HFp64EAFe3O/SSq
10zjuyXz3K5DCDIJTz0T1/uCrMfxwBWjFpqb5LQyJ5hvxqhpkFiReoWlA4dP7mntQh
11usIzzyJeU0aEE75Pj8QPj9huF08TZk0B8bT75XgSE7nGJd5IDaJv4t/7tzJgE6vY
12NX6oQNdfX7u7el++PBFlRdQqGKKdpQ5FVRUqvkPA1Vfvp1Qp11pvMNnJdHZNBiL9
13Ca3iyXtODYgCQF2R1sr+IiRkCZ7Z+8y8BCBuzXYCGN1kf4hkQWAS9DzEFhBURZSI
148o3TIQgwaOtjmAOmUX616B5t0uKws+Uqdh9RSgewzyFA66Gs6O9z2xZpBc47M8P1
15U7j/5/GJ
16=bQmZ
17-----END PGP SIGNATURE-----

Timestamp of file with hash e2bfe4a67aa719d2065d58a1574be2aaf95e2d291d18d60bb78e1c080e12e664 -

sipsorcery commented at 8:36 pm on December 5, 2019: member

re-ACK 8bda0960f94dfb6462fc810cd61a8a065730eb79.

MarcoFalke referenced this in commit 5d0b7f9e3d on Dec 5, 2019

MarcoFalke merged this on Dec 5, 2019

MarcoFalke closed this on Dec 5, 2019

MarcoFalke referenced this in commit cf43f3f0a8 on Dec 5, 2019

jasonbcox referenced this in commit f9c9a3ff0c on Nov 5, 2020

deadalnix referenced this in commit 929b49252f on Dec 28, 2020

deadalnix referenced this in commit 3977ecf92c on Dec 29, 2020

random-zebra referenced this in commit 93f43f0f81 on Apr 14, 2021

DrahtBot locked this on Dec 16, 2021

Move events_hasher into RNGState() #17670

Conflicts