Fix double-free of path string on S3 error paths#30437
Conversation
When an S3Client/S3File method creates a blob via
constructS3FileWithS3CredentialsAndOptions (or the internal store
variant), Blob.Store.initS3* calls PathLike.toThreadSafe() on a copy of
the caller's path. For WTF-backed strings this transfers the caller's
StringImpl refs to a new isolated copy, leaving the caller's PathLike
pointing at an impl whose refs it no longer owns.
If a subsequent step fails (e.g. signRequest -> MissingCredentials in
getPresignUrlFrom), the caller's `errdefer path.deinit()` fires and
derefs the original impl again, tripping the
`hasAtLeastOneRef()` assertion in debug builds and corrupting memory in
release builds. Only non-static StringImpls hit the assert, which is
why `Bun.s3.presign("abc")` crashed while `Bun.s3.presign("path")`
(a static common identifier) did not.
Guard the errdefer with a `path_consumed` flag that is set once the
blob store has taken ownership, so the path is only deinit'd on
failures that happen before the store is created.
|
Updated 1:47 PM PT - May 9th, 2026
❌ @robobun, your commit c8fa802 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30437That installs a local version of the PR into your bun-30437 --bun |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR fixes potential double-free issues in S3 path cleanup by introducing a ChangesS3 Path Ownership Cleanup
🚥 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 path_consumed = false; | ||
| errdefer if (!path_consumed) path.deinit(); | ||
|
|
||
| const options = args.nextEat(); | ||
| var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); | ||
| path_consumed = true; |
There was a problem hiding this comment.
🔴 The path_consumed flag is set after constructS3FileWithS3CredentialsAndOptions returns, but ownership of path actually transfers inside that function at the Blob.Store.initS3* call — and there is still fallible code after it (try opts.getTruthyComptime(globalObject, "type") / try file_type.toSlice(...)). If that post-initS3 read throws, errdefer store.deinit() frees the store, the error propagates with path_consumed still false, and the caller's errdefer derefs the already-released original WTFStringImpl — the same double-free this PR is fixing. Consider passing path by pointer and clearing it right after initS3, or hoisting the "type" handling before store creation; this applies to every call site touched in S3Client.zig and S3File.zig.
Extended reasoning...
What the bug is
This PR's fix moves the ownership boundary to "after constructS3FileWithS3CredentialsAndOptions / constructS3FileInternalStore returns successfully". But the real ownership-transfer point is inside those functions, at the Blob.Store.initS3 / initS3WithReferencedCredentials call. Between that transfer point and the function's successful return, there is still fallible JS-visible code. If that code throws, we end up in exactly the state the PR set out to fix: the store has consumed the caller's path refs, yet the caller's errdefer if (!path_consumed) path.deinit() still fires.
The specific code path
In constructS3FileWithS3CredentialsAndOptions (and the near-identical constructS3FileWithS3Credentials):
Blob.Store.initS3*is called (Store.zig:68-71 / 97-100). It takespathlikeby value, copies it locally, and callspath.toThreadSafe(). For.slice_with_underlying_stringthis reachesBunString__toThreadSafe(BunString.cpp:211-224), which installs an isolated copy and callsexisting->deref()on the originalWTFStringImpl. The caller'spathvariable now points at an impl whose refs have been released — this is the PR's own stated premise.errdefer store.deinit()is registered.- After that,
try opts.getTruthyComptime(globalObject, "type")andtry file_type.toSlice(...)run. These perform a JS property access on a user-supplied options object and can throw.
If step 3 throws: errdefer store.deinit() frees the store (and its thread-safe copy of the path), the error propagates back to e.g. S3Client.presign, and since the construct function never returned, path_consumed is still false. The caller's errdefer if (!path_consumed) path.deinit() then derefs the already-released original impl → hasAtLeastOneRef() assertion in debug / double-free in release.
Why existing code doesn't prevent it
The PR's flag is set on the line after the try, so it cannot observe partial progress inside the callee. The callee's own errdefer store.deinit() correctly cleans up the store but does nothing to tell the caller that the original path refs were already consumed. Note that getCredentialsWithOptions (credentials_jsc.zig:203) also reads "type" once before initS3, so a plain throwing getter would fail early (which is handled correctly); a stateful getter or Proxy that succeeds the first time and throws the second is what reaches the post-initS3 read.
Step-by-step proof
let n = 0;
new Bun.S3Client({}).presign("abc", {
accessKeyId: "a", secretAccessKey: "b", bucket: "c", endpoint: "http://localhost",
get type() { if (n++ > 0) throw new Error("boom"); return "text/plain"; }
});S3Client.presignparsespath = .slice_with_underlying_stringfor"abc", setspath_consumed = false, registerserrdefer if (!path_consumed) path.deinit().- Calls
constructS3FileWithS3CredentialsAndOptions.getCredentialsWithOptionsreadstype(n=0 → returns"text/plain", n becomes 1). Blob.Store.initS3WithReferencedCredentials(path, …)runs →toThreadSafe()derefs the original impl behind the caller'spath.opts.getTruthyComptime(globalObject, "type")readstypeagain (n=1 → throws).error.JSErroris returned.errdefer store.deinit()frees the store; error propagates topresign.path_consumedis stillfalse→path.deinit()runs → derefs the already-released impl → crash.
Impact and scope
This is the same crash class (Fuzzilli fingerprint 1ebbda306723eeb6) the PR targets, just on a window the patch doesn't cover. Fuzzilli routinely generates exactly these adversarial getters/Proxies, so this remaining window is likely to be rediscovered. It applies to every call site this PR modified: S3Client.{file,presign,exists,size,stat,write,unlink} and S3File.{presign,unlink,write,size,exists,stat}.
How to fix
Move the ownership signal to the actual transfer point. Two straightforward options:
- Have
constructS3FileWithS3CredentialsAndOptions/constructS3FileWithS3Credentialstakepath: *jsc.Node.PathLikeand setpath.* = .{ .string = bun.PathString.empty }(or equivalent) immediately afterinitS3*returns. Callers can then unconditionallyerrdefer path.deinit()since deinit on an empty path is a no-op. - Or hoist the
"type"handling to beforeinitS3*, so there is no fallible code between ownership transfer and return.
1 participant
What
Bun.s3.presign("abc")(and the staticBun.S3Client.presign) crashed withreached unreachable codein debug builds when credentials are missing:Why
Blob.Store.initS3*receives thePathLikeby value and callspath.toThreadSafe()on its local copy. For a.slice_with_underlying_stringpath this replaces the underlyingWTFStringImpl*with an isolated copy and releases the refs the caller was holding on the original impl — ownership is transferred to the store.If a later step fails (e.g.
signRequest→MissingCredentialsingetPresignUrlFrom), the caller'serrdefer path.deinit()fires and derefs the original impl again — a double-free. Static strings ("path","name","type", …) masked this because their refcount never drops, so only "normal" path strings tripped the assertion.How
Add a
path_consumedflag set immediately after the blob store is created. Theerrdefernow only deinits the path when the failure happened before ownership was transferred.Applied to all affected methods on
S3Client(instance + static) and theS3Filestatic entrypoints (presign,unlink,write,exists,size,stat).Test
test/js/bun/s3/s3-error-path-cleanup.test.ts— panics on the unpatched build, passes after. Also covers the early-failure path (invalid option type before store creation) to ensure the path is still freed there, and a GC stress loop.Found by Fuzzilli (fingerprint
1ebbda306723eeb6).