Skip to content

Commit b10c85c

Browse files
AlexStocksOmX
andcommitted
fix(resp): bound aggregate parser allocations
Reject aggregate declarations above the Redis 8.8.1 INT_MAX boundary and cap initial Array, Map, Set, and Push capacity at 1024 entries so unauthenticated input cannot directly request an unbounded Vec allocation. Add regression coverage for exact boundary overflow, i64 capacity overflow, and the maximum accepted declaration without changing existing null or incomplete-frame semantics. Constraint: This commit changes only the RESP parser and its in-file tests; bulk payload and connection buffer limits remain separate work. Confidence: High; the original test failed with capacity overflow and passed after the shared capacity guard was applied. Scope-risk: Low; normal aggregate parsing is unchanged and all four allocation sites use one private helper. Tested: Windows and WSL cargo test -p resp; Windows and WSL target Clippy; cargo fmt --all -- --check; git diff --check. Not-tested: Full workspace and process-level network suites were not rerun because no network, storage, Cargo, or server code changed. Co-authored-by: OmX <omx@oh-my-codex.dev>
1 parent 727590e commit b10c85c

1 file changed

Lines changed: 67 additions & 4 deletions

File tree

src/resp/src/parse.rs

Lines changed: 67 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ impl Default for RespParse {
6464
}
6565

6666
impl RespParse {
67+
const MAX_AGGREGATE_LENGTH: i64 = i32::MAX as i64;
68+
const MAX_PREALLOCATED_AGGREGATE_LENGTH: usize = 1024;
69+
6770
pub fn new(version: RespVersion) -> Self {
6871
Self {
6972
version,
@@ -82,6 +85,24 @@ impl RespParse {
8285
self.version = version;
8386
}
8487

88+
fn aggregate_capacity(
89+
input: &[u8],
90+
len: i64,
91+
) -> Result<usize, nom::Err<nom::error::Error<&[u8]>>> {
92+
if len > Self::MAX_AGGREGATE_LENGTH {
93+
return Err(nom::Err::Failure(nom::error::Error::new(
94+
input,
95+
nom::error::ErrorKind::Verify,
96+
)));
97+
}
98+
99+
usize::try_from(len)
100+
.map(|len| len.min(Self::MAX_PREALLOCATED_AGGREGATE_LENGTH))
101+
.map_err(|_| {
102+
nom::Err::Failure(nom::error::Error::new(input, nom::error::ErrorKind::Verify))
103+
})
104+
}
105+
85106
/// Detect protocol version from the first byte of input
86107
pub fn detect_version(input: &[u8]) -> RespVersion {
87108
if input.is_empty() {
@@ -212,7 +233,7 @@ impl RespParse {
212233
}
213234

214235
let mut remaining = input;
215-
let mut elements = Vec::with_capacity(len as usize);
236+
let mut elements = Vec::with_capacity(Self::aggregate_capacity(input, len)?);
216237

217238
for _ in 0..len {
218239
let (new_remaining, element) = Self::parse_resp_data(remaining)?;
@@ -353,7 +374,7 @@ impl RespParse {
353374
}
354375

355376
let mut remaining = input;
356-
let mut pairs = Vec::with_capacity(len as usize);
377+
let mut pairs = Vec::with_capacity(Self::aggregate_capacity(input, len)?);
357378

358379
for _ in 0..len {
359380
let (new_remaining, key) = Self::parse_resp_data(remaining)?;
@@ -382,7 +403,7 @@ impl RespParse {
382403
}
383404

384405
let mut remaining = input;
385-
let mut elements = Vec::with_capacity(len as usize);
406+
let mut elements = Vec::with_capacity(Self::aggregate_capacity(input, len)?);
386407

387408
for _ in 0..len {
388409
let (new_remaining, element) = Self::parse_resp_data(remaining)?;
@@ -410,7 +431,7 @@ impl RespParse {
410431
}
411432

412433
let mut remaining = input;
413-
let mut elements = Vec::with_capacity(len as usize);
434+
let mut elements = Vec::with_capacity(Self::aggregate_capacity(input, len)?);
414435

415436
for _ in 0..len {
416437
let (new_remaining, element) = Self::parse_resp_data(remaining)?;
@@ -908,6 +929,48 @@ mod tests {
908929
);
909930
}
910931

932+
// Regression for #395: untrusted aggregate lengths must not drive allocation size.
933+
#[test]
934+
fn test_reject_oversized_aggregate_lengths_without_panicking() {
935+
for frame in [
936+
"*2147483648\r\n",
937+
"%2147483648\r\n",
938+
"~2147483648\r\n",
939+
">2147483648\r\n",
940+
"*9223372036854775807\r\n",
941+
"%9223372036854775807\r\n",
942+
"~9223372036854775807\r\n",
943+
">9223372036854775807\r\n",
944+
] {
945+
let mut parser = RespParse::new(RespVersion::RESP3);
946+
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
947+
parser.parse(Bytes::copy_from_slice(frame.as_bytes()))
948+
}));
949+
950+
assert!(
951+
matches!(result, Ok(RespParseResult::Error(_))),
952+
"expected a parse error for {frame:?}, got {result:?}"
953+
);
954+
}
955+
}
956+
957+
#[test]
958+
fn test_maximum_aggregate_lengths_do_not_preallocate_declared_size() {
959+
for frame in [
960+
"*2147483647\r\n",
961+
"%2147483647\r\n",
962+
"~2147483647\r\n",
963+
">2147483647\r\n",
964+
] {
965+
let mut parser = RespParse::new(RespVersion::RESP3);
966+
assert_eq!(
967+
parser.parse(Bytes::copy_from_slice(frame.as_bytes())),
968+
RespParseResult::Incomplete,
969+
"unexpected parse result for {frame:?}"
970+
);
971+
}
972+
}
973+
911974
#[test]
912975
fn test_auto_detect_resp3() {
913976
let mut parser = RespParse::new(RespVersion::RESP2);

0 commit comments

Comments
 (0)