Bug 108

Chain.hasBlocks with a negative max_height dereferences null

Status: Open
Symptom: Crash; worst effect seen: nothing observed
Where seen: Never observed
Trigger: Unusual input from a peer or caller
Introduced: 1be8ff280c78, 2020-01-21
First reported: 2026-10-30 (first report)
Fixed: Not yet
Tracked in: Not tracked on GitHub yet
In released binaries: No
Defect ever on master: Yes

Summary

Calling hasBlocks on the Chain IPC interface with a negative max_height makes bitcoin-node dereference a null block pointer and crash. The method was written for the wallet running in the same process, which never passes a negative height. Only the open PR bitcoin#29409, which exposes Chain over IPC, makes it reachable, so master and releases are not affected. It was found by reading the code and has not been run. No fix yet; the related fixes in bitcoin#36520 do not cover it.

What happened

@enirox001, testing bitcoin#36520 ("interfaces: Avoid node crashes from invalid Chain IPC calls"), noted on 2026-10-30 that hasBlocks can dereference null when GetAncestor gets a negative height, the same kind of problem as the getBlockHash assert that PR fixes.

Root cause

ChainImpl::hasBlocks() in src/node/interfaces.cpp checks whether every block in a height range below block_hash has its data on disk. It looks the hash up, and if max_height is set and not above that block's height, moves to the ancestor at max_height with CBlockIndex::GetAncestor(). GetAncestor() returns null for a negative height, and the loop that follows reads block->nStatus without checking. Every known block has a height of 0 or more, so any negative max_height takes this path when the hash is known.

The function's comment says a max_height "that's too high" will not crash, and the loop guards a min_height that is too low, but negative values were never considered. The in-process wallet's only caller, rescanblockchain, rejects negative heights before calling. Over IPC, bitcoin#29409's schema sends maxHeight as a plain Int32 with a separate hasMaxHeight flag, so any client can send one.

Origin: the lines are unchanged since ryanofsky added hasBlocks in 1be8ff280c78 ("wallet: Avoid use of Chain::Lock in rescanblockchain", merged in bitcoin#17954, 2020-04-14, first released in v0.21.0), when its only caller was the wallet in the same process. It becomes a crash only with bitcoin#29409, which is not merged.

Fix

No fix yet. The likely change is the one bitcoin#36520 makes for getBlockHash: reject a negative max_height (or treat it as an empty range) instead of dereferencing the lookup result.

Next steps

Add a hasBlocks check to bitcoin#36520, or a follow-up, with a case in its invalidArguments test.
As enirox001 suggested, look for one shared way to validate arguments and failed lookups across the Chain methods that bitcoin#29409 exposes, rather than finding them one at a time.

Timeline

2020-04-14: hasBlocks added with these lines, in bitcoin#17954 (1be8ff280c78); the only caller is the in-process wallet.
2024-02-08: bitcoin#29409 opened to expose Chain over IPC; still open.
2026-10-30: reported by @enirox001 in a review of bitcoin#36520.
Releases: not affected. On master, Chain is not exposed over IPC.

Lessons

What would have prevented this bug, found it sooner, or made it easier to debug. The patterns page groups these lessons across all bugs.

Pattern An in-process interface trusting what now comes from another process: An assertion on what is now input from a peer

Prevented by a different API shape
Earliest detector a test with a non-C++ client (did not exist then)
Diagnosis easy
Fix design easy
Rust partly

Would Rust have prevented it? Partly. In Rust the ancestor lookup would return an Option and the caller would have to handle None or unwrap it; an unwrap panics like the null dereference crashes, but the missing case is visible at the call site. More on Rust

Could it have been prevented?

Yes, by the same rule as the getBlockHash assert: treat every argument of a method exposed over IPC as untrusted. The function's own comment shows the author thought about bad ranges ("max_height that's too high will not crash"), but only about the values the wallet could produce.

Could it have been found sooner?

Yes. A sweep of the Chain methods for unchecked lookups, done once when bitcoin#29409 was opened, or a fuzz target over the interface, would find it. It was found by a reviewer of bitcoin#36520, the PR that fixes the first such reports one at a time.

What made it hard to debug?

Nothing yet: it was found by reading the code before anyone hit it.

Lesson

Fixing reported cases of "internal precondition, now remote input" one by one leaves the siblings; when an interface goes remote, check all of its methods at once.

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.

Field	Value
Symptom	crash
Worst effect observed	none
Seen in	never
Scope	production
Trigger	Unusual input (`unusual-input`)
Phase	steady-state
Side	server
Component	bitcoin-ipc
Root cause kind	assumption-violated
Fix kind	none
Detection	review
Needs a peer that is not libmultiprocess C++	no
Needed a sanitizer to see	no
Defect ever on master	yes
Released	no
Confidence	high