Skip to content

psbt: fix nil pointer panic in TaprootLeafScript serialization - #2498

Open
olegnazarov23 wants to merge 1 commit into
btcsuite:masterfrom
olegnazarov23:fix/psbt-nil-taproot-leaf-panic
Open

psbt: fix nil pointer panic in TaprootLeafScript serialization#2498
olegnazarov23 wants to merge 1 commit into
btcsuite:masterfrom
olegnazarov23:fix/psbt-nil-taproot-leaf-panic

Conversation

@olegnazarov23

Copy link
Copy Markdown

Summary

Fix a nil pointer dereference panic when calling Packet.B64Encode() (or Serialize) on a PSBT that contains a nil entry in PInput.TaprootLeafScript.

Problem

If TaprootLeafScript contains a nil *TaprootTapLeafScript entry:

packet.Inputs[0].TaprootLeafScript = []*TaprootTapLeafScript{nil}

Calling B64Encode() panics during the sort.Slice call or when accessing leafScript.Script, instead of returning an error.

Fix

  • partial_input.go: Add nil check before sorting and serializing TaprootLeafScript entries. Returns ErrInvalidPsbtFormat if a nil entry is found.
  • utils.go: Add nil guard in FindLeafScript to skip nil entries instead of panicking.
  • psbt_test.go: Add regression test TestSerializeNilTaprootLeafScript.

Fixes #2495

When a PInput contains a nil entry in its TaprootLeafScript slice,
calling Serialize (via B64Encode) panics with a nil pointer dereference
during the sort or field access.

Add a nil check before sorting and serializing TaprootLeafScript
entries, returning ErrInvalidPsbtFormat instead of panicking. Also
add a nil guard in FindLeafScript to skip nil entries.

Fixes btcsuite#2495

@Lrifton92 Lrifton92 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clean diagnosis here — the root cause analysis is spot on and the panic is definitely still live. I reproduced it on current master (1966c384) with a throwaway test against the psbt/ module: p.Inputs[0].TaprootLeafScript = []*TaprootTapLeafScript{nil} followed by B64Encode() still panics with invalid memory address or nil pointer dereference.

A few observations that might be useful, mostly about how this sits relative to the other open work on the same bug.

The patch targets a tree that is no longer on master

This PR modifies btcutil/psbt/, but that directory was removed from master in 258049c9 (merge of #1825, "btcec-v2-no-circular-dep", 2026-05-14). This PR's merge base (1c55c7c1, 2026-03-10) predates that removal, which is why the diff still renders cleanly.

$ git ls-tree -d origin/master btcutil/ | awk '{print $4}'
btcutil/bloom
btcutil/coinset
btcutil/gcs
btcutil/hdkeychain
btcutil/txsort

The live package is now the top-level psbt/ directory, published as its own module github.com/btcsuite/btcd/psbt/v2 and using the /v2 submodule import paths (wire/v2, txscript/v2). So this would need a retarget rather than a plain rebase.

The two hunks apply opposite semantics to the same invariant

In partial_input.go a nil entry is rejected (return ErrInvalidPsbtFormat), but in FindLeafScript it is skipped (continue). Worth aligning these one way or the other.

On the continue specifically: with a nil present, FindLeafScript now returns "leaf script for target leaf hash %x not found in input", which points a caller at a missing-leaf problem when the actual problem is a malformed input. That error then propagates into finalizeTaprootInput's "control block for script spend signature not found", compounding the misdirection. An error naming the nil entry and its index would keep the diagnosis at the fault site.

Mapping the actual panic surface

I instrumented this on master — a probe that plants a nil in each taproot slice and records the top psbt frame from runtime/debug.Stack(). Nine distinct source lines panic, not the handful the various PRs address:

# line function
1 partial_input.go:503 PInput.serialize (scriptSpend.XOnlyPubKey)
2 partial_input.go:524 PInput.serialize (leafScript.Script)
3 taproot.go:229 SerializeTaprootBip32Derivation (len(d.LeafHashes))
4 taproot.go:49 (*TaprootScriptSpendSig).SortBefore
5 taproot.go:70 (*TaprootTapLeafScript).SortBefore
6 taproot.go:95 (*TaprootBip32Derivation).SortBefore
7 utils.go:469 FindLeafScript (leaf.LeafVersion)
8 finalizer.go:58 isFinalizableWitnessInput (sig.LeafHash)
9 finalizer.go:547 finalizeTaprootInput (TaprootScriptSpendSig[0].LeafHash)

Two things worth flagging for anyone reproducing this:

  • The three SortBefore sites only panic at len >= 2. With a single-element slice sort.Slice never invokes the comparator — I confirmed this with an instrumented counter on the real []*TaprootTapLeafScript type (len=1 → 0 comparator calls, len=2 → 1, len=3 → 3). So a nil in a one-element slice sails through the sort and panics later at the field access (partial_input.go:524), while the same nil in a two-element slice panics inside SortBefore (taproot.go:70). The panic site moves with slice length, which makes this easy to under-count when testing with one element.
  • None of this is reachable from parsing. PInput.deserialize only ever appends non-nil pointers (partial_input.go:284, :315, :338), and ReadTaprootBip32Derivation never returns (nil, nil) — every return nil is paired with an error. So a nil can only be introduced by caller-side construction of a PInput. This is a broken-invariant/robustness issue, not a malicious-PSBT attack vector, and I'd rather say that plainly than let the panic count imply otherwise.

Two further lines (partial_input.go:549, partial_output.go:242) also dereference a nil but are unreachable, since taproot.go:229 panics first. Latent rather than live.

Where the three open PRs stand

There are currently three open PRs fixing this bug with three different semantics. Note they don't target the same tree, so this isn't quite apples-to-apples:

approach target tree guards added tests review
#2498 (this one) reject in serialize, skip in FindLeafScript btcutil/psbt/ (removed from master) 2 1 regression test none
#2511 filter nil entries btcutil/psbt/ (removed from master) 3 none none, CONFLICTING
#2539 reject nil taproot fields psbt/ (current) 7, across 5 functions 132 lines, incl. a 4-case table approved 2026-07-20

I applied #2539's diff to master in a detached worktree and re-ran the probe: 11 of my 12 panic cases go quiet. One does not:

SerializeTaprootBip32Derivation(nil)   panic=true  @ psbt/taproot.go:229

#2539 guards that function's two callers but not the function itself, and it's exported — so psbt.SerializeTaprootBip32Derivation(nil) still panics for any external caller after that PR lands. That looks like the one gap worth closing regardless of which approach wins.

For what it's worth, on the reject-vs-filter question, the evidence points toward reject:

  • A nil entry is unreachable from parsing (see above) — it's a broken invariant rather than a valid data state.
  • BIP-371 lines up with that. PSBT_IN_TAP_LEAF_SCRIPT = 0x15 carries "the control block for this leaf as specified in BIP 341" as its keydata; a nil entry has no control block and is not representable on the wire. BIP-371 also ships explicit invalid-PSBT vectors — "Control block that is too long" and "too short" — and this package already rejects both at deserialization via checkValid()ErrInvalidKeyData. I ran the two real vectors from the BIP through NewFromRawBytes to confirm: both return Invalid key data, errors.Is(err, ErrInvalidKeyData) == true. Dropping a nil silently would be inconsistent with how the package already treats the malformed-leaf case one step away.
  • Silent filtering changes the serialized output. Dropping the entry produces a PSBT the caller did not ask for and returns success, which is harder to debug than an error.

I'll leave the call on how to consolidate these to the maintainers — I mainly wanted the tree-removal point, the full panic surface, and the residual SerializeTaprootBip32Derivation gap on the record, since none of those are obvious from the diffs alone.

Smaller notes

  • Returning bare ErrInvalidPsbtFormat loses the index; wrapping with fmt.Errorf("nil taproot leaf script at index %d: %w", idx, ErrInvalidPsbtFormat) keeps errors.Is working while telling the caller which entry is at fault.
  • On the PR description: since sort.Slice doesn't call the comparator for a one-element slice, the panic in that test occurs at the leafScript.Script field access rather than inside the sort. A two-element case with a nil at index 0 or 1 would exercise the SortBefore path as well, which is a distinct crash site.
  • PInput.IsSane() is currently a return true stub with a TODO, and it is already called both at the top of PInput.serialize and from Packet.SanityCheck. It looks like the natural home for this class of invariant, covering both entry points in one place — though its bool return can't carry per-index diagnostics, so that would mean an error-returning variant rather than reusing it as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug]: pstb.Packet.B64Encode panics when given a nil TaprootScriptLeaf entry

2 participants