fix(s3): double deref of path string on error in S3Client static methods - #29643
fix(s3): double deref of path string on error in S3Client static methods#29643robobun wants to merge 1 commit into
Conversation
When S3Client.presign/unlink/write/size/exists/stat are called with a path and the underlying operation throws (e.g. missing credentials), the path was freed twice: once by blob.deinit() via the blob store, and again by the errdefer on path_or_blob. Clear path_or_blob after ownership is transferred to the blob so the errdefer is a no-op.
|
Updated 11:23 AM PT - Apr 23rd, 2026
❌ @robobun, your commit a721934 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29643That installs a local version of the PR into your bun-29643 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR modifies S3 file operations to properly handle path cleanup in error scenarios by mutating the path_or_blob value to an invalid file descriptor, and adds a test case to validate S3Client.presign error handling when credentials are missing. Changes
🚥 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 |
|
Duplicate of #29081 (same fix, same approach). |
![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-deref still occurs if constructS3FileWithS3Credentials throws after Blob.Store.initS3 has stored the path (e.g. when the post-initS3 try opts.getTruthyComptime(globalObject, "type") throws). In that case errdefer store.deinit() derefs the path, the error propagates before the new neutralization line runs, and the caller's errdefer path_or_blob.path.deinit() derefs it again. Consider neutralizing path inside constructS3FileWithS3Credentials immediately after initS3 succeeds (or having initS3 ref/dupe the path) so ownership transfer is atomic with respect to errors.
Extended reasoning...
What the bug is
The PR neutralizes path_or_blob after constructS3FileInternalStore returns successfully. But constructS3FileInternalStore → constructS3FileWithS3Credentials can throw after it has already transferred ownership of the path into the blob store via Blob.Store.initS3. When that happens, the inner errdefer store.deinit() derefs the path, then the caller's outer errdefer path_or_blob.path.deinit() derefs it a second time — the exact hasAtLeastOneRef() crash this PR is meant to fix.
Code path
In constructS3FileWithS3Credentials (S3File.zig):
const store = bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, ...));
errdefer store.deinit(); // derefs path on error
...
if (try opts.getTruthyComptime(globalObject, "type")) |file_type| { // can throw
...
var str = try file_type.toSlice(globalObject, ...); // can throwinitS3 stores the PathLike in the S3 store without adding a net ref (PathLike.toThreadSafe on a slice_with_underlying_string is a no-op when the impl is already thread-safe — this is why the original bug crashed at all). After that point, both the store and the caller's path_or_blob think they own the same +1.
Why the fix doesn't cover it
The neutralization line is placed here:
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } }; // ← only runs on SUCCESSIf constructS3FileInternalStore throws, control never reaches the neutralization, so the outer errdefer still sees the live path and derefs it again.
Step-by-step proof
Repro (note: type is read once in getCredentialsWithOptions before initS3 and once after, so the getter must succeed on the first read and throw on the second — a plain get type(){ throw 1 } would throw too early):
let n = 0;
Bun.S3Client.presign("some/bucket/key", {
accessKeyId: "x", secretAccessKey: "x",
get type() { if (n++) throw new Error("boom"); return "text/plain"; },
});PathOrBlob.fromJSNoCopyproduces a.pathwith refcount 1;errdefer path_or_blob.path.deinit()is armed.constructS3FileInternalStore→constructS3FileWithS3CredentialscallsgetCredentialsWithOptions, which readstype(1st call → returns"text/plain").Blob.Store.initS3(path, ...)stores the path in the S3 store (no net ref added).errdefer store.deinit()is armed.try opts.getTruthyComptime(globalObject, "type")readstypeagain (2nd call → throws).errdefer store.deinit()runs →Store.deinit→S3.deinit→pathlike.deinit()→ first deref (refcount 1 → 0, string freed).- Error propagates out of
constructS3FileInternalStore; the newpath_or_blob = .{ .path = .{ .fd = ... } }line is never reached. - Caller's
errdefer path_or_blob.path.deinit()runs → second deref →hasAtLeastOneRef()assertion crash.
The same applies to all six call sites (presign, unlink, write, size, exists, stat) and also to constructS3FileWithS3CredentialsAndOptions.
Impact
Same crash class as the original Fuzzilli find (reached unreachable code in WTFStringImplStruct.deref). The trigger is more contrived than the original (requires a stateful getter/Proxy that throws on the second access rather than a simple missing-credentials error), but it's squarely in the territory a fuzzer will hit, and it's the exact bug class this PR's title claims to fix.
Suggested fix
Move the ownership transfer to where it actually happens: inside constructS3FileWithS3Credentials (and constructS3FileWithS3CredentialsAndOptions), immediately after initS3 succeeds, either neutralize the by-value path parameter so the inner errdefer is the sole owner, or change the calling convention so callers pass *PathLike and the callee zeroes it on transfer. Alternatively, have initS3 dupe/ref the path so the caller retains independent ownership.
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } }; |
There was a problem hiding this comment.
🟣 The same double-deref pattern exists in the instance methods in src/bun.js/webcore/S3Client.zig (presign, exists, size, stat, write, unlink, lines ~144-259): errdefer path.deinit() followed by constructS3FileWithS3CredentialsAndOptions (which transfers path ownership into the store) followed by defer blob.detach(). This is pre-existing — the PR doesn't touch that file — but e.g. new Bun.S3Client({accessKeyId:'x',secretAccessKey:'y',bucket:'b'}).presign('key', {expiresIn: -1}) will hit the same hasAtLeastOneRef() crash, so it may be worth applying the same neutralization there (or in a follow-up) so the fuzzer fingerprint doesn't immediately resurface via the instance API.
Extended reasoning...
What the bug is
This PR correctly fixes the double-deref of the path string in the static S3Client methods in src/bun.js/webcore/S3File.zig. However, the instance methods on S3Client in src/bun.js/webcore/S3Client.zig have a structurally identical pattern that is not touched by this PR and will produce the exact same hasAtLeastOneRef() crash.
For example, S3Client.zig:144-159 (presign):
const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse { ... };
errdefer path.deinit();
const options = args.nextEat();
var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ...);
defer blob.detach();
return S3File.getPresignUrlFrom(&blob, globalThis, options);The same shape appears in exists (172-176), size (189-193), stat (206-210), write (220-232), and unlink (254-258).
Code path / why nothing prevents it
constructS3FileWithS3CredentialsAndOptions calls Blob.Store.initS3(path, ...) (or initS3WithReferencedCredentials), which stores the PathLike in the blob's S3 store without adding a net ref — ownership transfers, exactly as described in this PR's root-cause analysis. blob.detach() calls store.deref(), which (since the store starts at refcount 1) runs Store.deinit() → s3.deinit() → pathlike.deinit(). That is the first deref. Because the trailing call returned an error, the errdefer path.deinit() then runs as well — second deref → reached unreachable code in WTFStringImplStruct.deref.
This is the same mechanism the PR fixes for the static methods; the instance methods just live in a different file and use blob.detach() instead of blob.deinit(), but Blob.deinit() simply calls detach(), so the cleanup path is identical.
Step-by-step proof
const client = new Bun.S3Client({ accessKeyId: 'x', secretAccessKey: 'y', bucket: 'b' })— constructs anS3Clientwith valid-looking credentials.client.presign('key', { expiresIn: -1 })entersS3Client.presign.PathLike.fromJSsucceeds →pathholds one ref to the"key"string.errdefer path.deinit()is armed.constructS3FileWithS3CredentialsAndOptionssucceeds (it does not readexpiresIn;getCredentialsWithOptionsonly reads credential/ACL/part-size fields). The returned blob's store now ownspath.defer blob.detach()is armed.getPresignUrlFromreadsexpiresIn, sees-1 <= 0, and throwsthrowInvalidArguments("expiresIn must be greather than 0").- Defers run LIFO:
blob.detach()→store.deref()→ store refcount 1→0 →s3.deinit()→pathlike.deinit()(first deref of"key"). errdefer path.deinit()runs → second deref of the same string →hasAtLeastOneRef()assertion fails → crash.
Other easy triggers from the same family: { method: 'OPTIONS' } (invalid method), an oversized sessionToken (sign error ERR_S3_INVALID_SESSION_TOKEN), or for exists/size/stat/unlink any condition that makes S3.stat/unlink throw synchronously (e.g. sign error).
Impact
Same impact as the bug this PR fixes: a user-triggerable hard crash (reached unreachable code) instead of a catchable JS error, reachable from the public new Bun.S3Client(...).presign/exists/size/stat/write/unlink API. Since this PR is fixing a Fuzzilli fingerprint for this exact crash class, the fuzzer will likely rediscover it via the instance-method entry point.
How to fix
Apply the same neutralization used in this PR: after constructS3FileWithS3CredentialsAndOptions succeeds, clear the outer owner so the errdefer becomes a no-op. Since these methods hold a bare PathLike (not a PathOrBlob), the simplest equivalent is to make path a var and set path = .{ .fd = bun.invalid_fd }; immediately after the blob is constructed (or alternatively restructure to drop the errdefer once ownership transfers).
This is pre-existing — S3Client.zig is not modified by this PR — so it's fine to land as a follow-up, but flagging here since it's the same crash class the PR title/description claims to address.
1 participant
What does this PR do?
Fixes a crash (
reached unreachable codefrom thehasAtLeastOneRef()assertion inWTFStringImplStruct.deref) when a staticBun.S3Clientmethod that accepts a path —presign,unlink,write,size,exists,stat— throws after successfully constructing the internal S3 blob.For example, calling
Bun.S3Client.presign("some/bucket/key")with no credentials configured would crash instead of throwingERR_S3_MISSING_CREDENTIALS.Root cause
constructS3FileInternalStorestores the path in the blob'sS3store (transferring ownership). If the subsequent operation throws,defer blob.deinit()derefs the path via the store, then the outererrdeferderefs it again.Fix
After the blob is created, overwrite
path_or_blobwith a harmless.fdvalue so theerrdefer'sdeinit()becomes a no-op once ownership has transferred.How did you verify your code works?
test/js/bun/s3/s3.test.tsthat crashes onmainand passes with this fix.S3Client.presignwith valid credentials) still produces a URL.Found by Fuzzilli. Fingerprint:
a7f2578a4715b60c