Skip to content

Conversation

@arowser
Copy link
Contributor

@arowser arowser commented May 27, 2014

Change the big size local var "char pch[200000]" to "new char[200000]" to avoid the possibly stack overflow.

src/util.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

jgarzik commented May 27, 2014

Indeed. An RAII vector is superior to something you must remember to manually delete[].

@laanwj
Copy link
Member

laanwj commented Jun 3, 2014

@arowser are you going to update this to use a vector instead?

@arowser
Copy link
Contributor Author

arowser commented Jun 4, 2014

@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 (:

src/util.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use

std::vector<unsigned char>

instead.

@laanwj
Copy link
Member

laanwj commented Jun 11, 2014

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
Copy link

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
Copy link
Member

laanwj commented Jun 23, 2014

Closing in favor of #4392

@laanwj laanwj closed this Jun 25, 2014
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants