Skip to content

Commit 0bff9b0

Browse files
committed
psbt: guard against empty PSBT transaction
There is a crashing bug in psbt.h which works as follows. This occurs in the psbt_deserialize_input fuzz test, which deserializes an input and then tries to reserialize it. Here a PSET input may contain a mainchain transaction, which is where our trouble is. The process is as follows: 1. On line 901, we create a CTransactionRef, which is a newtype around std::shared_ptr<CTransaction> which defaults to being null. 2. On line 903 we then call `UnserializeFromVector` to populate this, where this is a helper function which attempts to read some number of objects from a byte vector, i.e. a length-prefixed blob. 3. HOWEVER, `UnserializeFromVector` when given an empty vector, decides that it has successfully deserialized zero elements, and returns. 4. Then, on line 904 we assign the CTransactionRef, which is a valid std::shared_ptr whose internal pointer is NULL, to `m_peg_in_tx`, which is a variant of monostate, Bitcoin::CTransactionRef, and CTransactionRef. Its variant changes from the default monostate to CTransactionRef. 5. Then, on line 419, we call `std::get_if<CTransactionRef>` on this object, which returns a std::optional<CTransactionRef>. Because `m_peg_in_tx` is in the `CTransactionRef` variant, this succeeds, returning a true std::optional containing a valid std::shared_ptr which contains a NULL pointer. 6. Then, on line 420, we call `if (peg_in_tx)`, which is true, because we have a true std::optional. We then dereference it on line 423, which is perfectly legal, to get our std::shared_ptr, and pass this shared pointer to SerializeToVector. 7. SerializeToVector passes through like 6 layers of serialize.h obfuscation and eventually dereferences the shared pointer, but because it's NULL, this is a NULL pointer dereference, and we get a crash. There are two lessons here: 1. Don't use C++. As I say in acf709b, where I introduced some of the offending code here (but only by replacing boost stuff with their STL equivalents; the bug existed before I did this), "what a trainwreck of a language". 2. Don't use `UnserializeFromVector` and expect it to throw if the data you're deserializing is malformed. If it's malformed in the sense of being empty, it will "succeed" and silently do nothing. I glanced at every other instance of UnserializeFromVector to check what will happen when it's passed an empty string. I believe there are no other cases where it will fail to initialize a NULL pointer, so I believe that this will not cause other crashes. But I also believe that the behavior in this case is almost always wrong and that we parse malformed PSETs in crazy and incorrect ways all over the place. If anybody has a problem with this, I encourage you to go review the Bitcoin PSBT2 PRs, which this stuff is based on, which have been languishing in rebase hell for the better part of a decade. Don't blame the author for not writing perfect code in a hostile language with no support. After this PR I ran the fuzzer for 16 hours on a 192-thread machine (so 3072 CPU-hours) and didn't see any more crashes.
1 parent f7826f9 commit 0bff9b0

1 file changed

Lines changed: 6 additions & 2 deletions

File tree

src/psbt.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -901,12 +901,16 @@ struct PSBTInput
901901
Sidechain::Bitcoin::CTransactionRef tx;
902902
OverrideStream<Stream> os(&s, s.GetType(), s.GetVersion());
903903
UnserializeFromVector(os, tx);
904-
m_peg_in_tx = tx;
904+
if (tx) {
905+
m_peg_in_tx = tx;
906+
}
905907
} else {
906908
CTransactionRef tx;
907909
OverrideStream<Stream> os(&s, s.GetType(), s.GetVersion());
908910
UnserializeFromVector(os, tx);
909-
m_peg_in_tx = tx;
911+
if (tx) {
912+
m_peg_in_tx = tx;
913+
}
910914
}
911915
break;
912916
}

0 commit comments

Comments
 (0)