Skip to content

[SPARK-59636][CORE] Fix integer overflow in GcmTransportCipher - #58909

Open
aajisaka wants to merge 1 commit into
apache:masterfrom
aajisaka:SPARK-59636
Open

aajisaka wants to merge 1 commit into
apache:masterfrom
aajisaka:SPARK-59636

Conversation

@aajisaka

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Fix integer overflow in GcmTransportCipher

Why are the changes needed?

To fix shuffle error in large shuffle block (>2GB) when AES-GCM is enabled for RPC encryption:

26/09/09 16:18:07 WARN org.apache.spark.network.server.TransportChannelHandler: Exception in connection from <host:port>
java.lang.IllegalArgumentException
    at java.nio.Buffer.limit(Buffer.java:275)
    at org.apache.spark.network.crypto.GcmTransportCipher$DecryptionHandler.channelRead(GcmTransportCipher.java:458)
    ...

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added regression tests. Also, applied the patch and ran Spark jobs with 3GB shuffle blocks successfully in Spark on YARN cluster.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: OpenCode (GPT-5.6 Sol)

expectedLength = expectedLengthBuffer.getLong();
if (expectedLength < 0) {
throw new IllegalStateException("Invalid expected ciphertext length.");
if (expectedLength < LENGTH_HEADER_BYTES + (long) headerLength) {

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.

nit. (long) looks unnecessary here because both operands are small int values.

Comment on lines +448 to +452
if (expectedRemaining <= 0) {
throw new IllegalStateException(
"Invalid ciphertext state: expectedLength=" + expectedLength
+ ", ciphertextRead=" + ciphertextRead);
}

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.

Is this reachable? With the new lower-bound check in initializeExpectedLength, ciphertextRead <= expectedLength always holds after the header is read, and completed becomes true when they are equal, so this loop is not entered with expectedRemaining <= 0. Shall we remove this?

"Invalid ciphertext state: expectedLength=" + expectedLength
+ ", ciphertextRead=" + ciphertextRead);
}
int bytesToRead = (int) Math.min((long) readableBytes, expectedRemaining);

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.

nit. (long) is unnecessary because Math.min(long, long) promotes readableBytes automatically.

Comment on lines +588 to +589
long expectedLength = (long) GcmTransportCipher.LENGTH_HEADER_BYTES +
streaming.getHeaderLength() + Integer.MAX_VALUE + 1L;

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.

nit. Could you add a comment explaining that the remaining ciphertext length is Integer.MAX_VALUE + 1, which becomes Integer.MIN_VALUE when cast to int?

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.

2 participants