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).
A native macOS task does not aware of Linux container settings, and it
does not use the `depends_built_cache`.
Github-Pull: #25444
Rebased-From: 8e017f3288
Windows native builds are really a best effort, but not a blocker.
Let's keep running them to see what we can fix, but keeping a green
pipeline. Also disabled a couple of long-running tests due to being
close to the timeout
When we have an invalid program, use a zero CMR and create a valid
control block/taptweak. Otherwise we fail to hit the Simplicity logic at
all with bad programs.
#
# You are committing on CAMUS
#
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch 2024-12--simple-fuzz
# Changes to be committed:
# modified: src/test/fuzz/simplicity_tx.cpp
#
# Untracked files:
# reduced-corpus/
# run-fuzz-merge-dir-CPzq/
# run-fuzz-merge-dir-sscf/
#
The first fuzztest takes a Simplicity program and a transaction and
directly calls the Simplicity interpreter with some context cobbled
together from the transaction. It also tries messing with the budget
and computes AMRs to check that the AMR-check works, even though on
the blockchain AMRs are never used.
It also attempts mangling programs to directly fuzz the parser, type
inference and CMR checking.
THIS test, on the other hand, takes a transaction, looks for Simplicity
programs (or witnesses which look like Simplicity programs), computes
their CMRs to produce a correct corresponding scriptPubKey, creates
scriptchecks, and executes them. This should do an end-to-end coverage
of the whole Simplicity consensus logic, including all the new branches
in interpreter.cpp.
To produce seeds for this, I have a a local fuzz target which uses
rust-simplicity and rust-elements to produce programs, deep Taproot trees,
and transactions. I run this to get high coverage, then dump the
resulting complete transactions to disk, where they can be used as
seeds for this test.
This fuzz target takes its seeds in a simple and well-defined format: a
four-byte LE budget, then a transaction, Simplicity program and witness,
each prefixed by a four-byte LE length. The fuzz target extracts any
additional randomness it needs from the txid of the first input of the
transaction, since this data is not interpreted in any other way we
therefore won't confuse the fuzzer.
The reason for this design, rather than a more typical "just query the
fuzzer when you need stuff", is to make it possible to fairly easily
generate test vectors from sources other than this fuzz test. (For
example, I have an alternate target which uses Rust code to generate
well-formed Simplicity programs, which quickly gets high coverage at the
expense of being an unmaintainable mess.)
This commit includes a .c file with a small function to comute the AMR
of a program. This is needed to pass a correct AMR to the Simplicity
interpreter, to exercise all the AMR-checking paths. In practice this is
not really necessary; Elements passes NULL to disable these AMR checks.
* feeDelta tracked the delta (to be applied on top of the actual fee)
* m_modified_fee tracks the actual fee with the delta included
* Instead of passing in the new total delta to the Updater, pass in by
how much the total delta should be modified.
This is needed for the next commit, but makes sense on its own because
the same is done by UpdateDescendantState and UpdateAncestorState.
Cherry-pick of fa52cf8e11https://github.com/bitcoin/bitcoin/pull/23418 (1/2)
