Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions src/http/InternalState.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,9 +286,12 @@

// gzip stores the size of the uncompressed data in the last 4 bytes of the stream
// But it's only valid if the stream is less than 4.7 GB, since it's 4 bytes.
// If we know that the stream is going to be larger than our
// pre-allocated buffer, then let's dynamically allocate the exact
// size.
// When the trailer gives us a plausible size, decompress straight
// into the caller's buffer instead of going through the shared
// scratch buffer and copying the whole body a second time. A lying
// trailer is harmless: the reservation is capped at 32 MB, and an
// undersized reservation makes decompress_to_vec fail with
// InsufficientSpace, which falls through to the streaming slow path.
if self.encoding == Encoding::Gzip
&& buffer.len() > 16
&& buffer.len() < 1024 * 1024 * 1024
Expand All @@ -299,9 +302,7 @@
.expect("infallible: size matches"),
);
// Since this is arbtirary input from the internet, let's set an upper bound of 32 MB for the allocation size.
if (estimated_size as usize) > deflater.shared_buffer.len()
&& estimated_size < 32 * 1024 * 1024
{
if estimated_size > 0 && estimated_size < 32 * 1024 * 1024 {
body_out_str.list.reserve_exact(
(estimated_size as usize).saturating_sub(body_out_str.list.len()),
);
Expand All @@ -311,8 +312,26 @@
&mut body_out_str.list,
bun_libdeflate::Encoding::Gzip,
);
if result.status == bun_libdeflate::Status::Success {
// Trailing bytes after the gzip stream mean the ISIZE
// we reserved from wasn't the real trailer — fall to
// the streaming slow path, which handles multi-member
// streams and rejects garbage.
if result.status == bun_libdeflate::Status::Success
&& result.read == buffer.len()
{
still_needs_to_decompress = false;
// The ISIZE trailer is attacker-controlled: a
// lying value reserves up to 32 MB for a tiny
// body, and the buffer is later adopted as-is
// into JS objects whose GC accounting sees only
// `len`. Right-size grossly oversized
// reservations before they leave the HTTP layer.
let list = &mut body_out_str.list;
if list.capacity() > list.len().saturating_mul(2)
&& list.capacity() - list.len() > 64 * 1024
{
list.shrink_to_fit();
}

Check failure on line 334 in src/http/InternalState.rs

View check run for this annotation

Claude / Claude Code Review

shrink_to_fit gated on read==buffer.len(), so lying-ISIZE-via-trailing-data reservation is never shrunk

The `shrink_to_fit` is gated on `result.read == buffer.len()`, but the only way `cap >> len` can happen here is exactly when that guard is **false** (Success + trailing data, as in the new "lying huge ISIZE" RSS test) — so the over-reserved Vec falls through to the slow path unshrunk and is then swapped into the JS body with ~30 MB capacity behind a 17-byte view. Conversely, when `read == buffer.len()` the buffer was a single valid gzip member, libdeflate has verified ISIZE == output length, so
Comment thread
robobun marked this conversation as resolved.
}

break 'libdeflate;
Expand Down
22 changes: 17 additions & 5 deletions src/runtime/webcore/fetch/FetchTasklet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2275,11 +2275,23 @@ impl FetchTasklet {
}
} else {
if success {
bun_core::handle_oom(
task_ref
.scheduled_response_buffer
.write(task_ref.response_buffer.list.as_slice()),
);
if task_ref.scheduled_response_buffer.list.is_empty() && !task_ref.result.has_more {
// Final delivery into an empty scheduled buffer — the common
// buffered-response case, where this callback carries the
// complete body. Hand the accumulated bytes over instead of
// copying them. Intermediate streaming chunks keep the copy
// below so `response_buffer` retains its capacity for reuse.
core::mem::swap(
&mut task_ref.scheduled_response_buffer.list,
&mut task_ref.response_buffer.list,
);
} else {
bun_core::handle_oom(
task_ref
.scheduled_response_buffer
.write(task_ref.response_buffer.list.as_slice()),
);
}
}
// reset for reuse
task_ref.response_buffer.reset();
Expand Down
114 changes: 113 additions & 1 deletion test/js/web/fetch/fetch-gzip.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Socket } from "bun";
import { beforeAll, describe, expect, it } from "bun:test";
import { gcTick } from "harness";
import { bunEnv, bunExe, gcTick } from "harness";
import { once } from "node:events";
import { createServer } from "node:http";
import { createServer as createNetServer } from "node:net";
Expand Down Expand Up @@ -297,6 +297,57 @@
done();
});

describe("gzip response edge cases", () => {
// Behavior pins for the libdeflate fast path and its fallbacks: honest
// streams decode byte-exactly; integrity violations (which both libdeflate
// and zlib verify) reject with ZlibError regardless of which decode path
// ran. The corrupted-trailer cases cover each exit from the exact-size
// reservation: `crc-corrupt` enters it and falls through on BadData,
// `isize-undersized` enters it with a reservation too small for the actual
// data and falls through on InsufficientSpace, and `isize-oversized`
// (~4.28 GB trailer) is rejected by the 32 MB cap and takes the shared
// scratch-buffer path instead.
const payload = Buffer.alloc(300 * 1024);
for (let i = 0; i < payload.length; i++) payload[i] = (i * 13) & 0xff;

function corrupt(data: Buffer, offsetFromEnd: number) {
const gz = Buffer.from(Bun.gzipSync(data));
gz[gz.length - offsetFromEnd] ^= 0xff;
return gz;
}

const cases: Record<string, { body: Uint8Array; expected: Buffer | "error" }> = {
"honest-large": { body: Bun.gzipSync(payload), expected: payload },
"honest-small": { body: Bun.gzipSync(Buffer.from("hello gzip world")), expected: Buffer.from("hello gzip world") },
"empty": { body: Bun.gzipSync(Buffer.alloc(0)), expected: Buffer.alloc(0) },
// ISIZE trailer is the last 4 bytes, little-endian; 300 KiB = 0x0004B000.
// Flipping the MSB yields 0xFF04B000 (> 32 MB cap); flipping the second
// byte yields 0x00044F00 = 282368 (< actual 307200, so the exact-size
// reservation comes up short).
"isize-oversized": { body: corrupt(payload, 1), expected: "error" },
"isize-undersized": { body: corrupt(payload, 3), expected: "error" },
"crc-corrupt": { body: corrupt(payload, 8), expected: "error" },
"truncated": { body: Buffer.from(Bun.gzipSync(payload)).subarray(0, 1000), expected: "error" },
};

for (const [name, c] of Object.entries(cases)) {
it(`decodes or rejects: ${name}`, async () => {
using server = Bun.serve({
port: 0,
fetch: () => new Response(c.body, { headers: { "Content-Encoding": "gzip" } }),
});
if (c.expected === "error") {
// Depending on delivery, the rejection can surface from fetch()
// itself (fully-buffered body) or from reading the body.
await expect(fetch(server.url).then(r => r.arrayBuffer())).rejects.toThrow("ZlibError");
} else {
const got = Buffer.from(await (await fetch(server.url)).arrayBuffer());
expect(Buffer.compare(got, c.expected)).toBe(0);
}
});
}
});

describe("empty compressed responses", () => {
// A response that declares Content-Encoding but sends zero body bytes must
// resolve as an empty body, like Node — not fail with ZlibError.
Expand All @@ -320,3 +371,64 @@
});
}
});

describe("gzip ISIZE trailer handling", () => {
// The 4-byte ISIZE trailer is attacker-controlled input that sizes the
// decompress reservation. A lying value must neither break decoding nor
// leave a grossly oversized allocation pinned behind the body's bytes.
it("gzip with trailing data decodes the first member without corruption", async () => {
// Trailing bytes (a second gzip member) make the first member's ISIZE
// trailer not the buffer's last 4 bytes — the reservation fast path must
// fall through without duplicating or corrupting output. Decoding only
// the first member matches released Bun's (and the slow path's) behavior.
const a = Buffer.from("first-member ");
const b = Buffer.from("second-member");
const body = Buffer.concat([Buffer.from(Bun.gzipSync(a)), Buffer.from(Bun.gzipSync(b))]);
using server = Bun.serve({
port: 0,
fetch: () => new Response(body, { headers: { "Content-Encoding": "gzip" } }),
});
const text = await (await fetch(server.url)).text();
expect(text).toBe("first-member ");
});

it("a lying huge ISIZE on a tiny body does not retain the reservation", async () => {
// 30 MB ISIZE on a ~40-byte body: the response must decode correctly and
// the process must not accumulate ~30 MB per request of GC-invisible
// capacity behind the adopted bytes.
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
// valid tiny gzip + trailing junk whose last 4 bytes spell a huge
// ISIZE: the reservation reads the buffer's final 4 bytes, which with
// trailing data are attacker-chosen (the real trailer is intact, so
// the stream itself still decodes)
const lie = Buffer.alloc(4);
lie.writeUInt32LE(30 * 1024 * 1024, 0);
const gz = Buffer.concat([Buffer.from(Bun.gzipSync(Buffer.from("tiny body payload"))), lie]);
using server = Bun.serve({
port: 0,
fetch: () => new Response(gz, { headers: { "Content-Encoding": "gzip" } }),
});
const held = [];
for (let i = 0; i < 64; i++) {
held.push(await (await fetch(server.url)).bytes());
if (held[i].length !== 17) throw new Error("bad decode: " + held[i].length);
}
Bun.gc(true);
// 64 lying responses x 30MB capacity would be ~1.9GB; bounded means fixed
console.log(JSON.stringify({ rssMB: Math.round(process.memoryUsage.rss() / 1048576) }));
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
const { rssMB } = JSON.parse(stdout.trim().split("\n").at(-1));
expect(rssMB).toBeLessThan(700);
expect(exitCode).toBe(0);

Check warning on line 432 in test/js/web/fetch/fetch-gzip.test.ts

View check run for this annotation

Claude / Claude Code Review

Lying-ISIZE subprocess test pipes stderr but never reads/asserts it

nit: `stderr: "pipe"` is set but `proc.stderr` is never read. If the child throws (e.g. the `held[i].length !== 17` check, or `fetch()` rejecting), stdout is empty and `JSON.parse("")` fails with an opaque `Unexpected end of JSON input` before the exitCode assertion runs — the real diagnostic in stderr is lost. Per the repo convention for `bunEnv` subprocess tests, include `proc.stderr.text()` in the `Promise.all` and assert `expect(stderr).toBe("")` before parsing stdout / asserting exitCode.
Comment thread
robobun marked this conversation as resolved.
});
});
Loading