Skip to content

Conversation

@achow101
Copy link
Member

@achow101 achow101 commented Jul 1, 2021

In many places in ScriptPubKeyMan managing code, we assume that the ScriptPubKeyMan being retrieved actually exists and is not a nullptr. Thus removing a ScriptPubKeyMan requires erasing the object from the map rather than setting it to a nullptr.

This fixes a segmentation fault that can be reached with test/functional/wallet_descriptors.py --descriptors

In many places in ScriptPubKeyMan managing code, we assume that the
ScriptPubKeyMan being retrieved actually exists and is not a nullptr.
Thus removing a ScriptPubKeyMan requires erasing the object from the
map rather than setting it to a nullptr.
@fanquake
Copy link
Member

fanquake commented Jul 1, 2021

cc @S3RK

@fanquake fanquake added the Wallet label Jul 1, 2021
@S3RK
Copy link
Contributor

S3RK commented Jul 1, 2021

ACK b945a31
Verified that's the only nullptr assignments from #19651 and #20191
Compiled and ran ./test/functional/wallet_descriptor.py --descriptors locally

I totally missed that, thanks @achow101 for fixing that!

@fanquake fanquake merged commit fa46e48 into bitcoin:master Jul 1, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 1, 2021
…lptr

b945a31 wallet: erase spkmans rather than setting to nullptr (Andrew Chow)

Pull request description:

  In many places in ScriptPubKeyMan managing code, we assume that the ScriptPubKeyMan being retrieved actually exists and is not a nullptr. Thus removing a ScriptPubKeyMan requires erasing the object from the map rather than setting it to a nullptr.

  This fixes a segmentation fault that can be reached with `test/functional/wallet_descriptors.py --descriptors`

ACKs for top commit:
  S3RK:
    ACK b945a31

Tree-SHA512: 344a4cf9b1c168428750c751dcd24c52032506f20c81977fe93c4b5307ea209de72bb62a9c5284820f225b03acdc9573fceb734833d29b82f49d5a799ddcaea7
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants