ranflarion opened a new pull request, #10522: URL: https://github.com/apache/arrow-rs/pull/10522
# Which issue does this PR close? - Closes #10521. # Rationale for this change `MessageReader::maybe_next` reserves both the metadata length and the message body length before reading any of the bytes they describe. Both come out of the stream itself, so a corrupted or truncated stream is handed straight to the allocator: `MutableBuffer::from_len_zeroed(message.bodyLength() as usize)` on an implausible length either aborts the process (`memory allocation of N bytes failed`, which is not catchable and takes the host process with it) or panics on `LayoutError`. A negative `bodyLength` is accepted too, since `as usize` wraps it to a large positive length. I hit this fuzzing real IPC blocks rather than crafted ones: single-bit flips over the framing region of genuine streams produced `memory allocation of 1125899907497992 bytes failed` and `SIGABRT`. Where those blocks cross disk or a network, one flipped bit ends the process instead of failing a read the caller could retry. # What changes are included in this PR? `bodyLength` now goes through `usize::try_from`, so a negative length is a parse error rather than a huge positive one. Neither length reserves more than `MAX_PREALLOC_BYTES` (64 MiB) before the bytes behind it have arrived. Bodies up to that size are allocated in one go exactly as before; larger ones grow as the data arrives, which costs the reallocations that `MutableBuffer::reserve` doubling implies. That constant is the one judgement call here, trading the size of the bounded allocation a malformed stream can still ask for against how large a body keeps the single-allocation path, so it is worth a second opinion. The metadata read switches from `resize(meta_len, 0)` plus `read_exact` to `take(meta_len).read_to_end(&mut self.buf)`. That reuses the retained capacity across messages and drops the zeroing entirely, so it should be slightly cheaper than what it replaces rather than a cost, and `Take` returns `Ok(0)` at its limit so there is no extra read. Only the streaming path is touched. `read_block` on the file side has the same shape at `arrow-ipc/src/reader.rs:875` and two `unwrap()`s on block metadata besides; I left it alone to keep this reviewable, and noted it in the issue. This overlaps #9777, which is after the same zeroing for performance reasons. The two want the same thing here, and I am happy to rebase onto whatever lands first. # Are these changes tested? Yes, two tests in `arrow-ipc/src/reader.rs`. `test_stream_reader_rejects_implausible_body_length` covers `i64::MAX`, `1 << 50` and `-1`; `test_stream_reader_rejects_unbacked_metadata_length` covers a metadata length of `i32::MAX` with nineteen bytes behind it. Both fail without the change: the first panics inside `MutableBuffer::from_len_zeroed`, and the second spends 7.7s zeroing 2 GiB before reporting the wrong error. The existing `arrow-ipc` suite passes (139 tests), along with `cargo fmt --all --check` and `cargo clippy -p arrow-ipc --all-targets --all-features -- -D warnings`. # Are there any user-facing changes? No API changes. A stream that previously aborted or panicked now returns an `ArrowError`. No breaking changes. -- 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]
