Skip to content

fix(nfttx): return the unmarshal error from OutputStream.StateAt - #2582

Open
AkramBitar wants to merge 1 commit into
mainfrom
2560
Open

AkramBitar wants to merge 1 commit into
mainfrom
2560

Conversation

@AkramBitar

@AkramBitar AkramBitar commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

OutputStream.StateAt checked err == nil where it meant err != nil. A bad payload was reported as success with state left empty, so callers acted on garbage. The good path worked only because errors.Wrap(nil, ...) returns nil.

Three fixes:

  • flip the condition;
  • reject a type holding the JSON null literal — it unmarshals without writing anything, which is the same false success;
  • return an error for an out-of-range index instead of panicking.

Unit tests and FuzzStateAtNoPanic cover all three, and the fuzz target runs in the nightly matrix. Revert any one fix and a test fails.

QueryByKey in qe.go has the same inverted check. Left for #2561.

Fixes #2560

@AkramBitar AkramBitar added this to the Q4/26 milestone Oct 29, 2026
@AkramBitar AkramBitar added bug Something isn't working hardening labels Oct 29, 2026
@AkramBitar AkramBitar self-assigned this Oct 29, 2026
@AkramBitar
AkramBitar requested review from Effi-S and adecaro October 29, 2026 12:50
AkramBitar added a commit that referenced this pull request Oct 29, 2026
`QueryByKey` checked `err == nil` where it meant `err != nil`, so a malformed
state was hidden behind `ErrNoResults`. The good path worked only because
`errors.Wrap(nil, ...)` returns `nil`.

The decode moves into a `decodeState` helper that also rejects a type holding
the JSON `null` literal, which unmarshals without writing anything.

`OutputStream.StateAt` has the same inverted check; left to #2582.

Fixes #2561

Signed-off-by: AkramBitar <akram@il.ibm.com>
Effi-S

This comment was marked as duplicate.

@Effi-S Effi-S 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.

LGTM

`StateAt` checked `err == nil` where it meant `err != nil`. A bad payload was
reported as success with `state` left empty, so callers acted on garbage. The
good path worked only because `errors.Wrap(nil, ...)` returns `nil`.

Three fixes:

- flip the condition;
- reject a type holding the JSON `null` literal: it unmarshals without writing
  anything, which is the same false success;
- return an error for an out-of-range index instead of panicking.

Unit tests and `FuzzStateAtNoPanic` cover all three, and the fuzz target runs
in the nightly matrix. Revert any one fix and a test fails.

`QueryByKey` in `qe.go` has the same inverted check. Left for #2561.

Fixes #2560

Signed-off-by: AkramBitar <akram@il.ibm.com>

This branch has not been deployed

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

Labels

bug Something isn't working hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inverted error check swallows unmarshal failure in OutputStream.StateAt

2 participants