Skip to content

fix(joiner): add bounds checking on leaf reads#5524

Merged
janos merged 3 commits into
masterfrom
joiner-bounds-check
Jul 8, 2026
Merged

fix(joiner): add bounds checking on leaf reads#5524
janos merged 3 commits into
masterfrom
joiner-bounds-check

Conversation

@janos

@janos janos commented Jul 4, 2026

Copy link
Copy Markdown
Member

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

This PR fixes a panic (crash) and an infinite loop (hang/timeout) when reading malformed or corrupted trees in the joiner.

When the joiner processes a malformed tree where intermediate or root chunks advertise spans larger than the actual data present in their children it hangs in io.Copy as it always returns (0, nil), or panics when the data is requested by the look-ahead reader langos as the io.SeekEnd on joiner's Seek() reports longer data.

Tests are added for fix verification. The real world chunks and initial testing was provided by @acud, thanks.

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

AI Disclosure

  • This PR contains code that has been generated by an LLM.
  • I have reviewed the AI generated code thoroughly.
  • I possess the technical expertise to responsibly review the code generated in this PR.

Comment thread pkg/file/joiner/joiner.go Outdated
}

// Guard against slicing out-of-bounds if the computed end offset is invalid.
if dataOffsetEnd < dataOffsetStart || dataOffsetEnd > int64(len(data)) {

@martinconic martinconic Jul 7, 2026

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.

You can also use here dataLen like above, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, that is right, thanks.

@acud acud 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.

nice one, thanks!

@janos janos merged commit f9b977d into master Jul 8, 2026
15 checks passed
@janos janos deleted the joiner-bounds-check branch July 8, 2026 10:12
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.

5 participants