Implicit temporary CTransaction kept and used as reference after delete #5715

1. 
   luke-jr commented at 5:45 AM on January 27, 2015: member

   At least 3 locations in the current codebase (including 0.10) use constructs like:

   VerifyScript(scriptSig, prevPubKey, flags, SignatureChecker(CMutableTransaction, i))
   

   However, this causes an implicit copy of the CMutableTransaction to a temporary CTransaction, which SignatureChecker then saves a reference to. When the constructor completes, (at least within defined behaviour) the temporary is deleted and the SignatureChecker (if used) will still access it by reference.

   At least GCC 4.8 does not warn about this, and silently compiles it. I have seen a "random" crash as a result (in modified code), but cannot systematically reproduce it.

   Proposed solutions:

   1. Make CMutableTransaction a subclass of CTransaction. This means making GetHash a virtual, which harms performance in hot code (NACK).

   2. Make an alternative VerifyScript construct the SignatureChecker. This means it is once again aware of the SignatureChecker constructor parameters (NACK).

   3. Make SignatureChecker a template class able to work on either CTransaction or CMutableTransaction. This puts templates into consensus code (NACK).

   4. Make a separate MutableSignatureChecker class for CMutableTransactions. No known issues with this approach. (this seems okay)

   5. Make SignatureChecker store a copy of the CTransaction it is constructed with. This may have performance hits in hot code (NACK?).

   Besides fixing this, it should be made harder to accidentally trigger this behaviour by making SignatureChecker reject CMutableTransaction. This can be done by making a private constructor with CMutableTransaction, or by disallowing implicit conversions from CMutableTransaction to CTransaction.

2. laanwj added this to the milestone 0.10.0 on Jan 27, 2015
3. laanwj added the label Bug on Jan 27, 2015
4. laanwj added the label Priority High on Jan 27, 2015
5. 
   laanwj commented at 6:37 AM on January 27, 2015: member

   For now I'd propose #5717.

   Besides fixing this, it should be made harder to accidentally trigger this behaviour by making SignatureChecker reject CMutableTransaction. This can be done by making a private constructor with CMutableTransaction, or by disallowing implicit conversions from CMutableTransaction to CTransaction.

   That'd just be a workaround. Allowing conversions is fine. The problem is storing a reference instead of an explicit pointer, relying on c++'s intricacies with regard to temporaries to manage object lifetime.

6. 
   laanwj commented at 9:51 AM on February 3, 2015: member

   Fixed by #5719

7. laanwj closed this on Feb 3, 2015

   ---

8. MarcoFalke locked this on Sep 8, 2021

Milestone
0.10.0