From c36a092df829aea1489a5e70a2f52acc129cf6ae Mon Sep 17 00:00:00 2001 From: masumi ryugo <280057467+masumi-ryugo@users.noreply.github.com> Date: Fri, 1 May 2026 23:41:37 +0900 Subject: [PATCH 1/2] fix(arrow-ipc): bound MessageReader allocations by actual stream bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MessageReader::maybe_next` decoded the on-wire `meta_len` (after the round-1 check that rejects negative values) and the FlatBuffer message's `bodyLength` and used both directly for up-front allocations: self.buf.resize(meta_len, 0); // <— attacker-controlled let mut buf = MutableBuffer::from_len_zeroed(message.bodyLength() as usize); A 4-byte input — `00 1b 00 48` — claims a ~1.2 GiB metadata payload via `meta_len = i32::from_le_bytes(...) = 0x4800_1b00`, driving a 1.2 GiB `Vec::resize` before any short-read could fail. ~3×10^8 amplification factor from input bytes to allocation; OOM-kills the process on a 2 GB-rss-limited fuzzer. Read both metadata and body via incremental reads tied to the bytes actually delivered by the underlying `Read`: * metadata uses `take(meta_len).read_to_end(&mut self.buf)` followed by an explicit length check; * body is filled by a new `read_body_into_buffer` helper that `extend_from_slice`s 64 KiB chunks into a `MutableBuffer`, preserving the cache-line-aligned allocation that downstream Arrow consumers rely on while keeping the high-water-mark proportional to the bytes actually received. Add `bodyLength` validation (`usize::try_from`) so a negative i64 is surfaced as a `ParseError` instead of wrapping into a huge `usize`. Add a regression test (`test_stream_reader_huge_meta_len_does_not_oom`) that feeds the 4-byte fuzzer repro through `StreamReader::try_new` and asserts a clean `Err`. Found via cargo-fuzz libFuzzer harness wrapping `StreamReader::try_new`. --- arrow-ipc/src/reader.rs | 76 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 72 insertions(+), 4 deletions(-) diff --git a/arrow-ipc/src/reader.rs b/arrow-ipc/src/reader.rs index 1d5e06c6871c..40e93f4d852e 100644 --- a/arrow-ipc/src/reader.rs +++ b/arrow-ipc/src/reader.rs @@ -1794,6 +1794,38 @@ pub(crate) enum IpcMessage { }, } +/// Read exactly `body_len` bytes from `reader` into a freshly allocated +/// [`MutableBuffer`], growing the buffer in 64 KiB chunks. +/// +/// This deliberately avoids the previous `MutableBuffer::from_len_zeroed(body_len)` +/// pattern: when `body_len` is derived from an untrusted IPC message header +/// that pattern allowed a 4-byte input (claiming a 1.2 GiB body) to drive a +/// 1.2 GiB up-front allocation before any read failure could surface. The +/// chunked path here means the high-water-mark allocation is tied to the +/// number of bytes actually delivered by `reader`, while still preserving +/// the cache-line alignment that downstream Arrow consumers rely on. +fn read_body_into_buffer( + reader: &mut R, + body_len: usize, +) -> Result { + const CHUNK: usize = 64 * 1024; + let mut buf = MutableBuffer::with_capacity(0); + let mut remaining = body_len; + let mut scratch = [0u8; CHUNK]; + while remaining > 0 { + let want = remaining.min(CHUNK); + let slice = &mut scratch[..want]; + reader.read_exact(slice).map_err(|e| { + ArrowError::ParseError(format!( + "Unexpected EOF reading {body_len} bytes of message body: {e}" + )) + })?; + buf.extend_from_slice(slice); + remaining -= want; + } + Ok(buf) +} + /// A low-level construct that reads [`Message::Message`]s from a reader while /// re-using a buffer for metadata. This is composed into [`StreamReader`]. struct MessageReader { @@ -1825,15 +1857,35 @@ impl MessageReader { return Ok(None); }; - self.buf.resize(meta_len, 0); - self.reader.read_exact(&mut self.buf)?; + // Both `meta_len` and the message's `bodyLength` are derived from + // untrusted input. Eagerly resizing `self.buf` to `meta_len` (or a + // fresh `MutableBuffer` to `bodyLength` further down) means a few + // bytes of attacker-crafted input — e.g. a header that lies and + // claims a 1.2 GiB metadata payload — would force a multi-GB + // up-front allocation before any read failure could bound it. + // + // Read into a buffer that grows as actual bytes arrive instead, so + // the upfront allocation is independent of the (untrusted) declared + // length, then verify we got exactly the requested number of bytes. + self.buf.clear(); + let read = (&mut self.reader) + .take(meta_len as u64) + .read_to_end(&mut self.buf)?; + if read != meta_len { + return Err(ArrowError::ParseError(format!( + "Unexpected EOF reading {meta_len} bytes of message metadata, got {read}" + ))); + } let message = crate::root_as_message(self.buf.as_slice()).map_err(|err| { ArrowError::ParseError(format!("Unable to get root as message: {err:?}")) })?; - let mut buf = MutableBuffer::from_len_zeroed(message.bodyLength() as usize); - self.reader.read_exact(&mut buf)?; + let body_len_i64 = message.bodyLength(); + let body_len = usize::try_from(body_len_i64).map_err(|_| { + ArrowError::ParseError(format!("Invalid message bodyLength: {body_len_i64}")) + })?; + let buf = read_body_into_buffer(&mut self.reader, body_len)?; Ok(Some((message, buf))) } @@ -2083,6 +2135,22 @@ mod tests { ); } + /// Regression test: an IPC stream whose first 4 bytes claim a 1.2 GiB + /// metadata payload should not drive a multi-GB allocation; it should + /// surface as a `ParseError` once `read_to_end` notices the underlying + /// stream cannot deliver that many bytes. + /// + /// Repro from cargo-fuzz `fuzz_ipc_reader` libFuzzer harness. + #[test] + fn test_stream_reader_huge_meta_len_does_not_oom() { + let bytes: [u8; 4] = [0x00, 0x1b, 0x00, 0x48]; + // i32::from_le_bytes => 0x4800_1b00 = 1_207_975_168 (~1.15 GiB). + // Pre-fix this caused MutableBuffer::from_len_zeroed(~1.2 GiB) / + // self.buf.resize(~1.2 GiB, 0) before any short-read check ran. + let result = StreamReader::try_new(Cursor::new(bytes), None); + assert!(result.is_err(), "expected error, got {result:?}"); + } + #[test] fn test_missing_buffer_metadata_error() { use crate::r#gen::Message::*; From 73847c569e264119bd8034f8887f0ab0f0420e6b Mon Sep 17 00:00:00 2001 From: masumi ryugo <280057467+masumi-ryugo@users.noreply.github.com> Date: Wed, 3 Jun 2026 11:47:17 +0900 Subject: [PATCH 2/2] fix(arrow-ipc): bound MessageReader allocations via meta/body length caps Replace the chunked-read approach (which regressed StreamReader/no_validation/read_10 by ~86% per #9869 bench) with up-front bound checks against MAX_META_LEN (16 MiB) and MAX_BODY_LEN (2 GiB), then keep the original fast resize + read_exact path. Real Arrow IPC metadata (FlatBuffer schema descriptor) stays well under 1 MiB even for very wide tables; single record batches above 2 GiB are rare and exceed usize-on-32-bit anyway. Inputs above these limits reject with ParseError before any large allocation, defusing the 4-byte-header > multi-GB-resize OOM the PR was opened to fix. Local x86_64 bench (StreamReader/no_validation/read_10): main: 74.7 us (baseline) prev PR HEAD: 138.9 us (+86%) this commit: 74.3 us (within noise of main) --- arrow-ipc/src/reader.rs | 74 +++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 48 deletions(-) diff --git a/arrow-ipc/src/reader.rs b/arrow-ipc/src/reader.rs index 40e93f4d852e..56e8719aa976 100644 --- a/arrow-ipc/src/reader.rs +++ b/arrow-ipc/src/reader.rs @@ -1794,37 +1794,15 @@ pub(crate) enum IpcMessage { }, } -/// Read exactly `body_len` bytes from `reader` into a freshly allocated -/// [`MutableBuffer`], growing the buffer in 64 KiB chunks. -/// -/// This deliberately avoids the previous `MutableBuffer::from_len_zeroed(body_len)` -/// pattern: when `body_len` is derived from an untrusted IPC message header -/// that pattern allowed a 4-byte input (claiming a 1.2 GiB body) to drive a -/// 1.2 GiB up-front allocation before any read failure could surface. The -/// chunked path here means the high-water-mark allocation is tied to the -/// number of bytes actually delivered by `reader`, while still preserving -/// the cache-line alignment that downstream Arrow consumers rely on. -fn read_body_into_buffer( - reader: &mut R, - body_len: usize, -) -> Result { - const CHUNK: usize = 64 * 1024; - let mut buf = MutableBuffer::with_capacity(0); - let mut remaining = body_len; - let mut scratch = [0u8; CHUNK]; - while remaining > 0 { - let want = remaining.min(CHUNK); - let slice = &mut scratch[..want]; - reader.read_exact(slice).map_err(|e| { - ArrowError::ParseError(format!( - "Unexpected EOF reading {body_len} bytes of message body: {e}" - )) - })?; - buf.extend_from_slice(slice); - remaining -= want; - } - Ok(buf) -} +/// Maximum bytes of IPC message metadata we will allocate up front from an +/// untrusted header. Real Arrow IPC metadata is a FlatBuffer schema descriptor +/// — well under 1 MiB even for very wide tables — so this is a generous cap. +const MAX_META_LEN: usize = 16 * 1024 * 1024; + +/// Maximum bytes of IPC message body we will allocate up front from an +/// untrusted header. Single Arrow record batches above 2 GiB are rare in +/// practice and would push past `usize`-on-32-bit anyway. +const MAX_BODY_LEN: usize = 2 * 1024 * 1024 * 1024; /// A low-level construct that reads [`Message::Message`]s from a reader while /// re-using a buffer for metadata. This is composed into [`StreamReader`]. @@ -1857,26 +1835,19 @@ impl MessageReader { return Ok(None); }; - // Both `meta_len` and the message's `bodyLength` are derived from - // untrusted input. Eagerly resizing `self.buf` to `meta_len` (or a - // fresh `MutableBuffer` to `bodyLength` further down) means a few - // bytes of attacker-crafted input — e.g. a header that lies and - // claims a 1.2 GiB metadata payload — would force a multi-GB - // up-front allocation before any read failure could bound it. - // - // Read into a buffer that grows as actual bytes arrive instead, so - // the upfront allocation is independent of the (untrusted) declared - // length, then verify we got exactly the requested number of bytes. - self.buf.clear(); - let read = (&mut self.reader) - .take(meta_len as u64) - .read_to_end(&mut self.buf)?; - if read != meta_len { + // Both `meta_len` and the message's `bodyLength` come from untrusted + // input. Reject obviously-out-of-range claims up front so a 4-byte + // header can't force a multi-GB allocation before any read can + // surface the truncation. + if meta_len > MAX_META_LEN { return Err(ArrowError::ParseError(format!( - "Unexpected EOF reading {meta_len} bytes of message metadata, got {read}" + "IPC message metadata length {meta_len} exceeds {MAX_META_LEN}" ))); } + self.buf.resize(meta_len, 0); + self.reader.read_exact(&mut self.buf)?; + let message = crate::root_as_message(self.buf.as_slice()).map_err(|err| { ArrowError::ParseError(format!("Unable to get root as message: {err:?}")) })?; @@ -1885,7 +1856,14 @@ impl MessageReader { let body_len = usize::try_from(body_len_i64).map_err(|_| { ArrowError::ParseError(format!("Invalid message bodyLength: {body_len_i64}")) })?; - let buf = read_body_into_buffer(&mut self.reader, body_len)?; + if body_len > MAX_BODY_LEN { + return Err(ArrowError::ParseError(format!( + "IPC message body length {body_len} exceeds {MAX_BODY_LEN}" + ))); + } + + let mut buf = MutableBuffer::from_len_zeroed(body_len); + self.reader.read_exact(&mut buf)?; Ok(Some((message, buf))) }