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

pull arowser wants to merge 1 commits into bitcoin:master from arowser:remove_big_local_var changing 1 files +9 −10

arowser commented at 5:40 AM on May 27, 2014: contributor

Change the big size local var "char pch[200000]" to "new char[200000]" to avoid the possibly stack overflow.
in src/util.cpp:None in 8d9c67bcfa outdated
```
1143 | @@ -1144,7 +1144,7 @@ void ShrinkDebugFile()
1144 |      if (file && boost::filesystem::file_size(pathLog) > 10 * 1000000)
1145 |      {
1146 |          // Restart the file with some of the end
1147 | -        char pch[200000];
1148 | +        char *pch = new char[200000];
1149 |          fseek(file, -sizeof(pch), SEEK_END);
```
laanwj commented at 7:12 AM on May 27, 2014:

sizeof(pch) will be 4 or 8 here. You should use the correct amount (probably best to define a constant for the 200000).

sipa commented at 11:18 AM on May 27, 2014:

If we're going to use dynamic memory, please use a properly managed way (e.g. use a vector).

laanwj commented at 11:23 AM on May 27, 2014:

Switching to dynamic memory makes sense, 200KB is too much to allocate on the stack. It could exhaust the per-thread stack on some platforms. Using a vector sounds good to me.
jgarzik commented at 11:43 AM on May 27, 2014: contributor

Indeed. An RAII vector is superior to something you must remember to manually delete[].
laanwj commented at 10:38 AM on June 3, 2014: member

@arowser are you going to update this to use a vector<char> instead?
arowser commented at 12:47 AM on June 4, 2014: contributor

@laanwj Yes, I'll update it later, sorry for late. I'm have more than ten years experience in c lang, but I'm a c++ new guy, so I'm studying how change char * to vector in c++ and make sure the chang is ok before I push it again, thanks (:
change "char pch[200000]" to "new char[200000]" ea7875e8c3

in src/util.cpp:None in 5ba29bd765 outdated

 168 | @@ -169,15 +169,14 @@ void RandAddSeedPerfmon()
 169 |  #ifdef WIN32
 170 |      // Don't need this on Linux, OpenSSL automatically uses /dev/urandom
 171 |      // Seed with the entire set of perfmon data
 172 | -    unsigned char pdata[250000];
 173 | -    memset(pdata, 0, sizeof(pdata));
 174 | -    unsigned long nSize = sizeof(pdata);
 175 | -    long ret = RegQueryValueExA(HKEY_PERFORMANCE_DATA, "Global", NULL, NULL, pdata, &nSize);
 176 | +    std::vector <char> pdata(250000,0);

sipa commented at 4:19 AM on June 4, 2014:

Use

std::vector<unsigned char>

instead.

laanwj commented at 8:54 AM on June 11, 2014: member

Looks OK, but 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.
BitcoinPullTester commented at 6:56 AM on June 23, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4236_ea7875e8c3da92b0cbddb9c794843cfcf6c0a80c/ for binaries and test log. 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.
laanwj cross-referenced this on Jun 23, 2014 from issue Fix RandAddSeedPerfMon and move large stack objects to heap by laanwj
laanwj commented at 10:02 AM on June 23, 2014: member

Closing in favor of #4392
laanwj closed this on Jun 25, 2014
bitcoin locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4392 Fix RandAddSeedPerfMon and move large stack objects to heap