Arawoof06 commented on code in PR #1318:
URL: https://github.com/apache/arrow-java/pull/1318#discussion_r4190057551
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java:
##########
@@ -377,6 +381,24 @@ private static int readRawVarint32(int firstByte,
InputStream is) throws IOExcep
return CodedInputStream.readRawVarint32(firstByte, is);
}
+ /**
+ * Reject a field whose declared length is negative or larger than the bytes
left in the message.
+ *
+ * <p>The length prefix is read straight off the wire, and a field can never
be longer than the
+ * bytes still buffered for the message. Without this check an oversized
value drives an unbounded
+ * allocation before any content is read; the {@code new byte[size]} paths
above do so on the JVM
+ * heap, bypassing the {@link BufferAllocator} limit entirely.
+ */
+ private static void checkFieldLength(int size, InputStream stream) throws
IOException {
+ final int remaining = stream.available();
Review Comment:
Good catch, that would have broken every compressed stream. Done in fae1400:
the `available()` bound now only applies when the stream is `KnownLength`, and
other streams go through `readNBytes(size)` plus a length check, with the
`ArrowBuf` allocated only after the bytes have arrived. The `KnownLength` path
keeps the existing `GetReadableBuffer` fast path since the size is already
bounded there.
I also checked this end to end with a client doing `startPut(..., stub ->
stub.withCompression("gzip"))`: the previous revision fails server-side with
"field length 8 exceeds 1 bytes remaining", this one round-trips the stream.
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java:
##########
@@ -312,6 +314,7 @@ private static ArrowMessage frame(BufferAllocator
allocator, final InputStream s
case APP_METADATA_TAG:
{
int size = readRawVarint32(stream);
+ checkFieldLength(size, stream);
appMetadata = allocator.buffer(size);
Review Comment:
Done. The `catch` in `frame()` now releases `appMetadata` and `body` before
rethrowing (`AutoCloseables.close(ioe, appMetadata, body)`, which adds any
close failure as suppressed), and a repeated `app_metadata` releases the
earlier buffer the same way `BODY` already did.
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java:
##########
@@ -377,6 +381,24 @@ private static int readRawVarint32(int firstByte,
InputStream is) throws IOExcep
return CodedInputStream.readRawVarint32(firstByte, is);
}
+ /**
+ * Reject a field whose declared length is negative or larger than the bytes
left in the message.
+ *
+ * <p>The length prefix is read straight off the wire, and a field can never
be longer than the
+ * bytes still buffered for the message. Without this check an oversized
value drives an unbounded
+ * allocation before any content is read; the {@code new byte[size]} paths
above do so on the JVM
+ * heap, bypassing the {@link BufferAllocator} limit entirely.
+ */
+ private static void checkFieldLength(int size, InputStream stream) throws
IOException {
+ final int remaining = stream.available();
+ if (size < 0 || size > remaining) {
+ throw new IOException(
+ String.format(
+ "Malformed FlightData frame: field length %d exceeds %d bytes
remaining in the message",
+ size, remaining));
Review Comment:
Done. Negative lengths get their own message now, and the messages are built
with plain concatenation so there's no locale-sensitive formatting.
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java:
##########
@@ -296,6 +296,7 @@ private static ArrowMessage frame(BufferAllocator
allocator, final InputStream s
case DESCRIPTOR_TAG:
{
int size = readRawVarint32(stream);
+ checkFieldLength(size, stream);
Review Comment:
Done. The varint read and validation live in `readFieldLength`, and the four
call sites go through `readFieldBytes` (descriptor, header) or
`readFieldBuffer` (app_metadata, body), so there's no way to read a length
without the check.
--
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]
