Uh oh!
There was an error while loading. Please reload this page.
fix(sources): cap compressed and decompressed payload size to prevent OOM - #134
fix(sources): cap compressed and decompressed payload size to prevent OOM#134harshvardhan-s1 wants to merge 7 commits into
Conversation
janmejay-s1
left a comment
There was a problem hiding this comment.
If we have used any global variables anywhere else, let us delete those too, haven't completed the first pass yet, but adding comments I have gathered so far to unblock.
| use flate2::read::{MultiGzDecoder, ZlibDecoder}; | ||
| /// Default cap on the size of any decompressed payload. | ||
| /// | ||
| /// Prevents a compressed "bomb" from causing unbounded memory growth. | ||
| pub const DEFAULT_MAX_DECOMPRESSED_SIZE_BYTES: usize = 100 * 1024 * 1024; | ||
| static MAX_DECOMPRESSED_SIZE_BYTES: OnceLock<usize> = OnceLock::new(); | ||
| static MAX_ZLIB_COMPRESSED_FRAME_SIZE_BYTES: OnceLock<usize> = OnceLock::new(); | ||
| static MAX_ZSTD_WINDOW_LOG: OnceLock<Option<u32>> = OnceLock::new(); | ||
| /// Maps a decompressed cap to the largest compressed frame that can legitimately produce output | ||
| /// within it, using zlib's worst-case expansion of 13.5% + 11 bytes. This lets us reject an | ||
| /// oversized declared payload before buffering it, without rejecting a valid frame whose | ||
| /// decompressed content stays within the decompressed cap. | ||
| /// | ||
| /// See <https://zlib.net/zlib_tech.html> ("the worst case ... can result in an expansion of at | ||
| /// most 13.5%, plus eleven bytes"). | ||
| #[allow(clippy::cast_possible_truncation)] // limit derives from a usize; saturating math keeps it in range | ||
| const fn zlib_compressed_frame_limit(decompressed_limit: usize) -> usize { | ||
| (decompressed_limit as u64) | ||
| .saturating_mul(1135) | ||
| .saturating_div(1000) | ||
| .saturating_add(11) as usize | ||
| } | ||
| const DEFAULT_MAX_ZLIB_COMPRESSED_FRAME_SIZE_BYTES: usize = | ||
| zlib_compressed_frame_limit(DEFAULT_MAX_DECOMPRESSED_SIZE_BYTES); | ||
| const DEFAULT_MAX_ZSTD_WINDOW_LOG: Option<u32> = | ||
| zstd_window_log_max(DEFAULT_MAX_DECOMPRESSED_SIZE_BYTES); | ||
| /// Override the global decompressed payload size cap. Must be called before any sources start. | ||
| /// | ||
| /// # Panics | ||
| /// |
There was a problem hiding this comment.
Let us delete all these global variables and use GlobalOptions (lib/vector-core/src/config/global_options.rs:40) to pass along compression cap.
GlobalOptions are a part of all context-objects (SourceContext, SinkContext and TransformContext), so we can readily access it in any component that needs this.
| /// # Panics | ||
| /// | ||
| /// Panics if called more than once, as the global cap may only be initialized a single time. | ||
| pub fn set_max_decompressed_size_bytes(size: usize) { |
There was a problem hiding this comment.
Let us kill the setter, any place that needs these configs should read from global-options.
Let us create a type and add as many configs as we need in that and make it optional and set defaults for it, like so:
#[configurable_component]#[derive(Clone,Debug,Default,PartialEq)]pubstructGlobalOptions{/// ...#[serde(default)]limits:OperationalLimits}And we can have all these methods set the defaults correctly.
Eg.
#[derive(Clone,Copy,Debug,Default,Eq,PartialEq)]pubstructOperationalLimits{compression:CompressionLimits}implDefaultforCompressionLimits{///.....}etc.
| impl<S: Read> CappedDecoder<MultiGzDecoder<S>> { | ||
| /// Creates a capped gzip decoder using the global decompressed-size cap. | ||
| pub fn gzip(reader: S) -> Self { |
There was a problem hiding this comment.
Add param here ... gzip(reader: S, cl: &CompressionLimits) -> ...
| impl<S: Read> CappedDecoder<ZlibDecoder<S>> { | ||
| /// Creates a capped zlib/deflate decoder using the global decompressed-size cap. | ||
| pub fn zlib(reader: S) -> Self { |
| /// # Errors | ||
| /// | ||
| /// Returns an error if the zstd decoder cannot be initialized (e.g. invalid header). | ||
| pub fn zstd_with_limit(reader: S, limit: usize) -> io::Result<Self> { |
There was a problem hiding this comment.
delete this or make this private (if needed for testing)
| /// # Errors | ||
| /// | ||
| /// Returns an error if the zstd decoder cannot be initialized (e.g. invalid header). | ||
| pub fn zstd_http_with_limit(reader: S, limit: usize) -> io::Result<Self> { |
| fn zstd_with_window_log( | ||
| reader: S, | ||
| limit: usize, |
There was a problem hiding this comment.
replace this with &CompressionLimits
| /// The cap is the global decompressed-size limit ([`max_decompressed_size_bytes`]): it bounds the | ||
| /// raw (still-compressed) body a source buffers before decompression, so a large upload cannot | ||
| /// drive unbounded allocation independently of the decompressed-size cap. | ||
| pub(crate) fn capped_body() -> BoxedFilter<(Bytes,)> { |
There was a problem hiding this comment.
accept &CompressionLimits here as a param
| /// Supports gzip, deflate, snappy, zstd, and identity (no compression). | ||
| /// | ||
| /// Caps the decompressed output at the global limit to mitigate decompression-bomb DoS attacks. | ||
| pub fn decode(header: Option<&str>, body: Bytes) -> Result<Bytes, ErrorMessage> { |
There was a problem hiding this comment.
propagate upwards in all these call-sites
| } | ||
| /// Like [`decode`], but allows the caller to control the decompressed size cap. | ||
| fn decode_with_limit( |
There was a problem hiding this comment.
Inline this and rely on callers / tests passing in &CompressionLimits
This is a smell that the public method relies on state that the test can't easily control, which is bad design, when this pattern starts to emerge repeatedly, its better to re-evaluate the design.
| } | ||
| fn handle_decode_error(encoding: &str, error: impl std::error::Error) -> ErrorMessage { | ||
| fn decompress_snappy(body: &Bytes, max_decompressed_size: usize) -> Result<Bytes, ErrorMessage> { |
| fn request_body_too_large_error(max: usize) -> ErrorMessage { | ||
| ErrorMessage::new( | ||
| StatusCode::PAYLOAD_TOO_LARGE, | ||
| format!("Request body exceeds limit of {} bytes.", max), | ||
| ) | ||
| } | ||
| fn decompressed_too_large_error(encoding: &str, max: usize) -> ErrorMessage { | ||
| ErrorMessage::new( | ||
| StatusCode::PAYLOAD_TOO_LARGE, | ||
| format!( | ||
| "Decompressed {} body exceeds limit of {} bytes.", | ||
| encoding, max | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
avoid accepting usize for such cases, accept &CompressionLimits instead
janmejay-s1
left a comment
There was a problem hiding this comment.
Inject &CompressionLimits here: lib/codecs/src/actions/decoding/config.rs:44
janmejay-s1
left a comment
There was a problem hiding this comment.
partial review, continuing
| /// Returns the currently configured decompressed payload size cap. | ||
| pub fn max_decompressed_size_bytes() -> usize { | ||
| *MAX_DECOMPRESSED_SIZE_BYTES | ||
| .get() | ||
| .unwrap_or(&DEFAULT_MAX_DECOMPRESSED_SIZE_BYTES) | ||
| } | ||
| /// Returns the maximum compressed frame wire size we are willing to buffer, derived from the | ||
| /// decompressed cap plus zlib's worst-case expansion. See `zlib_compressed_frame_limit`. | ||
| pub fn max_zlib_compressed_frame_size_bytes() -> usize { | ||
| *MAX_ZLIB_COMPRESSED_FRAME_SIZE_BYTES | ||
| .get() | ||
| .unwrap_or(&DEFAULT_MAX_ZLIB_COMPRESSED_FRAME_SIZE_BYTES) | ||
| } |
There was a problem hiding this comment.
These methods can simply be deleted in favor of attributes on CompressionLimits
| /// that on top. | ||
| #[must_use] | ||
| #[allow(clippy::manual_clamp)] // `usize::clamp` is not a const fn; the manual form keeps this const | ||
| pub const fn zstd_window_log_max(max_decompressed_size: usize) -> Option<u32> { |
There was a problem hiding this comment.
Let us not use free-standing global functions like this, let us make these methods on CompressionLimits.
| } | ||
| impl<R: Read> CappedDecoder<R> { | ||
| fn with_limit(reader: R, limit: usize) -> Self { |
There was a problem hiding this comment.
Accept &CompressionLimits here instead of usize
| pub fn max_zstd_window_log() -> Option<u32> { | ||
| MAX_ZSTD_WINDOW_LOG | ||
| .get() | ||
| .copied() | ||
| .unwrap_or(DEFAULT_MAX_ZSTD_WINDOW_LOG) | ||
| } |
There was a problem hiding this comment.
Replace this and others with direct use of attribute to avoid trivial getters.
| @@ -206,18 +206,10 @@ impl Decompressor { | |||
| pub fn decompress(&self, bytes: Bytes) -> io::Result<Bytes> { | |||
There was a problem hiding this comment.
In this case it may be best to change
implFrom<Compression>forDecompressor{to
implFrom<(Compression,&'aCompressionLimits)>forDecompressor<'a>{But if this leads to an explosion of changes, perhaps it'd be better to
implFrom<(Compression,Arc<CompressionLimits>)>forDecompressor{Callers that have such cases can clone the copy in GlobalOptions and re-use everywhere.
| /// Defaults to the global `--max-decompressed-size-bytes` limit. | ||
| #[configurable(metadata(docs::type_unit = "bytes"))] | ||
| #[serde(default, skip_serializing_if = "vector_lib::serde::is_default")] | ||
| max_frame_bytes: Option<usize>, |
There was a problem hiding this comment.
ditto, simply use the default, make this usize (drop Option)
There was a problem hiding this comment.
and skip serialization if default
| acknowledgements: Default::default(), | ||
| connection_limit: Some(2), | ||
| connection_limit: default_connection_limit(), | ||
| max_frame_bytes: None, |
| struct FluentSource { | ||
| log_namespace: LogNamespace, | ||
| legacy_host_key_path: Option<OwnedValuePath>, | ||
| max_frame_bytes: Option<usize>, |
There was a problem hiding this comment.
drop Option across all usage
| /// Maximum number of bytes that may be buffered while waiting for a complete frame. Bounds | ||
| /// memory against a peer that declares an oversized msgpack structure and streams the bytes to | ||
| /// force unbounded buffering. | ||
| max_frame_size: usize, |
| fn new(log_namespace: LogNamespace, max_frame_bytes: Option<usize>) -> Self { | ||
| Self { | ||
| log_namespace, | ||
| max_frame_size: max_frame_bytes.unwrap_or_else(max_decompressed_size_bytes), |
There was a problem hiding this comment.
let us not do this here, let us make it a default in config
janmejay-s1
left a comment
There was a problem hiding this comment.
partial, continuing to review
| .map(|_| buf) | ||
| .map_err(Into::into) | ||
| } | ||
| Some("gzip") => CappedDecoder::gzip(io::Cursor::new(bin.into_vec())) |
There was a problem hiding this comment.
in such cases we can keep CompressionLimits in FluentSource and add it here just like log_namespace.
fndecoder(&self) -> Self::Decoder{FluentDecoder::new(self.log_namespace)}| /// | ||
| /// A frame within the byte cap can still carry a very large number of tiny entries, each of which | ||
| /// becomes an `Event`; this bounds the burst that one frame can turn into. | ||
| const MAX_ENTRIES_PER_FRAME: usize = 100_000; |
There was a problem hiding this comment.
does protocol support a limit? Wouldn't that be a better default?
| #[configurable(metadata(docs::type_unit = "bytes"))] | ||
| #[serde(default, skip_serializing_if = "vector_lib::serde::is_default")] | ||
| max_frame_bytes: Option<usize>, | ||
There was a problem hiding this comment.
create another config attribute for max_entries_per_frame and use MAX_ENTRIES_PER_FRAME only as a default for that
| /// Bounds how many events one frame may expand into. A frame within the byte cap can still | ||
| /// carry a very large number of tiny entries. | ||
| fn ensure_entry_count(count: usize) -> Result<(), DecodeError> { | ||
| if count > MAX_ENTRIES_PER_FRAME { |
There was a problem hiding this comment.
use a FluentSource attribute for enforcement (not this constant).
| /// | ||
| /// Fluent records are shallow in practice — a tag, a timestamp and a flat map of fields. This | ||
| /// leaves generous headroom for nested objects while keeping recursion far below any stack limit. | ||
| pub(super) const MAX_MSGPACK_DEPTH: usize = 128; |
There was a problem hiding this comment.
Let us expose this in config and use mapper to set a sensible default (we can also have a default here in Rust that sets it to 128). But let us not use hardcoded constants (unless they are protocol specified and even then its better to avoid).
| }) | ||
| .accept_compressed(CompressionEncoding::Gzip) | ||
| .max_decoding_message_size(usize::MAX); | ||
| .max_decoding_message_size(max_decompressed_size_bytes()); |
There was a problem hiding this comment.
directly use CompressionLimits from SourceContext here
| let (byte_size, body) = if gzip { | ||
| MultiGzDecoder::new(body.reader()) | ||
| .read_to_end(&mut data) | ||
| data = CappedDecoder::gzip(body.reader()) |
| match MultiGzDecoder::new(bytes.reader()).read_to_end(&mut data) { | ||
| Ok(0) => return Err(ApiError::NoData.into()), | ||
| Ok(_) => Value::from(Bytes::from(data)), | ||
| match CappedDecoder::gzip(bytes.reader()).decompress() { |
| .and(warp::header::optional::<String>("X-Forwarded-For")) | ||
| .and(self.gzip()) | ||
| .and(warp::body::bytes()) | ||
| .and(capped_body()) |
| /// test catches a systematic regression (a stall waiting on end-of-stream would cost hundreds | ||
| /// of ms) without tripping on CI scheduling noise. The median is used so one stalled sample | ||
| /// cannot fail the run. | ||
| #[tokio::test] |
There was a problem hiding this comment.
let us add a feature to isolate performance tests out, we can run them separately (a dedicated fork in the test-workflow, eventually), for now we can just run them as part of our regular test-suite by enabling the feature in dataplane-build
janmejay-s1
left a comment
There was a problem hiding this comment.
Accepted with comments, please address before merge.
| /// Returns whether `error` was raised because decompression hit the size cap (see | ||
| /// [`DecompressedSizeLimitExceeded`]). | ||
| #[must_use] | ||
| pub fn is_decompressed_size_limit_error(error: &io::Error) -> bool { |
There was a problem hiding this comment.
why not make this a function on the type? We can call like this: DecompLimitError::is(e) etc?
| } | ||
| } | ||
| fn new_decompressor() -> GzDecoder<LimitedWriter> { |
There was a problem hiding this comment.
pass &CompressionLimit here
| // Tonic added a default of 4MB in 0.9. Bound this by the global decompressed-size cap | ||
| // rather than `usize::MAX` so a single oversized message cannot drive unbounded | ||
| // allocation on this unauthenticated listener. | ||
| .max_decoding_message_size(max_decompressed_size_bytes()); |
| } | ||
| } | ||
| const fn new_nested() -> Self { |
| return Ok(None); | ||
| } | ||
| let payload_size = rest.get_u32() as usize; | ||
| let limit = max_decompressed_size_bytes(); |
There was a problem hiding this comment.
pass CompressionLimits from Source (from SourceContext)
| .read_to_end(&mut buf) | ||
| .context(DecompressionFailedSnafu) | ||
| .map(|_| BytesMut::from(&buf[..])); | ||
| let res = CappedDecoder::zlib_with_limit(io::Cursor::new(slice), limit) |
There was a problem hiding this comment.
rename the method to just zlib and accept &CompressionLimits?
| /// Maximum number of bytes allowed after decompressing a payload. | ||
| /// | ||
| /// Sources that decompress incoming payloads (gzip, deflate, zstd) enforce this cap to | ||
| /// prevent a compressed "bomb" from exhausting memory. Payloads whose decompressed size | ||
| /// exceeds the limit are rejected. | ||
| /// | ||
| /// Defaults to 104857600 (100 MiB). Raise this only when sources routinely receive | ||
| /// legitimately large compressed payloads. | ||
| /// | ||
| /// Must be at least 1024; a cap below that would reject essentially all traffic rather than | ||
| /// just bombs. | ||
| #[arg( | ||
| long, | ||
| env = "VECTOR_MAX_DECOMPRESSED_SIZE_BYTES", | ||
| default_value_t = vector_common::decompression::DEFAULT_MAX_DECOMPRESSED_SIZE_BYTES, | ||
| value_parser = parse_max_decompressed_size_bytes, | ||
| )] | ||
| pub max_decompressed_size_bytes: usize, | ||
| } | ||
| /// Lower bound for `--max-decompressed-size-bytes`. | ||
| /// | ||
| /// Guards against a value (notably `0`) that would silently reject all compressed ingestion, which | ||
| /// looks identical to a broken pipeline from the outside. | ||
| const MIN_MAX_DECOMPRESSED_SIZE_BYTES: usize = 1024; | ||
| fn parse_max_decompressed_size_bytes(raw: &str) -> Result<usize, String> { | ||
| let value: usize = raw | ||
| .parse() | ||
| .map_err(|_| format!("`{raw}` is not a valid number of bytes"))?; | ||
| if value < MIN_MAX_DECOMPRESSED_SIZE_BYTES { | ||
| return Err(format!( | ||
| "must be at least {MIN_MAX_DECOMPRESSED_SIZE_BYTES} bytes, got {value}" | ||
| )); | ||
| } | ||
| Ok(value) |
There was a problem hiding this comment.
Don't add this, let us accept this as part of config (GlobalOptions). That'd also have another advantage that we can change it from manager (or even allow users to set a higher cap for self-hosted sites).
| crate::metrics::init_global().expect("metrics initialization failed"); | ||
| vector_common::decompression::set_max_decompressed_size_bytes( |
There was a problem hiding this comment.
let us kill the setter and global entirely
| use super::*; | ||
| fn decompress_payload(payload: Vec<u8>) -> std::io::Result<Vec<u8>> { |
| { path = "zstd::stream::copy_decode", reason = "Use `vector_common::decompression::CappedDecoder::zstd` (or `CappedDecoder::zstd_http` in HTTP contexts) to enforce the decompression size cap." }, | ||
| { path = "zstd::stream::decode_all", reason = "Use `vector_common::decompression::CappedDecoder::zstd` (or `CappedDecoder::zstd_http` in HTTP contexts) to enforce the decompression size cap." }, | ||
| { path = "zstd::bulk::decompress", reason = "Use `vector_common::decompression::CappedDecoder::zstd` (or `CappedDecoder::zstd_http` in HTTP contexts) to enforce the decompression size cap." }, | ||
| { path = "warp::body::bytes", reason = "Reads the whole request body into memory unbounded. Use `crate::sources::util::http::capped_body()` to cap the compressed body size at the global decompressed-size limit." }, | ||
| ] | ||
| disallowed-types = [ | ||
| { path = "once_cell::sync::OnceCell", reason = "Use `std::sync::OnceLock` instead." }, | ||
| { path = "once_cell::unsync::OnceCell", reason = "Use `std::cell::OnceCell` instead." }, | ||
| { path = "once_cell::sync::Lazy", reason = "Use `std::sync::LazyLock` instead." }, | ||
| { path = "once_cell::unsync::Lazy", reason = "Use `std::sync::LazyCell` instead." }, | ||
| { path = "flate2::read::GzDecoder", reason = "Use `vector_common::decompression::CappedDecoder::gzip` to enforce the decompression size cap." }, | ||
| { path = "flate2::read::MultiGzDecoder", reason = "Use `vector_common::decompression::CappedDecoder::gzip` to enforce the decompression size cap." }, | ||
| { path = "flate2::read::ZlibDecoder", reason = "Use `vector_common::decompression::CappedDecoder::zlib` to enforce the decompression size cap." }, | ||
| { path = "flate2::read::DeflateDecoder", reason = "Use `vector_common::decompression::CappedDecoder` for decompression with the size cap." }, | ||
| { path = "zstd::stream::read::Decoder", reason = "Use `vector_common::decompression::CappedDecoder::zstd` (or `CappedDecoder::zstd_http` in HTTP contexts) to enforce the decompression size cap." }, |
There was a problem hiding this comment.
May be add some hint here that limits should be picked from component's context, which has GlobalOptions.
Sources that decompress untrusted input fed client-controlled payloads into
unbounded
read_to_endcalls, letting a small upload drive an arbitrarily largeallocation and OOM-kill the daemon. Port upstream's
CappedDecoder(vectordotdev/vector#25819, commit 3162ed1a2e) into
vector-commonand apply itat every affected call site.
The cap defaults to 100 MiB and is tunable with
--max-decompressed-size-bytes/VECTOR_MAX_DECOMPRESSED_SIZE_BYTES. Oversized payloads are rejected rather thanbuffered. For zstd the decoder window is clamped as well, since a crafted frame
allocates its window before emitting any output and so escapes an output-size
cap entirely; under HTTP the clamp follows RFC 9659's 8 MB ceiling.
Tickets fixed:
OBE-11554
OBE-11237
OBE-11233
OBE-10708 — same call site as OBE-11233
OBE-10706
OBE-10711
OBE-10710
OBE-11557
OBE-11559 - 61b436a logstash malformed frames #25664, #25825