wallet: Prevent segfault when sending to unspendable witness #13351

1. 
   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.
2. wallet: Prevent segfault when sending to unspendable witness fa36aa7965
3. MarcoFalke added the label Wallet on May 30, 2018
4. 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.
5. 
   promag commented at 8:04 pm on May 30, 2018: member

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

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

   utACK fa36aa7
8. 
   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?

9. 
   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.
10. MarcoFalke referenced this in commit 2140f6cbc5 on Jun 5, 2018
11. MarcoFalke merged this on Jun 5, 2018
12. MarcoFalke closed this on Jun 5, 2018

    ---

13. MarcoFalke deleted the branch on Jun 5, 2018
14. DrahtBot locked this on Sep 8, 2021

Contributors
MarcoFalke promag Empact sipa practicalswift

Labels
Wallet