Skip to content

fix(bits): guard BitArray against malformed Bits/Elems from peer proto - #46

Open
FletcherMan wants to merge 1 commit into
mainfrom
hotfix/bitarray-index-oob
Open

FletcherMan wants to merge 1 commit into
mainfrom
hotfix/bitarray-index-oob

Conversation

@FletcherMan

Copy link
Copy Markdown
Collaborator

A peer can send a consensus message (NewValidBlock/ProposalPOL/VoteSetBits) whose BitArray carries Bits>0 but no Elems. FromProto writes Bits unconditionally but only writes Elems when non-empty, so the decoded array is internally inconsistent. getTrueIndices then indexes Elems[-1] and panics with "index out of range [-1]", crashing the node from the catchup gossip path (reactor.go gossipDataForCatchup -> PickRandom -> getTrueIndices).

Minimal, surgical hotfix on the three bit_array.go sites that index Elems based on Bits without checking len(Elems):

  • getTrueIndices: treat a Bits/Elems-inconsistent array as having no set bits
  • setIndex: bounds-check i/64 against len(Elems)
  • IsFull: avoid Elems[:len-1] panic on empty Elems

Does not change FromProto or message ValidateBasic; a fuller validation-based fix (ported from CometBFT v0.40.0) is tracked separately.


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

A peer can send a consensus message (NewValidBlock/ProposalPOL/VoteSetBits)
whose BitArray carries Bits>0 but no Elems. FromProto writes Bits
unconditionally but only writes Elems when non-empty, so the decoded array
is internally inconsistent. getTrueIndices then indexes Elems[-1] and panics
with "index out of range [-1]", crashing the node from the catchup gossip
path (reactor.go gossipDataForCatchup -> PickRandom -> getTrueIndices).

Minimal, surgical hotfix on the three bit_array.go sites that index Elems
based on Bits without checking len(Elems):
- getTrueIndices: treat a Bits/Elems-inconsistent array as having no set bits
- setIndex: bounds-check i/64 against len(Elems)
- IsFull: avoid Elems[:len-1] panic on empty Elems

Does not change FromProto or message ValidateBasic; a fuller validation-based
fix (ported from CometBFT v0.40.0) is tracked separately.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@FletcherMan
FletcherMan requested a review from a team as a code owner October 6, 2026 18:05
@FletcherMan
FletcherMan requested review from twcctop and removed request for a team October 6, 2026 18:05

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.

Tip: disable this comment in your organization's Code Review settings.

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.

1 participant