Skip to content

Fix double-free of path string in S3Client methods on error #30465

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 double-free of path string in S3Client methods on error #30465

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
18 changes: 12 additions & 6 deletions src/runtime/webcore/S3Client.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ pub const S3Client = struct {
const arguments = callframe.arguments_old(2).slice();
var args = jsc.CallFrame.ArgumentsSlice.init(globalThis.bunVM(), arguments);
defer args.deinit();
const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
var path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
if (args.len() == 0) {
return globalThis.ERR(.MISSING_ARGS, "Expected a path to presign", .{}).throw();
}
Expand All @@ -155,6 +155,7 @@ pub const S3Client = struct {

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 = .{ .string = bun.PathString.empty };
defer blob.detach();
return S3File.getPresignUrlFrom(&blob, globalThis, options);
}
Expand All @@ -163,7 +164,7 @@ pub const S3Client = struct {
const arguments = callframe.arguments_old(2).slice();
var args = jsc.CallFrame.ArgumentsSlice.init(globalThis.bunVM(), arguments);
defer args.deinit();
const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
var path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
if (args.len() == 0) {
return globalThis.ERR(.MISSING_ARGS, "Expected a path to check if it exists", .{}).throw();
}
Expand All @@ -172,6 +173,7 @@ pub const S3Client = struct {
errdefer 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 = .{ .string = bun.PathString.empty };
defer blob.detach();
return S3File.S3BlobStatTask.exists(globalThis, &blob);
}
Expand All @@ -180,7 +182,7 @@ pub const S3Client = struct {
const arguments = callframe.arguments_old(2).slice();
var args = jsc.CallFrame.ArgumentsSlice.init(globalThis.bunVM(), arguments);
defer args.deinit();
const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
var path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
if (args.len() == 0) {
return globalThis.ERR(.MISSING_ARGS, "Expected a path to check the size of", .{}).throw();
}
Expand All @@ -189,6 +191,7 @@ pub const S3Client = struct {
errdefer 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 = .{ .string = bun.PathString.empty };
defer blob.detach();
return S3File.S3BlobStatTask.size(globalThis, &blob);
}
Expand All @@ -197,7 +200,7 @@ pub const S3Client = struct {
const arguments = callframe.arguments_old(2).slice();
var args = jsc.CallFrame.ArgumentsSlice.init(globalThis.bunVM(), arguments);
defer args.deinit();
const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
var path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
if (args.len() == 0) {
return globalThis.ERR(.MISSING_ARGS, "Expected a path to check the stat of", .{}).throw();
}
Expand All @@ -206,6 +209,7 @@ pub const S3Client = struct {
errdefer 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 = .{ .string = bun.PathString.empty };
defer blob.detach();
return S3File.S3BlobStatTask.stat(globalThis, &blob);
}
Expand All @@ -214,7 +218,7 @@ pub const S3Client = struct {
const arguments = callframe.arguments_old(3).slice();
var args = jsc.CallFrame.ArgumentsSlice.init(globalThis.bunVM(), arguments);
defer args.deinit();
const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
var path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
return globalThis.ERR(.MISSING_ARGS, "Expected a path to write to", .{}).throw();
};
errdefer path.deinit();
Expand All @@ -224,6 +228,7 @@ pub const S3Client = struct {

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 = .{ .string = bun.PathString.empty };
defer blob.detach();
var blob_internal: PathOrBlob = .{ .blob = blob };
return Blob.writeFileInternal(globalThis, &blob_internal, data, .{
Expand All @@ -248,12 +253,13 @@ pub const S3Client = struct {
const arguments = callframe.arguments_old(2).slice();
var args = jsc.CallFrame.ArgumentsSlice.init(globalThis.bunVM(), arguments);
defer args.deinit();
const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
var path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
return globalThis.ERR(.MISSING_ARGS, "Expected a path to unlink", .{}).throw();
};
errdefer 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 = .{ .string = bun.PathString.empty };
defer blob.detach();
return blob.store.?.data.s3.unlink(blob.store.?, globalThis, options);
}
Expand Down
6 changes: 6 additions & 0 deletions src/runtime/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/runtime/webcore/S3File.zig

View check run for this annotation

Claude / Claude Code Review

Double-free still reachable when construct* throws after initS3 

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

@claude claude Bot May 10, 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-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 transfer

Blob.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, StringImpl destroyed.
  • 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 the try construct... — it never executes.
  • The caller's errdefer path.deinit() (or errdefer path_or_blob.path.deinit() in S3File.zig) runs → derefs the already-freed StringImpl a 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);
  1. S3Client.presign parses path (a .slice_with_underlying_string holding one ref to "some/key.txt"'s StringImpl) and registers errdefer path.deinit().
  2. It calls constructS3FileWithS3CredentialsAndOptions(globalThis, path, opts, ...).
  3. Inside, getCredentialsWithOptions reads opts.type once (credentials_jsc.zig:203). The getter returns undefined on the first read — no throw.
  4. Blob.Store.initS3(path, ...) runs: ownership of the path's StringImpl ref moves into store.
  5. errdefer store.deinit() is registered.
  6. try opts.getTruthyComptime(globalObject, "type") reads opts.type again. The getter throws on this second read → error.JSError.
  7. errdefer store.deinit() fires → store is freed, which derefs the path → StringImpl refcount hits 0 and is destroyed.
  8. error.JSError propagates to S3Client.presign. path = .{ .string = bun.PathString.empty } is never reached.
  9. errdefer path.deinit() fires → derefs the destroyed StringImpl again → 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.

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
56 changes: 56 additions & 0 deletions test/js/bun/s3/s3-path-double-free.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";

// When an S3 operation given a path string throws after constructing the
// internal blob (e.g. missing credentials), the path string must not be
// dereferenced twice. Previously both `defer blob.deinit()` and the outer
// `errdefer path.deinit()` fired, over-releasing the underlying StringImpl.
test("S3Client methods do not double-free the path string when they throw", () => {
const { exitCode, stdout, stderr, signalCode } = Bun.spawnSync({
cmd: [
bunExe(),
"-e",
`
process.on("unhandledRejection", () => {});
const methods = ["presign", "exists", "size", "stat", "unlink", "delete"];

for (const m of methods) {
for (let i = 0; i < 3; i++) {
try { Bun.S3Client[m]("some/key/here.txt"); } catch {}
try { Bun.S3Client[m]("some/key/here.txt", "not an object"); } catch {}
}
}
for (let i = 0; i < 3; i++) {
try { Bun.S3Client.write("some/key/here.txt", "data", "not an object")?.catch?.(() => {}); } catch {}
}

const client = new Bun.S3Client({});
for (const m of methods) {
for (let i = 0; i < 3; i++) {
try { client[m]("some/key/here.txt"); } catch {}
try { client[m]("some/key/here.txt", "not an object"); } catch {}
}
}
for (let i = 0; i < 3; i++) {
try { client.write("some/key/here.txt", "data", "not an object")?.catch?.(() => {}); } catch {}
}

Bun.gc(true);
console.log("ok");
`,
],
env: {
...bunEnv,
AWS_ACCESS_KEY_ID: "",
AWS_SECRET_ACCESS_KEY: "",
S3_ACCESS_KEY_ID: "",
S3_SECRET_ACCESS_KEY: "",
},
stdout: "pipe",
stderr: "pipe",
});
expect(stderr.toString()).not.toContain("panic");

Check warning on line 52 in test/js/bun/s3/s3-path-double-free.test.ts

View check run for this annotation

Claude / Claude Code Review

Test checks stderr for "panic" — forbidden by repo guidelines 

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.

@claude claude Bot May 10, 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.

🟡 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:

  1. expect(signalCode).toBeFalsy() — fails if the child is killed by SIGABRT/SIGSEGV.
  2. expect(stdout.toString().trim()).toBe("ok") — fails if the child dies before reaching console.log("ok").
  3. 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):

  1. Child process double-frees the path string and aborts.
  2. signalCode is "SIGABRT" → expect(signalCode).toBeFalsy() fails.
  3. stdout never receives "ok" → expect(stdout...).toBe("ok") fails.
  4. 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.)

expect(signalCode).toBeFalsy();
expect(stdout.toString().trim()).toBe("ok");
expect(exitCode).toBe(0);
});
Loading