Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[qt] Fix potential memory leak in newPossibleKey(ChangeCWallet *wallet) #10920

Merged
merged 1 commit into from Nov 17, 2017

Conversation

Projects
None yet
8 participants
@practicalswift
Copy link
Member

practicalswift commented Jul 24, 2017

Fix potential memory leak in newPossibleKey(ChangeCWallet *wallet).

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jul 24, 2017

Is it possible to change this to use a unique_ptr instead?

@promag

This comment has been minimized.

Copy link
Member

promag commented Jul 24, 2017

👍 @sipa suggestion.

Correct me if I'm wrong, but from what I've seen, WalletModelTransaction::keyChange pointer is initialized to null, and only has a value when newPossibleKeyChange is called. So, in the first call, this would delete NULL.

Moreover, in the ~WalletModelTransaction it also delete keyChange which can be null if newPossibleKeyChange is not called.

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Jul 25, 2017

@sipa Sure, I'll fix!

@promag We're doing delete NULL all over the codebase assuming it is a no-op :-)

@promag

This comment has been minimized.

Copy link
Member

promag commented Jul 25, 2017

Yeah it's safe, can be avoided though.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jul 26, 2017

But there's no reason to avoid it..

@practicalswift practicalswift force-pushed the practicalswift:fix-newPossibleKeyChange-memory-leak branch 2 times, most recently Aug 7, 2017

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Aug 7, 2017

Now using unique_ptr. Please review :-)

@practicalswift practicalswift force-pushed the practicalswift:fix-newPossibleKeyChange-memory-leak branch to 446e261 Aug 7, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 7, 2017

utACK 1ce39c59c22a5878b5af6a4ff821db83c988e8b7

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK 446e261. FWIW #10244 (commit "Remove most direct bitcoin calls from qt/walletmodel.cpp") takes this cleanup further, combining key and transaction into a pending transaction object and referencing that through a unique pointer.

@MarcoFalke MarcoFalke added the GUI label Sep 25, 2017

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 8, 2017

utACK 446e261.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 17, 2017

utACK 446e261

@laanwj laanwj merged commit 446e261 into bitcoin:master Nov 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 17, 2017

Merge #10920: [qt] Fix potential memory leak in newPossibleKey(Change…
…CWallet *wallet)

446e261 [qt] Fix potential memory leak in newPossibleKey(ChangeCWallet *wallet) (practicalswift)

Pull request description:

  Fix potential memory leak in `newPossibleKey(ChangeCWallet *wallet)`.

Tree-SHA512: 252d3828133a0d241cc649aed1280e14a5d5ea47b7b2989039cfa5061a8e35183c7f36d7320aa0ac1b4dcab31e584b358dbbb2fe645a412371d0a460878e2b58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.