Fix RandAddSeedPerfMon and move large stack objects to heap #4392

laanwj · 2014-06-23T10:01:51Z

Rebased version of #4236 by @arowser, changed to use begin_ptr from #4293. Also use vXXXX variable name syntax for the vectors...

Testing: I'm a bit wary to merge as it affects entropy collection for randomness. I'd like someone with windows to verify that this does the right thing (so, start with -debug=rand and look for the RandAddSeed() XXX bytes message).

laanwj · 2014-06-23T14:30:39Z

Tried testing on wine, but the RegQueryValueExA(HKEY_PERFORMANCE_DATA) fails there :( (no such key)

laanwj · 2014-06-23T15:19:17Z

A bitcoind.exe to test with is available here: https://download.visucore.com/bitcoin/bitcoin-win-v0.9.99.0-gbe873f6.zip (gpg sigs https://download.visucore.com/bitcoin/bitcoin-win-v0.9.99.0-gbe873f6.zip.sig)

leofidus · 2014-06-23T18:53:34Z

Testing on Windows 8 64bit with @laanwj's binary, my log shows

2014-06-23 18:04:36 RandAddSeedPerfmon: Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code 234

I'm not sure how to further debug this. My regular Bitcoin 0.9.1 GUI does not seem to show any randomness-related debug messages (started with -debug).

laanwj · 2014-06-24T07:03:45Z

@leofidus Thanks for testing!

The regular client doesn't show any messages if the call fails, only if it succeeds. In the second commit of this pull I added that warning. So - it's failing in 0.9.1 as well.

But the error is different than mine on wine. Error 234 is ERROR_MORE_DATA. I doubt this ever worked at all.
According to the doc:

If the buffer specified by lpData parameter is not large enough to hold the data, the function returns ERROR_MORE_DATA and stores the required buffer size in the variable pointed to by lpcbData. In this case, the contents of the lpData buffer are undefined.

~~So handle this error properly we need to resize the buffer to the returned size and re-call the function.~~ Edit: no, that doesn't work specifically for HKEY_PERFORMANCE_DATA according to the above doc.

Diapolo · 2014-06-24T07:22:24Z

My official 0.9.1 client shows, that it's working here 2014-06-24 07:19:08 RandAddSeed() 219352 bytes. Didn't yet try this patch...

laanwj · 2014-06-24T08:29:35Z

@Diapolo Thanks for testing, although at this point I doubt that it depends on this patch. So at least it works in some cases.

Do you perhaps know if HKEY_PERFORMANCE_DATA returns a variable number of bytes that may be >250000?

Ah, got it:

If hKey specifies HKEY_PERFORMANCE_DATA and the lpData buffer is not large enough to contain all of the returned data, RegQueryValueEx returns ERROR_MORE_DATA and the value returned through the lpcbData parameter is undefined. This is because the size of the performance data can change from one call to the next. In this case, you must increase the buffer size and call RegQueryValueEx again passing the updated buffer size in the lpcbData parameter. Repeat this until the function succeeds. You need to maintain a separate variable to keep track of the buffer size, because the value returned by lpcbData is unpredictable.

So we need a loop.

Diapolo · 2014-06-24T09:17:42Z

@laanwj See http://msdn.microsoft.com/en-us/library/windows/desktop/ms724911%28v=vs.85%29.aspx for sample code.

Diapolo · 2014-06-24T09:33:04Z

@laanwj We had a debate about that part of the code in 09. 2012, see https://bitcointalk.org/index.php?topic=113496.msg1225880#msg1225880

Diapolo · 2014-06-24T09:43:37Z

@laanwj What do you think about removing that source of entropy and use the debug.log with RAND_load_file() (https://www.openssl.org/docs/crypto/RAND_load_file.html)? The data should be quite random after the initial startup.

laanwj · 2014-06-24T10:07:34Z

Wut? It is known since 2012? I'm extremely disappointed and surprised here. Why did no one ever fix that or escalate it beyond a silly forum topic? Getting the performance data properly is simple, an example is even given in the MSDN documentation that I link above.

I don't think debug.log is a good source of entropy as it is - up to a point - controllable by the outside world through the network. But I'm not an expert on that. If you implement an extra entropy source for that be sure to just use part of the file, not all of it, or it could slow down start-up.

Diapolo · 2014-06-24T10:54:26Z

@laanwj I proposed a change (https://bitcointalk.org/index.php?topic=113496.msg1228012#msg1228012) and if I remember correctly also had a pull open for this. It was decided to leave as is.

laanwj · 2014-06-24T11:35:01Z

That makes no sense. I can only find one issue with RandAddSeedPerfmon in it: this one.
Anyhow, let's fix it...
Edit: I see this issue #2942. But it doesn't mention a specific problem with any of our current entropy harvesting functions.

Diapolo · 2014-06-24T14:10:44Z

Are you going to rework this to include a loop? I tend to just remove it, but have no strong enough feeling yet.

laanwj · 2014-06-24T14:14:23Z

Yes, I'm going to turn it into a loop. We have a dearth of entropy sources on windows so removing any sounds extremely unwise.

laanwj · 2014-06-24T15:02:40Z

Ok, it should always collect now unless there is >10MB of performance data.

A bitcoind.exe to test with is available here: https://download.visucore.com/bitcoin/bitcoin-win-v0.9.99.0-g3b8c1da.zip (gpg sigs https://download.visucore.com/bitcoin/bitcoin-win-v0.9.99.0-g3b8c1da.zip.sig)

laanwj · 2014-06-25T08:20:40Z

Again, can someone with windows test the above bitcoin-win-v0.9.99.0-g3b8c1da.zip and start with -debug=rand and look for the RandAddSeedPerfmon() XXX bytes message in debug.log?

Diapolo · 2014-06-26T13:00:11Z

src/util.cpp

+        static bool warned = false; // Warn only once
+        if (!warned)
+        {
+            LogPrintf("%s: Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code %i\n", __func__, ret);


Nit: Can we define to add a space before the function name :)? Also should this be part of the "rand" category or not? Which is a thing I need to consider with my OpenSSL related pulls also...

I don't understand, the function name is at the beginning of the line, adding a space there makes no sense.

And this is not in the rand category on purpose because people need to see it. It is not just for debugging. And it only emits the warning once anyway.

Just talking about this changed to this anyway:
LogPrintf("%s : Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code %i\n", __func__, ret);

We seem to use that syntax more often.

That's inconsistent too. The second : has no space before it. Also looks strange to me. In English you write like this: there is never a space before a (semi)colon. Anyhow, I'm not going to waste time discussing the formatting of log messages, sorry.

Diapolo · 2014-06-26T13:09:08Z

I'm currently testing this...

Currently we use a fixed buffer of 250000 bytes to request HKEY_PERFORMANCE_DATA. In many cases this is not enough, causing the entropy collection to be skipped. Use a loop that grows the buffer as specified in the RegQueryValueEx documentation: http://msdn.microsoft.com/en-us/library/windows/desktop/ms724911%28v=vs.85%29.aspx (as the size of the performance data can differ for every call, the normal solution of requesting the size then allocating that can't work)

Diapolo · 2014-06-26T13:36:08Z

2014-06-26 13:16:33 RandAddSeedPerfmon : 215208 bytes
2014-06-26 13:26:33 RandAddSeedPerfmon : 205416 bytes

Seems to work :).

ACK

laanwj · 2014-06-26T15:09:46Z

Thanks for testing. I added the 0 initializations. Ready for merge now.

8ae973c Allocate more space if necessary in RandSeedAddPerfMon (Wladimir J. van der Laan) be873f6 Issue warning if collecting RandSeed data failed (Wladimir J. van der Laan) fcb0a1b change "char pch[200000]" to "new char[200000]" (daniel)

BitcoinPullTester · 2014-06-26T16:38:21Z

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4392_8ae973c00cfb02161bb2c2a6c839e510cd409278/ for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

change "char pch[200000]" to "new char[200000]"

fcb0a1b

laanwj mentioned this pull request Jun 23, 2014

change "char pch[200000]" to "new char[200000]" #4236

Closed

Issue warning if collecting RandSeed data failed

be873f6

Diapolo mentioned this pull request Jun 24, 2014

[Qt] seed OpenSSL PNRG with Windows event data #4399

Merged

laanwj changed the title ~~change "char pch[200000]" to "new char[200000]" (rebased)~~ Fix RandAddSeedPerfMon and move large stack objects to heap Jun 24, 2014

Diapolo reviewed Jun 26, 2014
View reviewed changes

laanwj merged commit 8ae973c into bitcoin:master Jun 26, 2014

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Fix RandAddSeedPerfMon and move large stack objects to heap #4392

Fix RandAddSeedPerfMon and move large stack objects to heap #4392

Uh oh!

Conversation

laanwj commented Jun 23, 2014

Uh oh!

laanwj commented Jun 23, 2014

Uh oh!

laanwj commented Jun 23, 2014

Uh oh!

leofidus commented Jun 23, 2014

Uh oh!

laanwj commented Jun 24, 2014

Uh oh!

Diapolo commented Jun 24, 2014

Uh oh!

laanwj commented Jun 24, 2014

Uh oh!

Diapolo commented Jun 24, 2014

Uh oh!

Diapolo commented Jun 24, 2014

Uh oh!

Diapolo commented Jun 24, 2014

Uh oh!

laanwj commented Jun 24, 2014

Uh oh!

Diapolo commented Jun 24, 2014

Uh oh!

laanwj commented Jun 24, 2014

Uh oh!

Diapolo commented Jun 24, 2014

Uh oh!

laanwj commented Jun 24, 2014

Uh oh!

laanwj commented Jun 24, 2014

Uh oh!

laanwj commented Jun 25, 2014

Uh oh!

Diapolo Jun 26, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 26, 2014

Choose a reason for hiding this comment

Uh oh!

Diapolo Jun 26, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 26, 2014

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Jun 26, 2014

Uh oh!

Diapolo commented Jun 26, 2014

Uh oh!

laanwj commented Jun 26, 2014

Uh oh!

BitcoinPullTester commented Jun 26, 2014

Uh oh!

Uh oh!