Repository navigation
Conversation
A negative ArrowFieldNode length flows unchecked into buffer-size math (getValidityBufferSizeFromCount uses an unsigned shift) and into ArrowBuf.setOne/setZero, risking an oversized allocation and, with bounds checking disabled, memory corruption via Unsafe.setMemory. The IPC, JSON, and C Data deserialization paths all construct ArrowFieldNode from external metadata, so validate length at that single chokepoint. Closes apache#1421
ArrowFieldNode narrowed its arguments with checkedCastToInt, a plain cast when bounds checking is disabled, so a C Data length such as 2^32 - 100 passed the length check and was stored as -100. Narrow with Math.toIntExact unconditionally, and reject a null count below the -1 "unknown" sentinel. JsonFileReader sized a column's buffers from its count before building the ArrowFieldNode, so a negative count still allocated about 512 MB, and batch counts went straight into setRowCount. Reject a negative count as soon as it is read.
MessageSerializer.deserializeRecordBatch(RecordBatch, ArrowBuf) released the body only after building the ArrowRecordBatch, so an exception before that point leaked it. The negative-length check in ArrowFieldNode made a negative node length one such case; node and batch lengths above INT_MAX already were. ArrowStreamReader, ArrowFileReader, dictionary batches, and Flight's ArrowMessage hand the body to this method and do not release it themselves. Release the body in a finally block. ArrowRecordBatch retains the slices it keeps, so the release is also correct when deserialization succeeds.
deserializeRecordBatch left a negative length or a null count below -1 to the ArrowFieldNode constructor, which throws IllegalArgumentException. Check both in the loop so the IPC readers report every malformed node with the IOException they declare, and test the boundary values through deserializeRecordBatch.
Build the ArrowFieldNode inside the try block in ArrayImporter so an invalid length or null count reports the field it belongs to, and close the vector in Data.importVector when the import fails so children that were already loaded are released.
…eader leaks - Reject a negative record batch length, which the INT_MAX check let through. - Reject buffers with a negative offset or length, or that extend past the body, with an IOException. A negative length passed ArrowBuf.slice and made the ArrowRecordBatch constructor throw after retaining the body, leaking it. - Use an empty body for messages without one in deserializeMessageBatch, as ArrowStreamReader does, instead of failing with a NullPointerException. - Close the record batch in JsonFileReader.read() and release the local buffers in readFromJsonIntoVector when reading a column fails. - Detect overflow of the offset buffer count in JsonFileReader.
…ionary - Build the ArrowFieldNode before importing children and the dictionary, so an array with an invalid length or null count no longer clears and reloads the provider's dictionary before it is rejected. - Use Math.toIntExact for n_children and n_buffers. - Compute the offsets buffer size as a long, so a length of Integer.MAX_VALUE no longer overflows, and reject a negative buffer capacity. - Keep the original exception if closing the vector fails after an import error, and close it on any Throwable.
Check the dictionary's length and null count together with the array's before importing anything, and import the dictionary after the array's own buffers. An array with an invalid length, null count or missing buffer, or a dictionary with an invalid length or null count, no longer clears the dictionary in the provider. Reword the comment to say only what is checked up front, and release the test vectors when an assertion fails.
A buffer allocated while reading a JSON buffer leaked if parsing failed before the buffer was returned, for example on an invalid value or an extra value before the closing bracket. Release it on failure, including for view and variadic buffers. Also reject a null count with IllegalArgumentException instead of a NullPointerException.
…dies on errors C Data: check n_children against the expected number of children, and n_buffers against the type's buffer layout, before reading the child and buffer pointers. A huge count no longer sizes the read, which could throw an OutOfMemoryError or read past the end of the pointer array. IPC: reject variadic buffer counts that are negative or larger than the number of buffers in the batch, before VectorLoader uses them to size a list. Release the message body when a message is rejected for its size, version or header type, and release the block buffer when reading a file block fails before the batch is deserialized. JSON: set the batch row count after the columns are read, as read(VectorSchemaRoot) does, so the vectors are not first allocated for a count the columns may not match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's Changed
Follow-up to #1422 (#1421). This branch is stacked on #1422, so until that is merged the diff also shows its commits. Only the last commit, "Check IPC and C Data counts before using them, and release message bodies on errors", is new here.
C Data (
ArrayImporter)n_childrenagainst the number of child vectors before reading the child pointers, instead of after.n_buffersbefore reading the buffer pointers. It must not be negative, and for types with a fixed buffer layout it must not exceed that layout's buffer count. Before this, a hugen_bufferssized along[]and threwOutOfMemoryError, which escapes the import'scatch (Exception).IPC (
MessageSerializer)VectorLoadercasts these counts tointand uses them to size a list.deserializeMessageBatch: release the body when the message is rejected for its size, metadata version or header type.deserializeRecordBatch/deserializeDictionaryBatchwith anArrowBlock: release the block buffer when the read is short or the metadata is malformed, before the batch is deserialized.JSON (
JsonFileReader)read()now sets the row count after the columns are read, asread(VectorSchemaRoot)already does, so vectors are not first allocated for the batch count.Tests in
RoundtripTest(buffer and child counts) andMessageSerializerTest(variadic counts, unsupported messages, truncated blocks). Without the fix, the newMessageSerializerTestcases fail, andn_buffers = Integer.MAX_VALUEcrashes the forked test JVM withOutOfMemoryError: Requested array size exceeds VM limit.Closes #1462.