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

wallet: Set encrypted_batch to nullptr after delete. Avoid double free in the case of NDEBUG. #14138

Merged
merged 1 commit into from Sep 10, 2018

Conversation

Projects
None yet
7 participants
@practicalswift
Copy link
Member

commented Sep 3, 2018

Set encrypted_batch to nullptr after delete. Avoid double free in the case of NDEBUG.

@fanquake fanquake added the Wallet label Sep 3, 2018

@domob1812

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

utACK fa462b3.

I wonder, though: Are we "explicitly" supporting running a binary compiled with NDEBUG? If so, then we should probably not use assert(false) in the two places here (and instead explicitly print an error message and exit using other means).

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

@domob1812 We explicitly do not support that, but at the same time the code should not contain strange bugs when run that way. This is not an argument against assert(false)-- that should be used to protect invariants where if violated potentially things would happen elsewhere.

@domob1812

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

@gmaxwell: Thanks for the clarification, that makes sense. I agree that we should still fix bugs like this.

@donaloconnor

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

utACK fa462b3

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #14144 (Refactoring: Clarify code using encrypted_batch in CWallet by domob1812)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

utACK fa462b3

ken2812221 pushed a commit to ken2812221/bitcoin that referenced this pull request Sep 10, 2018

Merge bitcoin#14138: wallet: Set encrypted_batch to nullptr after del…
…ete. Avoid double free in the case of NDEBUG.

fa462b3 wallet: Set encrypted_batch to nullptr after delete. Avoid double free in the case of NDEBUG. (practicalswift)

Pull request description:

  Set `encrypted_batch` to `nullptr` after delete. Avoid double free in the case of `NDEBUG`.

Tree-SHA512: 6f5ab40c82dd8c8713bbf1aacacdc837277c04769807f985248546be1c7ea269813c95379fbef982ac5683a45af0225613460a7446c39673b033f5f5edde2f5a

@laanwj laanwj merged commit fa462b3 into bitcoin:master Sep 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.