fix(zlib): prevent use-after-free in WorkPool compression operations#28250
fix(zlib): prevent use-after-free in WorkPool compression operations#28250robobun wants to merge 5 commits into
Conversation
Hold strong GC references to input/output buffer JSValues during async WorkPool decompression to prevent the garbage collector from collecting them while the native thread is still reading/writing the underlying memory. Closes #22567 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated 10:17 AM PT - Mar 18th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 28250That installs a local version of the PR into your bun-28250 --bun |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughHolds strong references to input and output buffers during async zlib/Brotli/Zstd WorkPool operations, releases them after work completes or on close, and adds a regression test that pipes HTTP responses through zlib.createGunzip() to verify no crash. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/22567.test.ts`:
- Line 53: Remove the timer-based watchdog: delete the setTimeout(() => {
console.error("timeout"); process.exit(1); }, 10000) from the spawned test
script and instead rely on the test harness awaiting the completion condition
(e.g., await the child process exit/promise or the specific condition you
already assert). Ensure any spawn logic (the code that calls setTimeout) returns
or awaits the promise that resolves/rejects on the real completion condition so
the test framework's timeout controls failures rather than an explicit
process.exit call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f43cc32f-a540-4b05-be48-72e2066e4f36
📒 Files selected for processing (5)
src/bun.js/node/node_zlib_binding.zigsrc/bun.js/node/zlib/NativeBrotli.zigsrc/bun.js/node/zlib/NativeZlib.zigsrc/bun.js/node/zlib/NativeZstd.zigtest/regression/issue/22567.test.ts
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
Remove the manual setTimeout watchdog from the spawned test subprocess and rely on the test harness timeout instead. The subprocess exits via process.exit(0) in the server.close() callback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/22567.test.ts`:
- 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.
- Around line 32-50: The http.get call can fail before its response callback,
leaving completed unchanged and the test hung; attach an error handler to the
Request object returned by http.get (the same call that currently sets up the
response callback) and on error log the error and call process.exit(1) so
failures surface immediately; ensure you reference the existing completed/total
variables and still call server.close if needed (or exit immediately) to avoid
hanging processes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8e54036-e9c6-424f-8b36-7b7c8f5a1081
📒 Files selected for processing (1)
test/regression/issue/22567.test.ts
| const gzip = zlib.createGzip(); | ||
| gzip.pipe(res); | ||
| for (let i = 0; i < 50; i++) { | ||
| gzip.write("Line " + i + ": " + "x".repeat(80) + "\\n"); |
There was a problem hiding this comment.
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.
| 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.
Handle connection-level errors on http.get to prevent the test from hanging if the request itself fails before receiving a response. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the fix correctly mirrors the existing this_value strong-reference pattern to also root the input/output buffers during async WorkPool operations.
Extended reasoning...
Overview
This PR fixes a use-after-free crash in zlib/Brotli/Zstd compression streams by holding strong GC references (jsc.Strong.Optional) to input and output JS buffer objects while their backing memory is being used by a WorkPool thread. The fix adds in_buf_value and out_buf_value fields to all three native compression types (NativeZlib, NativeBrotli, NativeZstd) and manages their lifecycle in CompressionStream.write(), runFromJSThread(), closeInternal(), and deinit(). A regression test is also included. As a bonus, the PR fixes a pre-existing bug in NativeZstd where this.this_value.deinit() was missing from deinit().
Security risks
This PR improves memory safety by preventing a use-after-free. There are no new security risks introduced — the change only adds GC rooting for buffers that were previously unrooted during async operations.
Level of scrutiny
The fix is mechanical and follows an established pattern already used for this_value in the same code. The strong references are set in the exact same location where this_value.set() is called, and cleared in the exact same locations where this_value is cleared/deinited. The write_in_progress guard prevents concurrent writes, so there is no risk of overwriting a previous strong reference. The pattern is applied identically across all three compression types.
Other factors
- The coderabbitai comments about
setTimeoutandhttp.geterror handling have been addressed in follow-up commits. - The remaining unaddressed nit about
"x".repeat(80)vsBuffer.allocis cosmetic and does not affect correctness. - The bug report about the explicit test timeout (15_000ms) is a valid convention violation but is a minor nit that does not affect the correctness of the fix itself.
| } | ||
| expect(stdout.trim()).toBe("OK"); | ||
| expect(exitCode).toBe(0); | ||
| }, 15_000); |
There was a problem hiding this comment.
🟡 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.
2 participants
Summary
zlib.createGunzip()(and other compression streams)Root Cause
In
CompressionStream.write(), raw pointers to JS ArrayBuffer backing stores are extracted and passed to a WorkPool thread for async compression/decompression. However, no strong references held the original JS buffer objects alive, so the GC could collect them between scheduling the WorkPool task and its completion — causing a segfault when the native thread accesses freed memory.The fix adds
in_buf_valueandout_buf_valueStrong.Optionalfields that are set before scheduling work and cleared when the work completes.Closes #22567
Test plan
test/regression/issue/22567.test.tsthat pipes HTTP responses throughcreateGunzip()with forced GCdeflate-streaming,zlib-handle-bounds-check,bytesWritten)🤖 Generated with Claude Code