Copilot commented on code in PR #582:
URL: https://github.com/apache/avro-rs/pull/582#discussion_r3566636336
##########
avro/src/codec.rs:
##########
@@ -187,34 +203,52 @@ impl Codec {
}
#[cfg(feature = "zstandard")]
Codec::Zstandard(_settings) => {
- use std::io::BufReader;
+ use std::io::{BufReader, Read};
use zstd::zstd_safe;
let mut decoded = Vec::new();
let buffer_size = zstd_safe::DCtx::in_size();
let buffer = BufReader::with_capacity(buffer_size,
&stream[..]);
- let mut decoder =
zstd::Decoder::new(buffer).map_err(Details::ZstdDecompress)?;
- std::io::copy(&mut decoder, &mut
decoded).map_err(Details::ZstdDecompress)?;
+ let decoder =
zstd::Decoder::new(buffer).map_err(Details::ZstdDecompress)?;
+ // Read one byte past the budget so an output that exactly
fills
+ // it is allowed, while a larger (bomb) output is detected.
+ decoder
+ .take((max_bytes as u64).saturating_add(1))
+ .read_to_end(&mut decoded)
+ .map_err(Details::ZstdDecompress)?;
+ if decoded.len() > max_bytes {
+ return Err(Details::MemoryAllocation { desired:
decoded.len(), maximum: max_bytes }.into());
+ }
decoded
}
#[cfg(feature = "bzip")]
Codec::Bzip2(_) => {
use bzip2::read::BzDecoder;
use std::io::Read;
- let mut decoder = BzDecoder::new(&stream[..]);
let mut decoded = Vec::new();
- decoder.read_to_end(&mut decoded).unwrap_or_else(|_|
unreachable!("No I/O errors possible with Vec<u8>"));
+ BzDecoder::new(&stream[..])
+ .take((max_bytes as u64).saturating_add(1))
+ .read_to_end(&mut decoded)
+ .unwrap_or_else(|_| unreachable!("No I/O errors possible
with Vec<u8>"));
Review Comment:
`BzDecoder::read_to_end` can return an error for invalid/corrupt bzip2 data
even when reading from an in-memory slice. Unwrapping here will panic on
malformed input, turning a decode failure into a process abort (DoS). Propagate
the decompression error as an `AvroResult` instead of panicking (likely by
adding a dedicated `Details::Bzip2Decompress(std::io::Error)` variant and
mapping to it).
##########
avro/src/codec.rs:
##########
@@ -187,34 +203,52 @@ impl Codec {
}
#[cfg(feature = "zstandard")]
Codec::Zstandard(_settings) => {
- use std::io::BufReader;
+ use std::io::{BufReader, Read};
use zstd::zstd_safe;
let mut decoded = Vec::new();
let buffer_size = zstd_safe::DCtx::in_size();
let buffer = BufReader::with_capacity(buffer_size,
&stream[..]);
- let mut decoder =
zstd::Decoder::new(buffer).map_err(Details::ZstdDecompress)?;
- std::io::copy(&mut decoder, &mut
decoded).map_err(Details::ZstdDecompress)?;
+ let decoder =
zstd::Decoder::new(buffer).map_err(Details::ZstdDecompress)?;
+ // Read one byte past the budget so an output that exactly
fills
+ // it is allowed, while a larger (bomb) output is detected.
+ decoder
+ .take((max_bytes as u64).saturating_add(1))
+ .read_to_end(&mut decoded)
+ .map_err(Details::ZstdDecompress)?;
+ if decoded.len() > max_bytes {
+ return Err(Details::MemoryAllocation { desired:
decoded.len(), maximum: max_bytes }.into());
+ }
decoded
}
#[cfg(feature = "bzip")]
Codec::Bzip2(_) => {
use bzip2::read::BzDecoder;
use std::io::Read;
- let mut decoder = BzDecoder::new(&stream[..]);
let mut decoded = Vec::new();
- decoder.read_to_end(&mut decoded).unwrap_or_else(|_|
unreachable!("No I/O errors possible with Vec<u8>"));
+ BzDecoder::new(&stream[..])
+ .take((max_bytes as u64).saturating_add(1))
+ .read_to_end(&mut decoded)
+ .unwrap_or_else(|_| unreachable!("No I/O errors possible
with Vec<u8>"));
+ if decoded.len() > max_bytes {
+ return Err(Details::MemoryAllocation { desired:
decoded.len(), maximum: max_bytes }.into());
+ }
decoded
}
#[cfg(feature = "xz")]
Codec::Xz(_) => {
use liblzma::read::XzDecoder;
use std::io::Read;
- let mut decoder = XzDecoder::new(&stream[..]);
let mut decoded: Vec<u8> = Vec::new();
- decoder.read_to_end(&mut decoded).unwrap_or_else(|_|
unreachable!("No I/O errors possible with Vec<u8>"));
+ XzDecoder::new(&stream[..])
+ .take((max_bytes as u64).saturating_add(1))
+ .read_to_end(&mut decoded)
+ .unwrap_or_else(|_| unreachable!("No I/O errors possible
with Vec<u8>"));
Review Comment:
`XzDecoder::read_to_end` can return an error for invalid/corrupt xz data
even when reading from an in-memory slice. Unwrapping here will panic on
malformed input, turning a decode failure into a process abort (DoS). Propagate
the decompression error as an `AvroResult` instead of panicking (likely by
adding a dedicated `Details::XzDecompress(std::io::Error)` variant and mapping
to it).
##########
avro/tests/decompression_bomb.rs:
##########
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Decompression must be bounded by `max_allocation_bytes` so a small
+//! compressed block cannot inflate to an enormous buffer and exhaust memory.
+//!
+//! This is a dedicated integration test so it runs in its own process, where
+//! the (write-once) allocation limit can be set to a small value; the payloads
+//! are then only a few MiB, keeping the test cheap and deterministic.
+
+use apache_avro::Codec;
+
+const BUDGET: usize = 1024 * 1024; // 1 MiB
+const BOMB_PLAINTEXT: usize = 8 * 1024 * 1024; // 8 MiB, well over the budget
+
+fn set_budget() {
+ apache_avro::util::max_allocation_bytes(BUDGET);
+}
+
+/// Compresses `plaintext` bytes with `codec` and asserts the decompressed
+/// output (larger than the budget) is rejected rather than allocated, then
that
+/// a small payload still round-trips.
+fn assert_bounded(codec: Codec) {
+ set_budget();
+
+ let mut bomb = vec![0u8; BOMB_PLAINTEXT];
+ codec.compress(&mut bomb).expect("compress bomb");
+ assert!(
+ bomb.len() <= BUDGET,
+ "compressed bomb ({} bytes) should be far smaller than the budget",
+ bomb.len()
+ );
+ let result = codec.decompress(&mut bomb);
+ assert!(
+ result.is_err(),
+ "a decompressed output exceeding the budget must be rejected, got
{result:?}"
+ );
Review Comment:
This assertion only checks that decompression fails, but it doesn’t confirm
it failed *because* the output exceeded `max_allocation_bytes`. If
decompression fails for another reason (e.g., codec bug/regression), the test
would still pass. Consider asserting the error is specifically
`Details::MemoryAllocation { .. }`.
--
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]
