Skip to content

Commit 992bd28

Browse files
committed
fix: unbounded body reads on two framings, and a signed plateau bug
MAX_BODY_BYTES was only checked against a DECLARED Content-Length. The close-delimited path (legal under RFC 7230, no length header at all) and the chunked path (each chunk individually under the cap, no limit on how many) both accumulated without bound until the wall-clock deadline - tens of seconds, long enough on loopback to reach gigabytes before the allocator's unconditional abort() kills the whole unattended run. Confirmed independently by three separate audit passes before being fixed here. Both framings now check the running total, not just a declaration, and both regression tests assert on ELAPSED TIME as well as the verdict: a cap enforced only at the read deadline would still pass a check that only looked at the final Malformed, and these fail if the harness ever falls back to waiting out the clock. plateau_check's steadiness test compared a SIGNED drift against a positive threshold: `drift < trend_pct`. Verdict::Steady's own contract is "not moving in any direction that matters", but a negative drift of any magnitude is always less than a positive threshold, so a memory window declining faster than the trend gate allowed was published as settled - the identical magnitude in the growing direction correctly failed the same test. Fixed to compare the magnitude. The first version of the regression test used too steep a decline and passed against the unfixed code for an unrelated reason (it went negative and tripped the separate mean &lt;= 0.0 guard); the real scenario has to stay gentle enough to isolate the comparison being tested. Methodology: ties the passthrough exclusion to the same four-necessity rule already stated for every other setting, rather than a separate list of reasons. A route only counts as out of the box if a gateway's own default deployment reaches it at its documented URL with nothing switched on to enable it - the same bar every other setting is held to, applied the same way to every entrant.
1 parent 6e21c1e commit 992bd28

3 files changed

Lines changed: 134 additions & 2 deletions

File tree

‎engine/src/http.rs‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,19 @@ fn read_exact_deadline(stream: &mut TcpStream, deadline: Instant, n: usize) -> R
184184
fn read_to_close(stream: &mut TcpStream, deadline: Instant) -> ReadOutcome {
185185
let mut buf = Vec::new();
186186
loop {
187+
// THE CAP APPLIES HERE TOO. This framing has no declared length to check up front - the
188+
// peer just streams until it closes the connection, or doesn't - so MAX_BODY_BYTES has to
189+
// be enforced against what has actually accumulated instead. It used to only be checked on
190+
// the declared-Content-Length path, which left this loop and the chunked one below able to
191+
// grow without limit for as long as the deadline allowed: on loopback, tens of seconds is
192+
// enough to reach gigabytes. The allocator's failure handler calls abort() unconditionally,
193+
// so that is not a panic this harness can catch - it is the eight-hour run dying outright,
194+
// and it is exactly what MAX_BODY_BYTES exists to prevent.
195+
if buf.len() > MAX_BODY_BYTES {
196+
return ReadOutcome::Err(io::Error::other(format!(
197+
"close-delimited body exceeded the {MAX_BODY_BYTES} byte cap before the peer closed the connection"
198+
)));
199+
}
187200
let remaining = deadline.saturating_duration_since(Instant::now());
188201
if remaining.is_zero() {
189202
return ReadOutcome::TimedOut(buf);
@@ -322,6 +335,7 @@ fn read_head(
322335
fn read_chunked_body(stream: &mut TcpStream, deadline: Instant, raw: &[u8]) -> Outcome {
323336
let mut body = Vec::new();
324337
let mut seen = raw.to_vec();
338+
let mut chunk_count: u64 = 0;
325339
loop {
326340
let size_line = match read_line(stream, deadline) {
327341
ReadOutcome::Full(line) => line,
@@ -392,6 +406,18 @@ fn read_chunked_body(stream: &mut TcpStream, deadline: Instant, raw: &[u8]) -> O
392406
};
393407
seen.extend_from_slice(&chunk);
394408
body.extend_from_slice(&chunk);
409+
chunk_count += 1;
410+
// THE AGGREGATE, not just each chunk. read_exact_deadline above already rejects any SINGLE
411+
// chunk declared larger than MAX_BODY_BYTES, but nothing stopped an unbounded NUMBER of
412+
// legally-sized chunks: a peer sending chunks just under the cap, back to back, for as long
413+
// as the deadline allows, grew `body` without limit. Checked after appending so the final
414+
// over-cap chunk is still visible in the Malformed evidence.
415+
if body.len() > MAX_BODY_BYTES {
416+
return malformed(
417+
&seen,
418+
format!("chunked body exceeded the {MAX_BODY_BYTES} byte cap across {chunk_count} chunk(s)"),
419+
);
420+
}
395421
// Each chunk body is followed by a bare CRLF before the next chunk size line.
396422
match read_line(stream, deadline) {
397423
ReadOutcome::Full(_) => {}
@@ -1281,6 +1307,82 @@ mod tests {
12811307
);
12821308
}
12831309

1310+
// A CLOSE-DELIMITED BODY HAS NO DECLARATION TO CAP - the peer just streams until it closes the
1311+
// connection, so the only place left to enforce MAX_BODY_BYTES is against what has actually
1312+
// accumulated. This cap was only ever checked against a declared Content-Length, so a peer that
1313+
// omits any framing header (legal: RFC 7230 permits close-delimited responses) and streams past
1314+
// MAX_BODY_BYTES before the deadline grew the buffer without limit until the wall-clock timeout,
1315+
// tens of seconds away - long enough on loopback to reach gigabytes and risk the allocator's
1316+
// unconditional abort() this whole cap exists to avoid.
1317+
#[test]
1318+
fn a_close_delimited_body_past_the_cap_is_rejected_without_waiting_for_the_deadline() {
1319+
let addr = spawn_server(|mut conn| {
1320+
let _ = read_request_head(&conn);
1321+
let _ = conn.write_all(b"HTTP/1.1 200 OK\r\n\r\n");
1322+
// Stream comfortably past MAX_BODY_BYTES (8 MiB) in 64 KiB writes, then stop without
1323+
// closing: if the cap is not enforced against the accumulator, this blocks until the
1324+
// read deadline instead of failing immediately on the byte that crosses the cap.
1325+
let chunk = vec![b'x'; 64 * 1024];
1326+
for _ in 0..170 {
1327+
if conn.write_all(&chunk).is_err() {
1328+
return;
1329+
}
1330+
}
1331+
});
1332+
1333+
let start = Instant::now();
1334+
let outcome = post_json(addr, "/x", b"{}", &[], Duration::from_secs(20));
1335+
match outcome {
1336+
Outcome::Malformed { message, .. } => {
1337+
assert!(message.contains("cap"), "must name the cap it exceeded, got {message:?}")
1338+
}
1339+
other => panic!("a close-delimited body past the cap must be Malformed, got {other:?}"),
1340+
}
1341+
assert!(
1342+
start.elapsed() < Duration::from_secs(10),
1343+
"must be refused as soon as the accumulator crosses the cap, not at the read deadline, took {:?}",
1344+
start.elapsed()
1345+
);
1346+
}
1347+
1348+
// The chunked sibling of the same defect: no single chunk here exceeds MAX_BODY_BYTES (each is
1349+
// legally sized), so the per-chunk check in read_exact_deadline never fires. Only a check on the
1350+
// running total across chunks catches an unbounded NUMBER of legally-sized chunks.
1351+
#[test]
1352+
fn a_chunked_body_past_the_cap_is_rejected_without_waiting_for_the_deadline() {
1353+
let addr = spawn_server(|mut conn| {
1354+
let _ = read_request_head(&conn);
1355+
let _ = conn.write_all(b"HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n");
1356+
let chunk = vec![b'x'; 64 * 1024];
1357+
for _ in 0..170 {
1358+
let head = format!("{:x}\r\n", chunk.len());
1359+
if conn.write_all(head.as_bytes()).is_err() {
1360+
return;
1361+
}
1362+
if conn.write_all(&chunk).is_err() {
1363+
return;
1364+
}
1365+
if conn.write_all(b"\r\n").is_err() {
1366+
return;
1367+
}
1368+
}
1369+
});
1370+
1371+
let start = Instant::now();
1372+
let outcome = post_json(addr, "/x", b"{}", &[], Duration::from_secs(20));
1373+
match outcome {
1374+
Outcome::Malformed { message, .. } => {
1375+
assert!(message.contains("cap"), "must name the cap it exceeded, got {message:?}")
1376+
}
1377+
other => panic!("a chunked body past the cap must be Malformed, got {other:?}"),
1378+
}
1379+
assert!(
1380+
start.elapsed() < Duration::from_secs(10),
1381+
"must be refused as soon as the running total crosses the cap, not at the read deadline, took {:?}",
1382+
start.elapsed()
1383+
);
1384+
}
1385+
12841386
// RFC 7230 section 3.3.3: when both are present, Transfer-Encoding wins and Content-Length is
12851387
// ignored. Getting this backwards truncates the body to the (bogus) declared length AND leaves
12861388
// the chunk framing undecoded, so the caller gets chunk-size lines inside what it believes is

‎engine/src/stats.rs‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,13 @@ pub fn plateau_check(samples: &[Sample], window_s: f64, trend_pct: f64, range_pc
133133
let hi = win.iter().map(|s| s.mib).fold(f64::NEG_INFINITY, f64::max);
134134
let spread = (hi - lo) / mean * 100.0;
135135

136-
if drift < trend_pct && spread < range_pct {
136+
// MAGNITUDE, NOT SIGN. `drift` is signed - positive when the window is still climbing, negative
137+
// when it is declining - but `Verdict::Steady`'s own contract is "not moving in any direction
138+
// that matters". Comparing the signed value against a positive threshold bounded growth only:
139+
// any decline, however steep, satisfied `drift < trend_pct` because a negative number is always
140+
// less than a positive one. A window whose second half ran measurably BELOW its first half -
141+
// a real, non-noise decline bigger than the configured trend gate - was published as settled.
142+
if drift.abs() < trend_pct && spread < range_pct {
137143
Verdict::Steady
138144
} else {
139145
Verdict::NotSteady { growth_rate_mib_per_min }
@@ -367,6 +373,30 @@ mod tests {
367373
(0..n).map(|i| Sample::new(i as f64 * dt_s, base + per_sample * i as f64)).collect()
368374
}
369375

376+
// A DECLINE MUST FAIL THE TREND TEST EXACTLY AS A CLIMB DOES. `drift` is signed; a window whose
377+
// second half runs measurably below its first half is not "steady", it is declining, and
378+
// Verdict::Steady's own doc says "not moving in any direction that matters". A comparison that
379+
// only bounds positive drift would call every decline steady regardless of how fast it fell.
380+
#[test]
381+
fn a_declining_window_fails_the_trend_test_at_the_same_magnitude_as_a_climbing_one() {
382+
// Gentle on purpose: the mean must stay well clear of zero, or the window trips the earlier
383+
// `mean &lt;= 0.0` short-circuit and returns NotSteady for an unrelated reason, masking whether
384+
// the trend comparison itself is symmetric. A ~1.5% decline on a 100 MiB base, against a 1%
385+
// trend gate and a wide range gate (so only the trend test is in play).
386+
let declining = linear_series(100.0, -18.0, 6.0, 12); // ~100.0 -> ~98.1 MiB over the window
387+
let verdict = plateau_check(&declining, 60.0, 1.0, 1.0e9);
388+
assert!(
389+
matches!(verdict, Verdict::NotSteady { .. }),
390+
"a real decline bigger than the trend gate must not be published as settled, got {verdict:?}"
391+
);
392+
393+
// The mirror image at the identical magnitude climbs instead, and must fail the same way -
394+
// proving the test is symmetric rather than just asserting decline is special-cased.
395+
let climbing = linear_series(100.0, 18.0, 6.0, 12);
396+
let climbing_verdict = plateau_check(&climbing, 60.0, 1.0, 1.0e9);
397+
assert!(matches!(climbing_verdict, Verdict::NotSteady { .. }));
398+
}
399+
370400
// Largest rate (MiB/min) that still certifies Steady at this window, found by bisection with the
371401
// range gate disabled (huge range_pct) so only the trend test is in play.
372402
fn boundary_rate(window_s: f64, n: usize, trend_pct: f64, base: f64) -> f64 {

‎site/index.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ <h3>Where this can still be wrong</h3>
217217

218218
<h3>Why we measure the unified route</h3>
219219
<p>Every gateway is measured on its unified OpenAI-compatible route, the endpoint a stock SDK points at, on the same terms as every other gateway. Some gateways also expose a passthrough route: a provider-specific endpoint that forwards to one named upstream and skips the gateway's own translation. It is faster, and it is a different product.</p>
220-
<p>We do not measure it, for three reasons. It is not the path most integrations take, since using it means changing your code and hard-coding which provider you call, whereas the unified endpoint is the point-your-SDK-here experience these gateways are built to sell. Not every gateway has one, so a passthrough column would pit a special route on some entrants against the normal route on others. And it is pinned to a single provider by construction, so calls on it bypass the cross-provider routing and failover that are the reason to deploy a gateway in the first place. The code is public if you want to check any of this: a run is driven by <a id="lnk-matrix" target="_blank" rel="noopener">run-on-ec2.sh</a>, and every measurement, the memory window included, is taken by <a id="lnk-memory" target="_blank" rel="noopener">the engine</a>.</p>
220+
<p>The reason is the same rule that governs every other setting: a gateway is measured out of the box, with no configuration beyond what boots it, points it at the test mock, exposes the ingress path the matrix exercises, or binds the port and cores the rig requires. Where a dedicated passthrough route is not the endpoint a stock SDK reaches by default, calling it means adding configuration or a different URL to get there, which fails that same bar. A passthrough column would also pit a special route on the gateways that have one against the normal route on the gateways that do not, and it is pinned to a single provider by construction, so it bypasses the cross-provider routing and failover that are the reason to deploy a gateway in the first place. Any route that a gateway reaches on its own default deployment, at its documented default URL, with nothing switched on to enable it, is simply what that gateway does out of the box, and is measured like everything else. The code is public if you want to check any of this: a run is driven by <a id="lnk-matrix" target="_blank" rel="noopener">run-on-ec2.sh</a>, and every measurement, the memory window included, is taken by <a id="lnk-memory" target="_blank" rel="noopener">the engine</a>.</p>
221221
</article>
222222

223223
<h3 class="charts-heading">Charts</h3>

0 commit comments

Comments
 (0)