Fix Memory Leak #2671

See Subject.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f584fc268e9efbfcfe524fc88b3a8cf818b2aff5 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.

Can you be more detailed with your commit message like fix memory leak in CKey::SetCompactSignature()?

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e628e185fd524ffd8cf0f1bbd83d21f5e4bda2bb 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.

Alright, can you now please also squash those 3 commits into one.

Don't know how to do that...

On May 19, 2013, at 2:53 PM, Philip Kaufmann notifications@github.com wrote:

Alright, can you now please also squash those 3 commits into one.

— Reply to this email directly or view it on GitHub.

git rebase origin -i replace pick with reword for the first commit and edit the commit message replace pick with squash for commit 2 and 3 git push origin master -f

I'm just ensuring with my comments, that core devs will merge your commit :).

Seems correct at first glance. I'd have to review the functions called in SetCompactSignature() before ACK'ing, to be 100% certain

ACK

I believe there is also a memory leak on Line 331 (same issue).

I can roll that into this patch or create a new one.

On May 19, 2013, at 3:29 PM, Pieter Wuille notifications@github.com wrote:

ACK

— Reply to this email directly or view it on GitHub.

@bytemaster If there are more leaks in key.cpp it would be fine to have fixes in this pull for them IMHO.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/173601705ccf189fd83e3854f71f6a872c6faeda 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.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a9280652ce61ddbbecfe16e18e1e464bb1f5d34d 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.

ACK (though ideally we should be using RAII so that these kind of mistakes cannot be made)

@bytemaster How did you find these problems, just by browsing the code of via some tool?

I was using the source code as an example for creating my own ECC library.

The real question is what kind of security implications are there? Could someone construct transactions to exploit these leaks? I doubt anyone could lose coins, but you could crash all of the nodes on the network with the right kind of trx spam.

On Mon, May 20, 2013 at 11:34 AM, Philip Kaufmann notifications@github.comwrote:

@bytemaster https://github.com/bytemaster How did you find these problems, just by browsing the code of via some tool?

— Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2671#issuecomment-18153974 .

@bytemaster We only use key recovery for verifymessage (the manual message signing stuff), and we only sign in response to user/rpc request, never P2P. Unless I'm missing something here there is no such risk in these cases.

(None the less— the fixes are fantastic and also point out the memleak testing I've been doing recently, which hasn't involved using sign message or creating transactions while under instrumentation, is inadequate. thanks!)

CompactSignature is indeed only used for message signing, so there is no remote vulnerability. Also, see #2600, which does a large refactor of the key.cpp code (it moved EC_KEY into an internal RAII wrapper, though not EC_SIG)

Wouldn't have hurt to pull this for 0.8.2 RC2 ;)?

@Diapolo I'm not eager to pull a fix for a non-network triggerable leak that I can't (easily) reproduce right before a release.