From ca626d179e8756c6c2c74f2aee9b9d9ebcfc756a Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 31 Oct 2026 21:10:43 +0000 Subject: [PATCH 1/4] test(byte-storage): pin that a rejected envelope copies no large payload A complete envelope with a bin32 payload of LARGE_PAYLOAD or more passes the pre-scan and reaches rmp_serde, whose payload visitor copies the payload before a later field rejects the envelope. Fails on main: every case requests at least the payload's length. --- src/byte_storage.rs | 2 +- src/read_allocation_probe.rs | 67 ++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/byte_storage.rs b/src/byte_storage.rs index 786e5ae..6a39f69 100644 --- a/src/byte_storage.rs +++ b/src/byte_storage.rs @@ -69,7 +69,7 @@ const MAX_COMPRESSION_RATIO: u64 = 1000; /// (128 KiB), even doubled, so it is never mmap-ed, and removing it cannot /// move either threshold. #[cfg(all(feature = "compression", feature = "checksum", feature = "messagepack"))] -const LARGE_PAYLOAD: usize = 64 * 1024; +pub(crate) const LARGE_PAYLOAD: usize = 64 * 1024; /// Inputs shorter than this are written as one literals-only LZ4 block instead /// of going through `lz4_flex::compress`. diff --git a/src/read_allocation_probe.rs b/src/read_allocation_probe.rs index ee91acd..faa1e49 100644 --- a/src/read_allocation_probe.rs +++ b/src/read_allocation_probe.rs @@ -151,6 +151,73 @@ mod tests { } } + /// The fallback decode copies a `bin` payload from `LARGE_PAYLOAD` up out + /// of the input only once it has accepted the envelope, as the one-pass + /// decode does. Each envelope here is a complete document, so it passes + /// the pre-scan and reaches `rmp_serde`, which reads the payload before it + /// finds the fault in a later field. + #[test] + fn rejected_envelope_reads_do_not_copy_a_large_payload() { + use crate::byte_storage::LARGE_PAYLOAD; + + // Both sides of the copy threshold. + for payload_len in [LARGE_PAYLOAD, 2 * LARGE_PAYLOAD] { + let mut payload = vec![0xc6]; + payload.extend_from_slice(&(payload_len as u32).to_be_bytes()); + payload.resize(payload.len() + payload_len, 0); + let checksum = [0x98, 1, 2, 3, 4, 5, 6, 7, 8]; + let short_checksum = [0x97, 1, 2, 3, 4, 5, 6, 7]; + let size = [0x01]; + let format = [0xa1, b'f']; + for (name, envelope) in [ + ( + "ends before format", + [&[0x93][..], &payload, &checksum, &size].concat(), + ), + ( + "trailing fifth element", + [&[0x95][..], &payload, &checksum, &size, &format, &[0xc0]].concat(), + ), + ( + "integer format", + [&[0x94][..], &payload, &checksum, &size, &[0x07]].concat(), + ), + ( + "invalid UTF-8 format", + [&[0x94][..], &payload, &checksum, &size, &[0xa1, 0xff]].concat(), + ), + ( + "7-byte checksum", + [&[0x94][..], &payload, &short_checksum, &size, &format].concat(), + ), + ] { + let storage = ByteStorage::new(None); + let (full, full_requested) = bytes_requested(|| storage.retrieve(&envelope)); + let (data, data_requested) = bytes_requested(|| storage.retrieve_data(&envelope)); + for (call, err, requested) in [ + ("retrieve", full.err(), full_requested), + ("retrieve_data", data.err(), data_requested), + ] { + let label = format!("[{name}, {payload_len} B payload] {call}"); + let Some(ByteStorageError::DeserializationFailed(msg)) = err else { + panic!("{label}: expected DeserializationFailed, got {err:?}"); + }; + // A pre-scan rejection never reaches the payload decode, + // so the count below would pass whatever it does. + assert!( + !msg.starts_with("decode pre-scan: "), + "{label}: rejected by the pre-scan, not rmp_serde: {msg}" + ); + assert!( + requested < LARGE_PAYLOAD, + "{label}: requested {requested} B from the allocator, \ + reaching LARGE_PAYLOAD ({msg})" + ); + } + } + } + } + /// On every `msgpack bin` vector, `retrieve_data` allocates what the LZ4 /// decoder does on its own and nothing else, since the one-pass decode /// takes every such vector without allocating, and `retrieve` adds only From aee21463b29d0ee80ae3180b8286f3893dcfbb94 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 31 Oct 2026 21:19:16 +0000 Subject: [PATCH 2/4] fix(byte-storage): copy a large payload only after the fallback decode accepts the envelope The fallback decode's payload visitor copied a bin or str payload of LARGE_PAYLOAD or more as soon as rmp_serde reached it, so an envelope that then failed in a later field (a short array, a bad format, a 7-byte checksum) had already copied its payload. The one-pass decode copies only after it has read all four fields. The visitor now always borrows, and decode_envelope_full applies the copy threshold once rmp_serde returns Ok. Accepted envelopes decode as before: a payload under LARGE_PAYLOAD is borrowed and a longer one is owned. --- README.md | 6 ++++-- SECURITY.md | 10 ++++++++++ src/byte_storage.rs | 27 +++++++++++++++++++++------ src/read_allocation_probe.rs | 10 +++++++--- 4 files changed, 42 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 90fce53..f850df9 100644 --- a/README.md +++ b/README.md @@ -431,8 +431,10 @@ summed-slot message, `decode pre-scan: declares more elements than the input can back`: any decoder, and the pre-scan's end-of-input check, rejects that envelope too, so only the exact message proves the slot check ran. `src/read_allocation_probe.rs` (unit tests only) bounds what the size-cap and -ratio reads allocate, with a positive control, and pins what an accepted -msgpack-`bin` `retrieve_data` and `retrieve` below 64 KiB allocate. +ratio reads allocate, with a positive control, pins what an accepted +msgpack-`bin` `retrieve_data` and `retrieve` below 64 KiB allocate, and +checks that a read rejecting an envelope copies none of a `bin` payload of +64 KiB or more. `tests/wire_format_constructed.rs` runs the fixture's constructed 32-bit ratio-product vector natively and, in CI, on `wasm32-unknown-unknown` under `wasm-bindgen-test`; `tests/wire_format_vectors.rs` decodes the constructed diff --git a/SECURITY.md b/SECURITY.md index 208e3a8..35954d3 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -194,6 +194,16 @@ input, including every input it would reject, to the pre-scan and `rmp_serde`, so a rejection is theirs: a pre-scan rejection carries the error below, and an `rmp_serde` rejection is `DeserializationFailed` with `rmp_serde`'s message. +Both decodes borrow a `bin` payload under 64 KiB from the input, and copy a +longer one out of it only once the envelope is accepted, so a rejected envelope +costs no payload-sized allocation. `rejected_envelope_reads_do_not_copy_a_large_payload` +in `src/read_allocation_probe.rs` counts what `retrieve` and `retrieve_data` +request from the allocator on five envelopes that pass the pre-scan, carry a +payload of 64 KiB or more, and fail the typed decode in a later field; each read +must request less than 64 KiB. A legacy int-array payload has no bytes to +borrow, so it is decoded into an owned buffer as it is read, rejected or not: +one byte per element, and each element takes at least one byte of input. + The pre-scan rejects: | Rule | Bound | diff --git a/src/byte_storage.rs b/src/byte_storage.rs index 6a39f69..232b901 100644 --- a/src/byte_storage.rs +++ b/src/byte_storage.rs @@ -746,6 +746,7 @@ fn decode_envelope_full( ByteStorageError::DeserializationFailed(format!("decode pre-scan: {what}")) })?; rmp_serde::from_slice(envelope_bytes) + .map(borrowed::StorageEnvelope::copy_large_payload) .map_err(|e| ByteStorageError::DeserializationFailed(e.to_string())) } @@ -834,6 +835,18 @@ mod borrowed { extract_payload(&self.compressed_data, &self.checksum, self.original_size) } + /// Copy a borrowed payload from `LARGE_PAYLOAD` up out of the input, + /// as `decode_one_pass` does once it has read the whole envelope. The + /// serde decode borrows every `bin` and `str` payload, and + /// `decode_envelope_full` calls this only after `rmp_serde` has + /// accepted the envelope, so a rejected envelope copies nothing. + pub(super) fn copy_large_payload(mut self) -> Self { + if let Cow::Borrowed(payload) = self.compressed_data { + self.compressed_data = borrow_below_large(payload); + } + self + } + /// One pass over the shape `rmp_serde` writes for every envelope: a /// fixarray of 4 holding `compressed_data` as bin8, bin16 or bin32; /// `checksum` as a fixarray of 8, each byte a positive fixint or a @@ -920,10 +933,12 @@ mod borrowed { } /// serde_bytes' `ByteBuf` decode, which `StorageEnvelope::compressed_data` - /// uses, except that a `bin` or `str` payload under `LARGE_PAYLOAD` is - /// borrowed. Owned bytes and strings reach `visit_bytes` and `visit_str` - /// through serde's default methods. serde_bytes' own `Cow` decode borrows - /// too, but words its type error differently. + /// uses, except that a `bin` or `str` payload is borrowed, whatever its + /// length: the envelope may still be rejected, and `copy_large_payload` + /// copies one from `LARGE_PAYLOAD` up only once it is accepted. Owned + /// bytes and strings reach `visit_bytes` and `visit_str` through serde's + /// default methods. serde_bytes' own `Cow` decode borrows too, but words + /// its type error differently. fn borrow_payload<'de, D: Deserializer<'de>>( deserializer: D, ) -> Result, D::Error> { @@ -937,11 +952,11 @@ mod borrowed { } fn visit_borrowed_bytes(self, v: &'de [u8]) -> Result { - Ok(borrow_below_large(v)) + Ok(Cow::Borrowed(v)) } fn visit_borrowed_str(self, v: &'de str) -> Result { - Ok(borrow_below_large(v.as_bytes())) + Ok(Cow::Borrowed(v.as_bytes())) } fn visit_bytes(self, v: &[u8]) -> Result { diff --git a/src/read_allocation_probe.rs b/src/read_allocation_probe.rs index faa1e49..7436d18 100644 --- a/src/read_allocation_probe.rs +++ b/src/read_allocation_probe.rs @@ -20,8 +20,12 @@ //! `LARGE_PAYLOAD` allocates: `retrieve_data` requests only what the LZ4 //! decoder does (the one-pass decode reads these envelopes without //! allocating), and `retrieve` adds the format `String` and nothing else. A -//! legacy int-array payload, and a payload from `LARGE_PAYLOAD` up, -//! are copied out of the input by design. +//! payload from `LARGE_PAYLOAD` up is copied out of the input by design, but +//! only once the envelope is accepted: +//! `rejected_envelope_reads_do_not_copy_a_large_payload` checks that a read +//! rejecting such an envelope requests less than `LARGE_PAYLOAD`. A legacy +//! int-array payload is decoded into an owned buffer as it is read, so it is +//! allocated whether or not the envelope is then accepted. //! //! The same count, [`bytes_requested`], also pins the write side: the //! `byte_storage` unit test `store_allocates_only_its_output_and_block` checks @@ -160,7 +164,7 @@ mod tests { fn rejected_envelope_reads_do_not_copy_a_large_payload() { use crate::byte_storage::LARGE_PAYLOAD; - // Both sides of the copy threshold. + // At the copy threshold, and past it. for payload_len in [LARGE_PAYLOAD, 2 * LARGE_PAYLOAD] { let mut payload = vec![0xc6]; payload.extend_from_slice(&(payload_len as u32).to_be_bytes()); From 04631a126cbd74f97db281f677fda452f1195830 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 31 Oct 2026 21:29:24 +0000 Subject: [PATCH 3/4] =?UTF-8?q?fix:=20address=20coderabbit=20review=20?= =?UTF-8?q?=E2=80=94=20scope=20the=20allocation=20claim=20to=20decode=20re?= =?UTF-8?q?jections,=20add=20a=20str32=20case?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docs said a rejected envelope copies no large payload. An envelope the decode accepts is still copied when the size, ratio or checksum check that follows rejects it, which is by design; the README, SECURITY.md and the probe module doc now say "rejected by the decode". The probe test also runs a str32 payload, which reaches the visitor's visit_borrowed_str. CodeRabbit-Resolved: SECURITY.md:198:Update the README allocati CodeRabbit-Resolved: read_allocation_probe.rs:168:Add a str32 case to the --- README.md | 4 ++-- SECURITY.md | 13 ++++++++----- src/byte_storage.rs | 2 +- src/read_allocation_probe.rs | 29 ++++++++++++++++++----------- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index f850df9..0b1e4fb 100644 --- a/README.md +++ b/README.md @@ -433,8 +433,8 @@ envelope too, so only the exact message proves the slot check ran. `src/read_allocation_probe.rs` (unit tests only) bounds what the size-cap and ratio reads allocate, with a positive control, pins what an accepted msgpack-`bin` `retrieve_data` and `retrieve` below 64 KiB allocate, and -checks that a read rejecting an envelope copies none of a `bin` payload of -64 KiB or more. +checks that a read whose envelope decode fails copies none of a `bin` or `str` +payload of 64 KiB or more. `tests/wire_format_constructed.rs` runs the fixture's constructed 32-bit ratio-product vector natively and, in CI, on `wasm32-unknown-unknown` under `wasm-bindgen-test`; `tests/wire_format_vectors.rs` decodes the constructed diff --git a/SECURITY.md b/SECURITY.md index 35954d3..33f29a4 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -195,12 +195,15 @@ so a rejection is theirs: a pre-scan rejection carries the error below, and an `rmp_serde` rejection is `DeserializationFailed` with `rmp_serde`'s message. Both decodes borrow a `bin` payload under 64 KiB from the input, and copy a -longer one out of it only once the envelope is accepted, so a rejected envelope -costs no payload-sized allocation. `rejected_envelope_reads_do_not_copy_a_large_payload` -in `src/read_allocation_probe.rs` counts what `retrieve` and `retrieve_data` +longer one out of it only once the decode has accepted the envelope, so an +envelope the decode rejects costs no payload-sized allocation. +`rejected_envelope_reads_do_not_copy_a_large_payload` in +`src/read_allocation_probe.rs` counts what `retrieve` and `retrieve_data` request from the allocator on five envelopes that pass the pre-scan, carry a -payload of 64 KiB or more, and fail the typed decode in a later field; each read -must request less than 64 KiB. A legacy int-array payload has no bytes to +`bin32` or `str32` payload of 64 KiB or more, and fail the typed decode in a +later field; each read must request less than 64 KiB. An envelope the decode +accepts has its long payload copied even when the size, ratio or checksum +check that follows rejects it. A legacy int-array payload has no bytes to borrow, so it is decoded into an owned buffer as it is read, rejected or not: one byte per element, and each element takes at least one byte of input. diff --git a/src/byte_storage.rs b/src/byte_storage.rs index 232b901..1dc3472 100644 --- a/src/byte_storage.rs +++ b/src/byte_storage.rs @@ -839,7 +839,7 @@ mod borrowed { /// as `decode_one_pass` does once it has read the whole envelope. The /// serde decode borrows every `bin` and `str` payload, and /// `decode_envelope_full` calls this only after `rmp_serde` has - /// accepted the envelope, so a rejected envelope copies nothing. + /// accepted the envelope, so an envelope it rejects copies nothing. pub(super) fn copy_large_payload(mut self) -> Self { if let Cow::Borrowed(payload) = self.compressed_data { self.compressed_data = borrow_below_large(payload); diff --git a/src/read_allocation_probe.rs b/src/read_allocation_probe.rs index 7436d18..9562126 100644 --- a/src/read_allocation_probe.rs +++ b/src/read_allocation_probe.rs @@ -21,9 +21,11 @@ //! decoder does (the one-pass decode reads these envelopes without //! allocating), and `retrieve` adds the format `String` and nothing else. A //! payload from `LARGE_PAYLOAD` up is copied out of the input by design, but -//! only once the envelope is accepted: +//! only once the envelope decode has accepted the envelope: //! `rejected_envelope_reads_do_not_copy_a_large_payload` checks that a read -//! rejecting such an envelope requests less than `LARGE_PAYLOAD`. A legacy +//! whose decode rejects such an envelope requests less than `LARGE_PAYLOAD`. +//! An envelope the decode accepts is copied even when `extract_payload` then +//! rejects it (a size cap, the ratio bound, a checksum mismatch). A legacy //! int-array payload is decoded into an owned buffer as it is read, so it is //! allocated whether or not the envelope is then accepted. //! @@ -155,18 +157,23 @@ mod tests { } } - /// The fallback decode copies a `bin` payload from `LARGE_PAYLOAD` up out - /// of the input only once it has accepted the envelope, as the one-pass - /// decode does. Each envelope here is a complete document, so it passes - /// the pre-scan and reaches `rmp_serde`, which reads the payload before it - /// finds the fault in a later field. + /// The fallback decode copies a `bin` or `str` payload from + /// `LARGE_PAYLOAD` up out of the input only once it has accepted the + /// envelope, as the one-pass decode does. Each envelope here is a complete + /// document, so it passes the pre-scan and reaches `rmp_serde`, which + /// reads the payload before it finds the fault in a later field. #[test] fn rejected_envelope_reads_do_not_copy_a_large_payload() { use crate::byte_storage::LARGE_PAYLOAD; - // At the copy threshold, and past it. - for payload_len in [LARGE_PAYLOAD, 2 * LARGE_PAYLOAD] { - let mut payload = vec![0xc6]; + // bin32 at the copy threshold and past it, and str32 (zero bytes are + // valid UTF-8), which reaches the visitor's other borrowing method. + for (kind, marker, payload_len) in [ + ("bin32", 0xc6, LARGE_PAYLOAD), + ("bin32", 0xc6, 2 * LARGE_PAYLOAD), + ("str32", 0xdb, LARGE_PAYLOAD), + ] { + let mut payload = vec![marker]; payload.extend_from_slice(&(payload_len as u32).to_be_bytes()); payload.resize(payload.len() + payload_len, 0); let checksum = [0x98, 1, 2, 3, 4, 5, 6, 7, 8]; @@ -202,7 +209,7 @@ mod tests { ("retrieve", full.err(), full_requested), ("retrieve_data", data.err(), data_requested), ] { - let label = format!("[{name}, {payload_len} B payload] {call}"); + let label = format!("[{name}, {payload_len} B {kind} payload] {call}"); let Some(ByteStorageError::DeserializationFailed(msg)) = err else { panic!("{label}: expected DeserializationFailed, got {err:?}"); }; From 337256bd8f1731011af50d0d1409a1aaf3aa6d23 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 31 Oct 2026 21:46:34 +0000 Subject: [PATCH 4/4] docs(byte-storage): scope the rejected-envelope claim to the payload copy A rejected read can still allocate in proportion to the input: rmp_serde's error message quotes an offending str field. What the decode guarantees is that it never copies the payload of an envelope it rejects, so SECURITY.md, the probe module doc and copy_large_payload's doc now say that and no more. --- SECURITY.md | 4 ++-- src/byte_storage.rs | 3 ++- src/read_allocation_probe.rs | 9 +++++---- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 33f29a4..d04f8d4 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -195,8 +195,8 @@ so a rejection is theirs: a pre-scan rejection carries the error below, and an `rmp_serde` rejection is `DeserializationFailed` with `rmp_serde`'s message. Both decodes borrow a `bin` payload under 64 KiB from the input, and copy a -longer one out of it only once the decode has accepted the envelope, so an -envelope the decode rejects costs no payload-sized allocation. +longer one out of it only once the decode has accepted the envelope, so the +decode never copies the payload of an envelope it rejects. `rejected_envelope_reads_do_not_copy_a_large_payload` in `src/read_allocation_probe.rs` counts what `retrieve` and `retrieve_data` request from the allocator on five envelopes that pass the pre-scan, carry a diff --git a/src/byte_storage.rs b/src/byte_storage.rs index 1dc3472..c065011 100644 --- a/src/byte_storage.rs +++ b/src/byte_storage.rs @@ -839,7 +839,8 @@ mod borrowed { /// as `decode_one_pass` does once it has read the whole envelope. The /// serde decode borrows every `bin` and `str` payload, and /// `decode_envelope_full` calls this only after `rmp_serde` has - /// accepted the envelope, so an envelope it rejects copies nothing. + /// accepted the envelope, so the payload of an envelope it rejects is + /// never copied. pub(super) fn copy_large_payload(mut self) -> Self { if let Cow::Borrowed(payload) = self.compressed_data { self.compressed_data = borrow_below_large(payload); diff --git a/src/read_allocation_probe.rs b/src/read_allocation_probe.rs index 9562126..4b0d513 100644 --- a/src/read_allocation_probe.rs +++ b/src/read_allocation_probe.rs @@ -22,10 +22,11 @@ //! allocating), and `retrieve` adds the format `String` and nothing else. A //! payload from `LARGE_PAYLOAD` up is copied out of the input by design, but //! only once the envelope decode has accepted the envelope: -//! `rejected_envelope_reads_do_not_copy_a_large_payload` checks that a read -//! whose decode rejects such an envelope requests less than `LARGE_PAYLOAD`. -//! An envelope the decode accepts is copied even when `extract_payload` then -//! rejects it (a size cap, the ratio bound, a checksum mismatch). A legacy +//! `rejected_envelope_reads_do_not_copy_a_large_payload` checks that reading +//! five such envelopes, each rejected by the decode in a later field, +//! requests less than `LARGE_PAYLOAD`. An envelope the decode accepts is +//! copied even when `extract_payload` then rejects it (a size cap, the ratio +//! bound, a checksum mismatch). A legacy //! int-array payload is decoded into an owned buffer as it is read, so it is //! allocated whether or not the envelope is then accepted. //!