Fix double-free of path string in S3Client methods on error - #30465
Fix double-free of path string in S3Client methods on error#30465robobun wants to merge 1 commit into
Conversation
When an S3 operation (presign, exists, size, stat, write, unlink) given a path string throws after constructing the internal blob store (e.g. missing credentials), ownership of the path has already transferred to the blob. Both defer blob.deinit() and the outer errdefer path.deinit() would then fire, over-releasing the underlying StringImpl and crashing. Neutralize the caller's path variable once the blob takes ownership so the errdefer becomes a no-op on later errors.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR fixes a double-free vulnerability in S3 path handling by neutralizing path ownership after blob construction. Both ChangesS3 Path Double-Free Prevention
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } }; |
There was a problem hiding this comment.
🔴 This fix is incomplete: the same double-free still occurs if constructS3FileWithS3CredentialsAndOptions / constructS3FileInternalStore throws after Blob.Store.initS3 has taken ownership of the path — e.g. when try opts.getTruthyComptime(globalObject, "type") or try file_type.toSlice(...) throws. In that case the inner errdefer store.deinit() frees the path, the error propagates, and the caller's errdefer path.deinit() fires again because this neutralization line is never reached. The neutralization needs to happen at the actual transfer point — e.g. have the construct functions take path by pointer and clear it immediately after initS3 (the same pattern this PR cites in Blob.zig), which also fixes the unmodified S3Client.file().
Extended reasoning...
What the bug is
This PR neutralizes the caller's path / path_or_blob variable on the line after try constructS3FileWithS3CredentialsAndOptions(...) / try constructS3FileInternalStore(...) returns. That closes the window for errors thrown after the construct call (e.g. getPresignUrlFrom throwing on missing credentials), but it leaves open a window inside the construct functions: ownership of path transfers to the store at Blob.Store.initS3(...), an errdefer store.deinit() is registered, and then there are still fallible calls — try opts.getTruthyComptime(globalObject, "type") and try file_type.toSlice(...). If either throws, the path is freed twice exactly as before.
The code path
In constructS3FileWithS3CredentialsAndOptions (and identically in constructS3FileWithS3Credentials, which constructS3FileInternalStore wraps):
var aws_options = try S3.S3Credentials.getCredentialsWithOptions(...); // (1) may throw — BEFORE transfer, safe
const store = ... Blob.Store.initS3(path, ...); // (2) ownership of `path` moves into store
errdefer store.deinit(); // (3)
...
if (try opts.getTruthyComptime(globalObject, "type")) |file_type| { // (4) may throw — AFTER transfer
...
var str = try file_type.toSlice(globalObject, bun.default_allocator); // (5) may throw — AFTER transferBlob.Store.initS3 calls path.toThreadSafe(), which (per SliceWithUnderlyingString.toThreadSafe) transfers the existing reference rather than adding one — the same fact this PR's description relies on. So after (2), the store holds the only reference to the path's StringImpl.
If (4) or (5) throws:
- (3)
errdefer store.deinit()runs → frees the store, which derefs the path → refcount goes to 0,StringImpldestroyed. - The error propagates out of
constructS3FileWith*to the caller. - In the caller (e.g.
S3Client.presign),path = .{ .string = bun.PathString.empty }is the next statement after thetry construct...— it never executes. - The caller's
errdefer path.deinit()(orerrdefer path_or_blob.path.deinit()inS3File.zig) runs → derefs the already-freedStringImpla second time.
That is the same ASSERTION FAILED: wasRemoved / use-after-free crash this PR set out to fix.
Why existing code doesn't prevent it
PathLike.deinit() is a no-op for .string / .buffer but for .slice_with_underlying_string / .threadsafe_string / .encoded_slice it derefs the underlying StringImpl. JS string paths produce one of the latter (otherwise the original bug this PR fixes wouldn't crash), so the second deinit() is not a no-op.
The PR description points to the right pattern — Blob.zig clears path_or_fd.* = .{ .path = .{ .string = bun.PathString.empty } } immediately at the transfer point — but this PR places the clear after the construct function returns instead of at the transfer point inside it.
Step-by-step proof
let n = 0;
const opts = { get type() { if (n++ > 0) throw new Error("boom"); return undefined; } };
new Bun.S3Client({}).presign("some/key.txt", opts);S3Client.presignparsespath(a.slice_with_underlying_stringholding one ref to"some/key.txt"'sStringImpl) and registerserrdefer path.deinit().- It calls
constructS3FileWithS3CredentialsAndOptions(globalThis, path, opts, ...). - Inside,
getCredentialsWithOptionsreadsopts.typeonce (credentials_jsc.zig:203). The getter returnsundefinedon the first read — no throw. Blob.Store.initS3(path, ...)runs: ownership of the path'sStringImplref moves intostore.errdefer store.deinit()is registered.try opts.getTruthyComptime(globalObject, "type")readsopts.typeagain. The getter throws on this second read →error.JSError.errdefer store.deinit()fires → store is freed, which derefs the path →StringImplrefcount hits 0 and is destroyed.error.JSErrorpropagates toS3Client.presign.path = .{ .string = bun.PathString.empty }is never reached.errdefer path.deinit()fires → derefs the destroyedStringImplagain → crash.
(A simple { get type() { throw ... } } would actually throw at step 3, before ownership transfers, which is safe — hence the stateful getter. The window is real regardless; only the repro needs the second-read trick.)
Impact and scope
Memory-safety crash (debug unreachable, release ASSERTION FAILED: wasRemoved in AtomStringImpl.cpp / heap corruption), same as the original bug. The trigger is narrower than the missing-credentials case — it requires options.type to throw on the second read, or file_type.toSlice to throw — but it is reachable from JS and is the exact bug class this PR claims to fix.
It applies to every call site this PR touches (S3Client.{presign,exists,size,stat,write,unlink} and S3File.{presign,exists,size,stat,write,unlink}) and to the unmodified S3Client.file(), where path is still const and never cleared.
How to fix
Move the neutralization to the actual transfer point. The cleanest option is to make constructS3FileWithS3CredentialsAndOptions / constructS3FileWithS3Credentials take path: *jsc.Node.PathLike and do path.* = .{ .string = bun.PathString.empty }; immediately after Blob.Store.initS3(...). Callers then pass &path and can drop the post-call neutralization (and S3Client.file() is fixed for free). Alternatively, neutralize the caller's variable before calling the construct function, since getCredentialsWithOptions doesn't touch path and initS3 is infallible apart from OOM — but the by-pointer approach matches the Blob.zig precedent the PR cites and is robust to future changes.
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| expect(stderr.toString()).not.toContain("panic"); |
There was a problem hiding this comment.
🟡 nit: per the root CLAUDE.md testing guidelines, tests should not assert that stderr does not contain "panic" — these checks never fail in CI. The signalCode, stdout == "ok", and exitCode === 0 assertions below already catch the crash, so this line can be dropped.
Extended reasoning...
The repo's root CLAUDE.md (testing section) states:
NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI.
The new test adds exactly this pattern at line 52:
expect(stderr.toString()).not.toContain("panic");Why it's redundant here. The regression this test guards against is a double-free that manifests as a SIGABRT (debug unreachable / WTF ASSERTION FAILED). The three assertions that follow already detect that failure mode robustly:
expect(signalCode).toBeFalsy()— fails if the child is killed by SIGABRT/SIGSEGV.expect(stdout.toString().trim()).toBe("ok")— fails if the child dies before reachingconsole.log("ok").expect(exitCode).toBe(0)— fails on any non-zero exit.
So the not.toContain("panic") check adds no coverage; if the process panics, one of the above already trips.
Why the guideline exists. In CI, release builds don't necessarily emit the literal string panic on this class of corruption (e.g. the WTF assertion path prints ASSERTION FAILED, and a use-after-free in a fully optimized build may just SIGSEGV with nothing on stderr). A not.toContain("panic") assertion can therefore pass even when the bug is present, giving false confidence — which is precisely why the guideline forbids it.
Step-by-step on current main (without the fix):
- Child process double-frees the path string and aborts.
signalCodeis"SIGABRT"→expect(signalCode).toBeFalsy()fails.stdoutnever receives"ok"→expect(stdout...).toBe("ok")fails.- The stderr check is never the deciding assertion.
Fix: delete line 52. The remaining three assertions are sufficient and match how other crash-regression tests in this repo are written.
(Noting for context: ~20 existing test files already contain this pattern, so this is a widespread legacy violation — but the guideline is unambiguous for new code, and this PR introduces a fresh instance.)
1 participant
What does this PR do?
Fixes a crash (double-free of the path string) in
S3Client/Bun.s3methods when they throw after constructing the internal S3 blob.Reproduction
crashes with:
panic: reached unreachable code(debug), orASSERTION FAILED: wasRemovedinAtomStringImpl.cppRoot cause
The static
Bun.S3Client.{presign,exists,size,stat,write,unlink}and the correspondingS3Clientinstance methods all follow this pattern:constructS3FileWithS3CredentialsAndOptions/constructS3FileInternalStoretransfer ownership ofpathinto the blob's store (viaBlob.Store.initS3, whosepath.toThreadSafe()does not add a reference). If the subsequent operation throws, thedefer blob.detach()frees the store — including the path — and then the outererrdefer path.deinit()frees it again.Fix
After the blob is successfully constructed, overwrite the caller's
path/path_or_blobwith an empty value so theerrdeferbecomes a no-op for subsequent errors. This matches the existing pattern inBlob.zig(e.g.path_or_fd.* = .{ .path = .{ .string = bun.PathString.empty } }after transferring ownership toBlob.Store.initS3).How did you verify your code works?
Added
test/js/bun/s3/s3-path-double-free.test.tswhich exercises all affected static and instance methods on the error path, forces GC, and asserts a clean exit. The test SIGABRTs on current main and passes with this fix.