Skip to content

[MINOR][CORE][3.5] Validate array counts against received bytes in shuffle fetch decoders - #58617

Open
holdenk wants to merge 3 commits into
apache:branch-3.5from
holdenk:17-validate-fetch-counts-3.5
Open

[MINOR][CORE][3.5] Validate array counts against received bytes in shuffle fetch decoders#58617
holdenk wants to merge 3 commits into
apache:branch-3.5from
holdenk:17-validate-fetch-counts-3.5

Conversation

@holdenk

@holdenk holdenk commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

FetchShuffleBlocks and FetchShuffleBlockChunks decode an outer element count and eagerly allocate a 2D array of that size. Check the count against the bytes actually received before allocating: each inner group carries its own 4-byte length prefix, so a valid count never exceeds readableBytes() / 4. Uses an explicit comparison since Objects.checkFromIndexSize is unavailable on Java 8.

Backport of 9687793

sfc-gh-hkarau and others added 3 commits September 2, 2026 21:15
Look the stream up before the client-id check so an unknown stream id is
rejected either way, and enforce that a stream is only read from the connection
that registered it. The mocked reverse clients in the request handler suites are
bound to their channel to match TransportContext.

branch-3.5 uses Guava Preconditions and JUnit 4.

Co-Authored-By: Holden Karau <holden@pigscanfly.ca>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>


Co-authored-by: Holden Karau <holden@pigscanfly.ca>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-Authored-By: Holden Karau <holden@pigscanfly.ca>
… fetch decoders

FetchShuffleBlocks and FetchShuffleBlockChunks decode an outer element
count and eagerly allocate a 2D array of that size. Check the count
against the bytes actually received before allocating: each inner group
carries its own 4-byte length prefix, so a valid count never exceeds
readableBytes() / 4. Uses an explicit comparison since
Objects.checkFromIndexSize is unavailable on Java 8.

Co-authored-by: Cursor <cursoragent@cursor.com>
@pan3793 pan3793 changed the title [CORE][MINOR][BACKPORT] Validate array counts against received bytes in shuffle fetch decoders [CORE][MINOR][3.5] Validate array counts against received bytes in shuffle fetch decoders Sep 8, 2026
@pan3793 pan3793 changed the title [CORE][MINOR][3.5] Validate array counts against received bytes in shuffle fetch decoders [MINOR][CORE][3.5] Validate array counts against received bytes in shuffle fetch decoders Sep 8, 2026

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

Reviewed commit 346e465. No actionable findings.

The outer-count checks reject negative or impossible counts before allocation, preserve valid encodings (including empty inner groups), and avoid APIs unavailable on Java 8. The full displayed patch was reviewed; the stream and installer changes are already present unchanged on branch-3.5, leaving the four-file decoder/test delta.

Validation:

  • Both decoder suites passed locally (4/4 tests).
  • Head and base each passed 20 valid fixtures through direct decoding and message dispatch. Another 144 bounded malformed decodes per revision confirmed the new guard rejects before consuming inner groups.
  • Local compilation targeted Java 8 using exact protocol sources and declared dependency versions, with existing Spark 3.5.3 jars for unrelated imports; execution used Java 17. This was a partial build, not a clean Spark build or a Java 8 runtime test.
  • Exact-head CI reports confirm both decoder suites, relevant stream suites, and the full installer suite passed. Five local installer helper tests and whitespace checks also passed.

CI caveat: workflow 34228034188 has a test-report upload failure (403) and a Kubernetes integration failure whose cause remains unconfirmed. These failures were not established as defects in this patch.

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

No actionable findings at 346e465. See the preceding review for validation details and the remaining CI caveats.

@uros-b uros-b 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.

Looks like a clean backport, thank you @holdenk, but could we please follow the PR description format?

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.

4 participants