Skip to content

fix: bounds check in Delete to prevent index out of range panic#280

Open
Yanhu007 wants to merge 1 commit intobuger:masterfrom
Yanhu007:fix/delete-bounds-check
Open

fix: bounds check in Delete to prevent index out of range panic#280
Yanhu007 wants to merge 1 commit intobuger:masterfrom
Yanhu007:fix/delete-bounds-check

Conversation

@Yanhu007
Copy link
Copy Markdown

Fixes #274

Problem

Delete accesses data[endOffset+tokEnd] without checking if the index is within bounds. tokenEnd() returns len(data) when no terminator is found, so endOffset+tokEnd can exceed the slice length, causing a panic:

panic: runtime error: index out of range [9] with length 9

Found by fuzzing.

Fix

Add a bounds check before accessing data[endOffset+tokEnd]:

if endOffset+tokEnd >= len(data) {
    return data
}

All existing tests pass.

Delete accesses data[endOffset+tokEnd] without checking if the
index is within bounds. When tokenEnd returns len(data) (no
terminator found), this causes an index out of range panic.

Found by fuzzing (issue buger#274).

Fixes buger#274
@colek42
Copy link
Copy Markdown

colek42 commented Apr 14, 2026

Hi @buger and @Yanhu007 — we hit the same panic in the TestifySec Platform supply-chain stack and ended up vendoring a fork of v1.1.1 with a defer/recover wrapper around Delete() as a stopgap. The bounds check in this PR is the more correct fix — it addresses the root cause rather than masking it.

Two things that may help land this:

Adversarial test fixtures

We built a regression suite for GHSA-6g7g-w4f8-9c9x while building our fork. Drops cleanly into parser_test.go. Happy to open a follow-up PR contributing it once this lands. The cases that crash upstream v1.1.1 today:

  • Truncated JSON immediately after a key (breaks endOffset calculation downstream)
  • Malformed nested objects with mismatched braces
  • Unicode escape sequences with truncated payloads
  • Several oss-fuzz outputs reduced to minimal repros

The PoC from #274 (eyJ0ZXN0Ijox base64) is one such case.

Bounds check coverage

The check at the call site fixes the specific panic in the linked stack trace, but tokenEnd() returning len(data) can affect other accessors in Delete too. Worth running the fuzz target (fuzz.go:31) for a few minutes after this lands to confirm no other related panics surface — happy to share the corpus we collected.

Tag a release after merge and we can drop our fork — there are 31 dependabot alerts in our org that this resolves.

Thanks for the fix.

@aleksandr4842
Copy link
Copy Markdown

@Yanhu007 hi!
Please, add this test to deleteTests:

	{
		desc: "GO-2026-4514: malformed JSON without closing brace",
		json: `{"a":1`,
		path: []string{"a"},
		data: `{`,
	},

buger added a commit that referenced this pull request Apr 19, 2026
Bug fixes:
- Fix Delete panic on truncated JSON (PR #280 class): tokenEnd returns
  len(data) as sentinel when no delimiter found, Delete used it as unchecked
  array index causing out-of-bounds panic on inputs like {"test":1
- Fix ArrayEach callback error swallowing: callback's err parameter was
  always nil despite the signature declaring it. Callers checking err in
  their callback had dead error handling code. Now the callback receives
  per-element parse errors before iteration stops.

Dead code removal (all verified safe by MC/DC analysis + 60 targeted tests):
- Remove tautological for-true loops (ArrayEach, Unescape, ObjectEach)
- Remove dead tokenEnd end==-1 guard in getType (tokenEnd never returns -1)
- Remove dead r<=basicMultilingualPlaneOffset in decodeUnicodeEscape (tautology)
- Remove dead data[i]=='{' block-skip in EachKey (structurally unreachable)
- Remove contradictory keys[level][0]!='[' in searchKeys (outer guard already checks ==)
- Remove dead e!=nil in ArrayEach o==0 branch (Get offset=0 always means error)
- Remove tautological ln>0 guard in findKeyStart (proven by prior nextToken check)

Formal verification (ReqProof):
- 92 requirements (7 stakeholder + 85 system) covering all API families
- 18 obligation classes including truncated_at_value_boundary, sentinel_value_boundary,
  error_propagation, and truncated_escape_sequence
- 100% requirement-level MC/DC (204/204 witness rows)
- 100% code-level MC/DC (203/203 decisions, 244/244 conditions)
- Kind2 realizability, consistency, and vacuity verification
- Z3 data property proofs and behavioral implication proofs
- 340 FLIP fixtures + 21 Z3 boundary fixtures
- CI integration via probelabs/proof-action@v1

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@buger
Copy link
Copy Markdown
Owner

buger commented Apr 19, 2026

I have applied formal verification techniques to jsonparser using reqproof.com (e.g. it now close to being bug free is as much as possible, with some math proof) - #281

It supersede this PR, appreciate your review.

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.

Oob (index out of range)

4 participants