Skip to content

fix(zlib): prevent use-after-free in WorkPool compression operations #28250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 5 commits into from
Closed

fix(zlib): prevent use-after-free in WorkPool compression operations #28250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2dbecdd
fix(zlib): prevent use-after-free in WorkPool compression operations
Mar 18, 2026
7b3a1c5
fix(zlib): address review - remove setTimeout watchdog from test
Mar 18, 2026
5334fb6
ci: retrigger build after cancellation
Mar 18, 2026
18f2f72
fix(zlib): add error handler on http.get request in test
Mar 18, 2026
469a0d4
ci: retrigger after cancellation
Mar 18, 2026
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
15 changes: 15 additions & 0 deletions src/bun.js/node/node_zlib_binding.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,14 @@ pub fn CompressionStream(comptime T: type) type {
// And make sure to clear it when we are done.
this.this_value.set(globalThis, this_value);

// Hold strong references to the input/output buffers to prevent
// GC from collecting them while the WorkPool thread is using the
// underlying memory.
if (!arguments[1].isNull()) {
this.in_buf_value.set(globalThis, arguments[1]);
}
this.out_buf_value.set(globalThis, arguments[4]);

const vm = globalThis.bunVM();
this.task = .{ .callback = &AsyncJob.runTask };
this.poll_ref.ref(vm);
Expand Down Expand Up @@ -149,6 +157,11 @@ pub fn CompressionStream(comptime T: type) type {

this.write_in_progress = false;

// Release the strong references to the input/output buffers now
// that the WorkPool thread is done using them.
this.in_buf_value.deinit();
this.out_buf_value.deinit();

// Clear the strong handle before we call any callbacks.
const this_value = this.this_value.trySwap() orelse {
debug("this_value is null in runFromJSThread", .{});
Expand Down Expand Up @@ -267,6 +280,8 @@ pub fn CompressionStream(comptime T: type) type {
this.pending_close = false;
this.closed = true;
this.this_value.deinit();
this.in_buf_value.deinit();
this.out_buf_value.deinit();
this.stream.close();
}

Expand Down
5 changes: 5 additions & 0 deletions src/bun.js/node/zlib/NativeBrotli.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ stream: Context = .{},
write_result: ?[*]u32 = null,
poll_ref: CountedKeepAlive = .{},
this_value: jsc.Strong.Optional = .empty,
/// Strong references to input/output buffers to prevent GC during async WorkPool operations.
in_buf_value: jsc.Strong.Optional = .empty,
out_buf_value: jsc.Strong.Optional = .empty,
write_in_progress: bool = false,
pending_close: bool = false,
closed: bool = false,
Expand Down Expand Up @@ -110,6 +113,8 @@ pub fn params(this: *@This(), globalThis: *jsc.JSGlobalObject, callframe: *jsc.C

fn deinit(this: *@This()) void {
this.this_value.deinit();
this.in_buf_value.deinit();
this.out_buf_value.deinit();
this.poll_ref.deinit();
switch (this.stream.mode) {
.BROTLI_ENCODE, .BROTLI_DECODE => this.stream.close(),
Expand Down
5 changes: 5 additions & 0 deletions src/bun.js/node/zlib/NativeZlib.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ stream: Context = .{},
write_result: ?[*]u32 = null,
poll_ref: CountedKeepAlive = .{},
this_value: jsc.Strong.Optional = .empty,
/// Strong references to input/output buffers to prevent GC during async WorkPool operations.
in_buf_value: jsc.Strong.Optional = .empty,
out_buf_value: jsc.Strong.Optional = .empty,
write_in_progress: bool = false,
pending_close: bool = false,
closed: bool = false,
Expand Down Expand Up @@ -107,6 +110,8 @@ pub fn params(this: *@This(), globalThis: *jsc.JSGlobalObject, callframe: *jsc.C

fn deinit(this: *@This()) void {
this.this_value.deinit();
this.in_buf_value.deinit();
this.out_buf_value.deinit();
this.poll_ref.deinit();
this.stream.close();
bun.destroy(this);
Expand Down
6 changes: 6 additions & 0 deletions src/bun.js/node/zlib/NativeZstd.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ stream: Context = .{},
write_result: ?[*]u32 = null,
poll_ref: CountedKeepAlive = .{},
this_value: jsc.Strong.Optional = .empty,
/// Strong references to input/output buffers to prevent GC during async WorkPool operations.
in_buf_value: jsc.Strong.Optional = .empty,
out_buf_value: jsc.Strong.Optional = .empty,
write_in_progress: bool = false,
pending_close: bool = false,
closed: bool = false,
Expand Down Expand Up @@ -111,6 +114,9 @@ pub fn params(this: *@This(), globalThis: *jsc.JSGlobalObject, callframe: *jsc.C
}

fn deinit(this: *@This()) void {
this.this_value.deinit();
this.in_buf_value.deinit();
this.out_buf_value.deinit();
this.poll_ref.deinit();
switch (this.stream.mode) {
.ZSTD_COMPRESS, .ZSTD_DECOMPRESS => this.stream.close(),
Expand Down
68 changes: 68 additions & 0 deletions test/regression/issue/22567.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";

// Test that piping HTTP responses through zlib.createGunzip() does not crash.
// Issue #22567: use-after-free when GC collects input/output buffers during
// async WorkPool decompression.
test("pipe HTTP response through createGunzip without crash", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const http = require("node:http");
const zlib = require("node:zlib");

const server = http.createServer((req, res) => {
res.writeHead(200, { "Content-Type": "application/octet-stream" });
const gzip = zlib.createGzip();
gzip.pipe(res);
for (let i = 0; i < 50; i++) {
gzip.write("Line " + i + ": " + "x".repeat(80) + "\\n");

@coderabbitai coderabbitai Bot Mar 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use Buffer.alloc(...).toString() for repetitive test payload content.

Line 21 currently uses "x".repeat(80), which conflicts with repository testing conventions.

♻️ Suggested fix 
-    gzip.write("Line " + i + ": " + "x".repeat(80) + "\\n");
+    gzip.write("Line " + i + ": " + Buffer.alloc(80, "x").toString() + "\\n");

As per coding guidelines: "Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
gzip.write("Line " + i + ": " + "x".repeat(80) + "\\n");
gzip.write("Line " + i + ": " + Buffer.alloc(80, "x").toString() + "\\n");
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/regression/issue/22567.test.ts` at line 21, Replace the repetitive
string construction in the test's gzip.write call: instead of using
"x".repeat(80) produce the 80-byte repeated payload via Buffer.alloc(80,
'x').toString() so the test follows the repository convention; update the
gzip.write invocation that currently builds "Line " + i + ": " + "x".repeat(80)
+ "\\n" to use Buffer.alloc(80, 'x').toString() for the repeated content.

}
gzip.end();
});

server.listen(0, () => {
const port = server.address().port;
let completed = 0;
const total = 3;

for (let i = 0; i < total; i++) {
http.get("http://localhost:" + port + "/", (response) => {
const gunzip = zlib.createGunzip();
const stream = response.pipe(gunzip);
let data = "";
stream.on("data", (chunk) => { data += chunk.toString(); });
stream.on("end", () => {
Bun.gc(true);
completed++;
if (completed >= total) {
console.log("OK");
server.close(() => process.exit(0));
}
});
stream.on("error", (err) => {
console.error("error:", err.message);
process.exit(1);
});
}).on("error", (err) => {
console.error("request error:", err.message);
process.exit(1);
});
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});
`,
],
env: bunEnv,
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

if (exitCode !== 0) {
console.log("stderr:", stderr);
}
expect(stdout.trim()).toBe("OK");
expect(exitCode).toBe(0);
}, 15_000);

@claude claude Bot Mar 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The test sets an explicit 15_000 ms timeout at line 68, violating the project convention in test/CLAUDE.md: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." Remove the second argument from the test() call.

Extended reasoning...

What the issue is

The regression test at test/regression/issue/22567.test.ts line 68 passes 15_000 as the second argument to test(), which sets an explicit 15-second timeout on the test case. The project's test/CLAUDE.md (line 120) explicitly states: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts."

The specific code

Line 68 reads:

}, 15_000);

This is the closing of the test("pipe HTTP response through createGunzip without crash", async () => { ... }, 15_000) call. The 15_000 is the timeout parameter.

Why this matters

While this is a convention/style violation rather than a functional bug, the CLAUDE.md rule exists for good reason: Bun's test runner already provides default timeouts, and explicit timeouts create maintenance burden and inconsistency across the test suite. The rule is marked as "CRITICAL" in the project guidelines.

Impact

The test will still pass correctly with or without the explicit timeout. This is purely a convention violation. It's worth noting that ~20 other regression test files also have explicit timeouts, so this is a widespread pattern, but new code should follow the documented convention.

How to fix

Simply remove the , 15_000 argument from line 68, changing:

}, 15_000);

to:

});

Bun's default test timeout will handle the case where the test hangs.

Loading