Copilot commented on code in PR #3919:
URL: https://github.com/apache/avro/pull/3919#discussion_r3727461130
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -320,6 +320,18 @@ boolean hasNextBlock() {
if (blockSize > Integer.MAX_VALUE || blockSize < 0) {
throw new IOException("Block size invalid or too large for this
implementation: " + blockSize);
}
+ // When the number of bytes remaining in the input is known (e.g. a
+ // byte-array- or known-length-stream-backed decoder), reject a declared
+ // block size that could not possibly be satisfied by the data available.
+ // This avoids eagerly allocating a large block buffer (see the DataBlock
+ // constructor) for a malformed, corrupted, or truncated file before any
+ // block byte has been read. A value of -1 means the remaining count is
+ // unknown, in which case the check is skipped.
+ int remaining = vin.remainingBytes();
+ if (remaining >= 0 && blockSize > remaining) {
+ throw new IOException("Block size " + blockSize + " exceeds the number
of bytes remaining in the input ("
+ + remaining + "). The file is likely corrupted or truncated.");
+ }
Review Comment:
The new remaining-bytes guard only compares `blockSize` to
`vin.remainingBytes()`, but reading a block also requires the sync marker
bytes. If `remaining == blockSize` (e.g., truncated right before the sync
marker), this still marks a block as available and can allocate the full block
buffer before failing later. Consider including `DataFileConstants.SYNC_SIZE`
in the required-bytes check so the guard matches the actual minimum bytes
needed to read a block.
##########
lang/java/avro/src/test/java/org/apache/avro/TestDataFileReader.java:
##########
@@ -319,4 +319,63 @@ private static byte[]
buildVersion12ContainerWithoutSchema() throws IOException
return output.toByteArray();
}
+
+ /**
+ * A block header may declare a block size much larger than the data actually
+ * present in a corrupted or truncated file. When the remaining byte count is
+ * known, the reader must reject such a block up front rather than
attempting to
+ * allocate a buffer of the declared size.
+ */
+ @Test
+ void oversizedBlockSizeIsRejectedBeforeAllocation() throws IOException {
+ Schema schema = new Schema.Parser().parse("{\"type\":\"int\"}");
+
+ // A spec-correct header with zero records, so no real data block is
written.
+ ByteArrayOutputStream fileBytes = new ByteArrayOutputStream();
+ try (DataFileWriter<Object> w = new DataFileWriter<>(new
GenericDatumWriter<>(schema))) {
+ w.create(schema, fileBytes);
+ }
+
+ // Append a single block header that declares a huge block size but no
data.
+ BinaryEncoder encoder = EncoderFactory.get().binaryEncoder(fileBytes,
null);
+ encoder.writeLong(1L); // block entry count
+ encoder.writeLong(2_000_000_000L); // block size in bytes, far larger than
what follows
+ encoder.flush();
+
+ byte[] malformed = fileBytes.toByteArray();
+
+ // hasNextBlock() surfaces block-header IOExceptions wrapped in an
+ // AvroRuntimeException, the same way the existing "block size too large"
+ // check does.
+ AvroRuntimeException exception = assertThrows(AvroRuntimeException.class,
() -> {
+ DataFileStream<Object> reader = new DataFileStream<>(new
ByteArrayInputStream(malformed),
+ new GenericDatumReader<>());
+ while (reader.hasNext()) {
+ reader.next();
+ }
+ });
Review Comment:
The `DataFileStream` created inside `assertThrows` isn’t closed when the
exception is thrown. Even though this test uses a `ByteArrayInputStream`, using
try-with-resources avoids leaking resources and matches the pattern used
elsewhere in this test class.
--
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]
