Skip to content

fix(NODE-7611): bounds check findNull scan in on-demand parser - #924

Open
AbinMadathil-Celigo wants to merge 3 commits into
mongodb:mainfrom
AbinMadathil-Celigo:node-7611-findnull-bounds
Open

fix(NODE-7611): bounds check findNull scan in on-demand parser#924
AbinMadathil-Celigo wants to merge 3 commits into
mongodb:mainfrom
AbinMadathil-Celigo:node-7611-findnull-bounds

Conversation

@AbinMadathil-Celigo

Copy link
Copy Markdown

Description

Summary of Changes

A malformed document with a string element declaring a zero length passes the on-demand parser's size/terminator guards and advances the cursor to the declared document end. The next loop iteration then calls findNull from past the end of the buffer: out-of-range Uint8Array reads return undefined (which is never 0x00), so the scan never terminates — a synchronous CPU hang on caller-controlled bytes.

findNull now stops at the end of the input and throws a BSONOffsetError('Null terminator not found') instead of scanning out of range forever. The existing "reached the document terminator" throw is preserved by folding both conditions into nullTerminatorOffset >= bytes.length - 1.

Repro (hangs before this change, throws after):

const BSON = require('bson');
// 10-byte document: string element with empty name declaring stringSize = 0
BSON.onDemand.parseToElements(
  new Uint8Array([0x0a, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00])
);
Notes for Reviewers
  • Only the experimental BSON.onDemand.parseToElements path is affected; the standard BSON.deserialize path already rejects stringSize <= 0.
  • The ticket also notes the stringSize <= 0 check is not mirrored on the on-demand path. I kept this PR focused on the non-termination bug; mirroring that check would need per-type care (e.g. a zero length is valid for binData) and could be a follow-up.

What is the motivation for this change?

Fixes NODE-7611 (part of NODE-7598).

Release Highlight

BSON.onDemand.parseToElements no longer hangs on malformed input

A malformed document containing a string element with a declared length of zero previously caused a non-terminating scan (synchronous CPU hang). The experimental on-demand parser now throws a BSONOffsetError instead.

Double check the following

  • Lint is passing (npm run check:lint)
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

A malformed document with a string element declaring a zero length
passes the size/terminator guards and advances the cursor to the
declared document end. The next loop iteration then called findNull
from past the end of the buffer: out-of-range Uint8Array reads return
undefined (never 0x00), so the scan never terminated — a synchronous
CPU hang on caller-controlled bytes.

findNull now stops at the end of the input and throws BSONOffsetError
instead of scanning out of range forever. The standard deserializer
path (BSON.deserialize) is unaffected; it already rejects
stringSize <= 0.
@AbinMadathil-Celigo
AbinMadathil-Celigo requested a review from a team as a code owner August 26, 2026 15:25
@tadjik1 tadjik1 self-assigned this Aug 27, 2026
@tadjik1 tadjik1 added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Aug 27, 2026
Comment thread test/node/parser/on_demand/parse_to_elements.test.ts

@tadjik1 tadjik1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hi @AbinMadathil-Celigo , thanks for your work! Please check one comment in the code, I believe there is another case we want to make sure works as expected.

…'s end

The length guard only compared the element's length against the whole
document size, so an element near the end of a (sub)document could
declare a value that extends past the document's null terminator and be
silently accepted — e.g. an embedded document whose int32 element's
value bytes actually belong to the outer document's next element.

The value must now fit within the current document: it may extend at
most up to the document's null terminator byte. This also catches the
zero-length string repro earlier with a more descriptive error; the
findNull bounds check remains as a safety net for name and regex
scans.

Also fixes a wire-inaccurate binary fixture in the parser tests that
declared 5 data bytes but provided 1 (asserted length is unchanged).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Primary Review In Review with primary reviewer, not yet ready for team's eyes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants