dongjoon-hyun commented on code in PR #58909:
URL: https://github.com/apache/spark/pull/58909#discussion_r4050849659
##########
common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java:
##########
@@ -442,8 +444,13 @@ public void channelRead(ChannelHandlerContext ctx, Object
ciphertextMessage)
int readableBytes = Math.min(
nettyBufReadableBytes,
ciphertextBuffer.remaining());
- int expectedRemaining = (int) (expectedLength -
ciphertextRead);
- int bytesToRead = Math.min(readableBytes,
expectedRemaining);
+ long expectedRemaining = expectedLength -
ciphertextRead;
+ if (expectedRemaining <= 0) {
+ throw new IllegalStateException(
+ "Invalid ciphertext state:
expectedLength=" + expectedLength
+ + ", ciphertextRead=" +
ciphertextRead);
+ }
Review Comment:
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?
##########
common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java:
##########
@@ -442,8 +444,13 @@ public void channelRead(ChannelHandlerContext ctx, Object
ciphertextMessage)
int readableBytes = Math.min(
nettyBufReadableBytes,
ciphertextBuffer.remaining());
- int expectedRemaining = (int) (expectedLength -
ciphertextRead);
- int bytesToRead = Math.min(readableBytes,
expectedRemaining);
+ long expectedRemaining = expectedLength -
ciphertextRead;
+ if (expectedRemaining <= 0) {
+ throw new IllegalStateException(
+ "Invalid ciphertext state:
expectedLength=" + expectedLength
+ + ", ciphertextRead=" +
ciphertextRead);
+ }
+ int bytesToRead = (int) Math.min((long) readableBytes,
expectedRemaining);
Review Comment:
nit. `(long)` is unnecessary because `Math.min(long, long)` promotes
`readableBytes` automatically.
##########
common/network-common/src/test/java/org/apache/spark/network/crypto/GcmAuthEngineSuite.java:
##########
@@ -570,6 +573,58 @@ public void testSplitLengthPrefix() throws Exception {
}
}
+ @Test
+ public void testCiphertextLengthLargerThanMaxInt() throws Exception {
+ TransportConf gcmConf = getConf(2, false);
+ try (AuthEngine client = new AuthEngine("appId", "secret", gcmConf);
+ AuthEngine server = new AuthEngine("appId", "secret", gcmConf)) {
+ AuthMessage clientChallenge = client.challenge();
+ AuthMessage serverResponse = server.response(clientChallenge);
+ client.deriveSessionCipher(clientChallenge, serverResponse);
+ GcmTransportCipher cipher = (GcmTransportCipher) server.sessionCipher();
+ GcmTransportCipher.DecryptionHandler decryptionHandler =
cipher.getDecryptionHandler();
+ AesGcmHkdfStreaming streaming = cipher.getAesGcmHkdfStreaming();
+
+ long expectedLength = (long) GcmTransportCipher.LENGTH_HEADER_BYTES +
+ streaming.getHeaderLength() + Integer.MAX_VALUE + 1L;
Review Comment:
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`?
##########
common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java:
##########
@@ -369,8 +370,9 @@ private boolean initializeExpectedLength(ByteBuf
ciphertextNettyBuf) {
}
expectedLengthBuffer.flip();
expectedLength = expectedLengthBuffer.getLong();
- if (expectedLength < 0) {
- throw new IllegalStateException("Invalid expected
ciphertext length.");
+ if (expectedLength < LENGTH_HEADER_BYTES + (long)
headerLength) {
Review Comment:
nit. `(long)` looks unnecessary here because both operands are small `int`
values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
