wallet: Prevent segfault when sending to unspendable witness #13351

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1806-walletUnspendableWitnessIsMine changing 2 files +27 −0

MarcoFalke commented at 7:51 PM on May 30, 2018: member

Previously we wouldn't care about the txnouttype, but after 4e91820531889e309dc4335fe0de8229c6426040 we switch on the type.
wallet: Prevent segfault when sending to unspendable witness fa36aa7965
MarcoFalke added the label Wallet on May 30, 2018
in src/script/standard.cpp:117 in fa36aa7965
```
 113 | @@ -114,6 +114,7 @@ bool Solver(const CScript& scriptPubKey, txnouttype& typeRet, std::vector<std::v
 114 |              vSolutionsRet.push_back(std::move(witnessprogram));
 115 |              return true;
 116 |          }
 117 | +        typeRet = TX_NONSTANDARD;
```
promag commented at 7:58 PM on May 30, 2018:

IMO this could be in line 86 above where vSolutionsRet is also cleared/initialized. It also prevents the same problem if in the future early failure returns are added.

Empact commented at 11:16 PM on May 30, 2018:

Could also drop lines 154 & 155 if you move this up.

MarcoFalke commented at 6:06 PM on June 4, 2018:

Could also drop lines 154 & 155 if you move this up.

That would just make it more fragile. As I understand this function signature, it is meant to return a "pair" of typeRet and vSolutionsRet. Setting that pair should happen as close to the return statement as possible (regardless of early-return or not).

MarcoFalke commented at 6:07 PM on June 4, 2018:

IMO this could be in line 86 above where vSolutionsRet is also cleared/initialized. It also prevents the same problem if in the future early failure returns are added.

This will silence valgrind and thus hide the same problem in the future instead of preventing it.

MarcoFalke commented at 6:28 PM on June 4, 2018:

I could add a vSolutionsRet.clear(); in this line, if people prefer...

promag commented at 11:32 PM on June 4, 2018:

I just consider best practice to reset outputs in the begin if the function has multiple exits. LGTM still.
promag commented at 8:04 PM on May 30, 2018: member

utACK fa36aa7.
sipa commented at 11:08 PM on May 30, 2018: member

utACK fa36aa79655f7ec76043976f3c1c207afa9bfd6f, with or without @promag's suggestion.
Empact commented at 11:18 PM on May 30, 2018: member

utACK fa36aa7
practicalswift commented at 8:45 AM on May 31, 2018: contributor

utACK fa36aa7 modulo @promag:s suggestion – putting it above is more robust.

How was this issue found?

Would running the test suite under valgrind have catched this, or was this condition simply not covered by tests?
MarcoFalke commented at 9:44 AM on May 31, 2018: member

@practicalswift The gui crashes on point and the tests I added in this commit fail valgrind.
MarcoFalke referenced this in commit 2140f6cbc5 on Jun 5, 2018
MarcoFalke merged this on Jun 5, 2018
MarcoFalke closed this on Jun 5, 2018
MarcoFalke deleted the branch on Jun 5, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

Labels