Skip to content

fix(s3): double deref of path string on error in S3Client static methods #29643

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 1 commit into from
Closed

fix(s3): double deref of path string on error in S3Client static methods #29643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions src/bun.js/webcore/S3File.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@
return globalThis.throwInvalidArguments("Expected a S3 or path to presign", .{});
}
const options = args.nextEat();
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } };

Check failure on line 87 in src/bun.js/webcore/S3File.zig

View check run for this annotation

Claude / Claude Code Review

Double-deref still possible if constructS3FileInternalStore throws after storing path 

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

Check notice on line 87 in src/bun.js/webcore/S3File.zig

View check run for this annotation

Claude / Claude Code Review

Same double-deref crash exists in S3Client instance methods 

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:
Comment on lines 86 to +87

@claude claude Bot Apr 23, 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.

🔴 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 throw

initS3 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 SUCCESS

If 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"; },
});
  1. PathOrBlob.fromJSNoCopy produces a .path with refcount 1; errdefer path_or_blob.path.deinit() is armed.
  2. constructS3FileInternalStore → constructS3FileWithS3Credentials calls getCredentialsWithOptions, which reads type (1st call → returns "text/plain").
  3. Blob.Store.initS3(path, ...) stores the path in the S3 store (no net ref added). errdefer store.deinit() is armed.
  4. try opts.getTruthyComptime(globalObject, "type") reads type again (2nd call → throws).
  5. errdefer store.deinit() runs → Store.deinit → S3.deinit → pathlike.deinit() → first deref (refcount 1 → 0, string freed).
  6. Error propagates out of constructS3FileInternalStore; the new path_or_blob = .{ .path = .{ .fd = ... } } line is never reached.
  7. 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.

Comment on lines 86 to +87

@claude claude Bot Apr 23, 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 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

  1. const client = new Bun.S3Client({ accessKeyId: 'x', secretAccessKey: 'y', bucket: 'b' }) — constructs an S3Client with valid-looking credentials.
  2. client.presign('key', { expiresIn: -1 }) enters S3Client.presign.
  3. PathLike.fromJS succeeds → path holds one ref to the "key" string. errdefer path.deinit() is armed.
  4. constructS3FileWithS3CredentialsAndOptions succeeds (it does not read expiresIn; getCredentialsWithOptions only reads credential/ACL/part-size fields). The returned blob's store now owns path. defer blob.detach() is armed.
  5. getPresignUrlFrom reads expiresIn, sees -1 <= 0, and throws throwInvalidArguments("expiresIn must be greather than 0").
  6. Defers run LIFO: blob.detach() → store.deref() → store refcount 1→0 → s3.deinit() → pathlike.deinit() (first deref of "key").
  7. 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.

defer blob.deinit();
return try getPresignUrlFrom(&blob, globalThis, options);
},
Expand Down Expand Up @@ -114,6 +115,7 @@
}
const options = args.nextEat();
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } };
defer blob.deinit();
return try blob.store.?.data.s3.unlink(blob.store.?, globalThis, options);
},
Expand Down Expand Up @@ -151,6 +153,7 @@
return globalThis.throwInvalidArguments("Expected a S3 or path to upload", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } };
defer blob.deinit();

var blob_internal: PathOrBlob = .{ .blob = blob };
Expand Down Expand Up @@ -190,6 +193,7 @@
return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } };
defer blob.deinit();

return S3BlobStatTask.size(globalThis, &blob);
Expand Down Expand Up @@ -223,6 +227,7 @@
return globalThis.throwInvalidArguments("Expected a S3 or path to check if it exists", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } };
defer blob.deinit();

return S3BlobStatTask.exists(globalThis, &blob);
Expand Down Expand Up @@ -574,6 +579,7 @@
return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } };
defer blob.deinit();

return S3BlobStatTask.stat(globalThis, &blob);
Expand Down
4 changes: 4 additions & 0 deletions test/js/bun/s3/s3.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1564,6 +1564,10 @@ describe.concurrent("s3 missing credentials", () => {
await Bun.s3.presign("test");
});
});
it("S3Client.presign static", () => {
expect(() => S3Client.presign("some/bucket/key")).toThrow("Missing S3 credentials");
expect(() => S3Client.presign("some/bucket/key", {})).toThrow("Missing S3 credentials");
});
it("file", async () => {
assertMissingCredentials(async () => {
await Bun.s3.file("test").text();
Expand Down
Loading