Skip to content

s3: fix path double-free when presign throws after blob creation #30351

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 4 commits into from
+131 −35
Closed

s3: fix path double-free when presign throws after blob creation #30351

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97cc97e
s3: fix path double-free when presign throws after blob creation
robobun May 7, 2026
8fbfed4
s3: neutralize path inside constructor once initS3 takes ownership
robobun May 7, 2026
861406a
test: run S3 path double-free cases in subprocesses
robobun May 7, 2026
a5092c4
test: drop per-test timeouts and redundant panic assertion
robobun May 7, 2026
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 = .{ .path = .{ .string = bun.PathString.empty } } };

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

View check run for this annotation

Claude / Claude Code Review

Double-free still possible when constructS3File* throws after initS3 

The fix is incomplete: the same double-free still occurs if `constructS3FileWithS3Credentials*` itself throws *after* `initS3` has taken ownership of the path — its internal `errdefer store.deinit()` derefs the path once, and since the new neutralization line sits *after* the `try`, the caller's `errdefer path.deinit()` derefs it a second time. The post-`initS3` `try opts.getTruthyComptime(globalObject, "type")` is reachable from JS via a stateful `type` getter (it must succeed once for `getCred
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
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 = .{ .path = .{ .string = bun.PathString.empty } } };
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 = .{ .path = .{ .string = bun.PathString.empty } } };
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 = .{ .path = .{ .string = bun.PathString.empty } } };
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 = .{ .path = .{ .string = bun.PathString.empty } } };
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 = .{ .path = .{ .string = bun.PathString.empty } } };
defer blob.deinit();

return S3BlobStatTask.stat(globalThis, &blob);
Expand Down
25 changes: 25 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,25 @@
import { describe, expect, test } from "bun:test";

// When an S3 operation throws after the temporary blob store has taken
// ownership of the path, the caller's errdefer must not deref the path a
// second time. Previously this asserted in debug builds (string refcount
// underflow) when presign threw synchronously.
describe("S3 path ownership on error", () => {
// use an options value that makes getPresignUrlFrom throw synchronously
// regardless of any ambient AWS credentials in the environment.
const badOptions = { expiresIn: -1, accessKeyId: "x", secretAccessKey: "y" };

test("S3Client.presign (static)", () => {
// use a fresh non-interned string so the refcount starts at 1
const path = ["some", "path", Math.random()].join("-");
expect(() => Bun.S3Client.presign(path, badOptions)).toThrow();
Bun.gc(true);
});

test("S3Client#presign (instance)", () => {
const client = new Bun.S3Client({});
const path = ["some", "path", Math.random()].join("-");
expect(() => client.presign(path, badOptions)).toThrow();
Bun.gc(true);
});
});
Loading