This merge was already done and resolved, but then I had to fix a
previous commit. I then cherry-picked this resolved merge, did the
outstanding PRs from the Elements side, confusing the merge ancestors
and having to "redo" this merge - which just means this one is empty.
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 acf709b3ab,
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.
ubsan doesn't like assigning arbitrary uint8_t values to bool. It's easy
to avoid doing, so do it. (We do this specifically in PSET since that's
Elements-specific code, but the same issue is present in Bitcoin in the
Unserialize impl for bool in serialize.h. Upstream this is only used in
the wallet database, where it may be that non 0/1 values are impossible
(absent a corrupt wallet).
