Skip to content

s3: don't double-free path when presign throws after blob creation #30467

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 2 commits into from
+117 −47
Closed

s3: don't double-free path when presign throws after blob creation #30467

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffe2a78
s3: don't double-free path when presign throws after blob creation
robobun May 10, 2026
6319bc1
Blob: use path.deinit() in s3:// errdefer to avoid double-unprotect
robobun May 10, 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
3 changes: 2 additions & 1 deletion src/runtime/webcore/Blob.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2069,7 +2069,8 @@

if (path == .path) {
if (strings.hasPrefixComptime(path.path.slice(), "s3://")) {
return try S3File.constructInternalJS(globalObject, path.path, options);
errdefer path.deinitAndUnprotect();
return try S3File.constructInternalJS(globalObject, &path.path, options);

Check warning on line 2073 in src/runtime/webcore/Blob.zig

View check run for this annotation

Claude / Claude Code Review

errdefer deinitAndUnprotect double-unprotects Buffer-backed s3:// paths 

nit: This errdefer uses `path.deinitAndUnprotect()` while every other new errdefer in this PR (`S3Client.staticFile`, `S3File.constructInternal`, the `S3Client` instance methods) uses `path.deinit()`. For a `.buffer`-backed path (e.g. `Bun.file(Buffer.from("s3://b/k"), {accessKeyId: 123})`) where `getCredentialsWithOptions` throws before the neuter, this unprotects the buffer once here and again in `defer args.deinit()` from the `protectEat` bitset (+1/-2). JSC's `HashCountedSet::remove` tolerat
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
}
}
defer path.deinitAndUnprotect();
Expand Down
39 changes: 21 additions & 18 deletions src/runtime/webcore/S3Client.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,23 +129,23 @@ 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 ", .{}).throw();
}
return globalThis.throwInvalidArguments("Expected a path", .{});
};
errdefer path.deinit();
const options = args.nextEat();
var blob = Blob.new(try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer));
var blob = Blob.new(try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, &path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer));
return blob.toJS(globalThis);
}

pub fn presign(ptr: *@This(), globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSError!JSValue {
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 @@ -154,7 +154,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);
var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, &path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
defer blob.detach();
return S3File.getPresignUrlFrom(&blob, globalThis, options);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
Expand All @@ -163,15 +163,15 @@ 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();
}
return globalThis.throwInvalidArguments("Expected a path to check if it exists", .{});
};
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);
var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, &path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
defer blob.detach();
return S3File.S3BlobStatTask.exists(globalThis, &blob);
}
Expand All @@ -180,15 +180,15 @@ 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();
}
return globalThis.throwInvalidArguments("Expected a path to check the size of", .{});
};
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);
var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, &path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
defer blob.detach();
return S3File.S3BlobStatTask.size(globalThis, &blob);
}
Expand All @@ -197,15 +197,15 @@ 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();
}
return globalThis.throwInvalidArguments("Expected a path to check the stat of", .{});
};
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);
var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, &path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
defer blob.detach();
return S3File.S3BlobStatTask.stat(globalThis, &blob);
}
Expand All @@ -214,7 +214,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 @@ -223,7 +223,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);
var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, &path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
defer blob.detach();
var blob_internal: PathOrBlob = .{ .blob = blob };
return Blob.writeFileInternal(globalThis, &blob_internal, data, .{
Expand All @@ -238,7 +238,8 @@ pub const S3Client = struct {
const object_keys = args[0];
const options = args[1];

var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, .{ .string = bun.PathString.empty }, options, ptr.credentials, ptr.options, null, null, ptr.request_payer);
var empty_path: jsc.Node.PathLike = .{ .string = bun.PathString.empty };
var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, &empty_path, options, ptr.credentials, ptr.options, null, null, ptr.request_payer);

defer blob.detach();
return blob.store.?.data.s3.listObjects(blob.store.?, globalThis, object_keys, options);
Expand All @@ -248,12 +249,12 @@ 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);
var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, &path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
defer blob.detach();
return blob.store.?.data.s3.unlink(blob.store.?, globalThis, options);
}
Expand Down Expand Up @@ -296,11 +297,12 @@ pub const S3Client = struct {
var args = jsc.CallFrame.ArgumentsSlice.init(globalThis.bunVM(), arguments);
defer args.deinit();

const path = (try jsc.Node.PathLike.fromJS(globalThis, &args)) orelse {
var path = (try jsc.Node.PathLike.fromJS(globalThis, &args)) orelse {
return globalThis.throwInvalidArguments("Expected file path string", .{});
};
errdefer path.deinit();

return try S3File.constructInternalJS(globalThis, path, args.nextEat());
return try S3File.constructInternalJS(globalThis, &path, args.nextEat());
}
pub fn staticStat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSError!JSValue {
return S3File.stat(globalThis, callframe);
Expand All @@ -314,7 +316,8 @@ pub const S3Client = struct {
// get credentials from env
const existing_credentials = globalThis.bunVM().transpiler.env.getS3Credentials();

var blob = try S3File.constructS3FileWithS3Credentials(globalThis, .{ .string = bun.PathString.empty }, options, existing_credentials);
var empty_path: jsc.Node.PathLike = .{ .string = bun.PathString.empty };
var blob = try S3File.constructS3FileWithS3Credentials(globalThis, &empty_path, options, existing_credentials);

defer blob.detach();
return blob.store.?.data.s3.listObjects(blob.store.?, globalThis, object_keys, options);
Expand Down
65 changes: 37 additions & 28 deletions src/runtime/webcore/S3File.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,12 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J
}

switch (path_or_blob) {
.path => |path| {
if (path == .fd) {
.path => |*path| {
if (path.* == .fd) {
return globalThis.throwInvalidArguments("Expected a S3 or path to presign", .{});
}
const options = args.nextEat();
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
var blob = try constructS3FileInternalStore(globalThis, &path.path, options);
defer blob.deinit();
return try getPresignUrlFrom(&blob, globalThis, options);
},
Expand All @@ -108,12 +108,12 @@ pub fn unlink(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS
}

switch (path_or_blob) {
.path => |path| {
if (path == .fd) {
.path => |*path| {
if (path.* == .fd) {
return globalThis.throwInvalidArguments("Expected a S3 or path to delete", .{});
}
const options = args.nextEat();
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
var blob = try constructS3FileInternalStore(globalThis, &path.path, options);
defer blob.deinit();
return try blob.store.?.data.s3.unlink(blob.store.?, globalThis, options);
},
Expand Down Expand Up @@ -145,12 +145,12 @@ pub fn write(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSE
};

switch (path_or_blob) {
.path => |path| {
.path => |*path| {
const options = args.nextEat();
if (path == .fd) {
if (path.* == .fd) {
return globalThis.throwInvalidArguments("Expected a S3 or path to upload", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
var blob = try constructS3FileInternalStore(globalThis, &path.path, options);
defer blob.deinit();

var blob_internal: PathOrBlob = .{ .blob = blob };
Expand Down Expand Up @@ -184,12 +184,12 @@ pub fn size(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr
}

switch (path_or_blob) {
.path => |path| {
.path => |*path| {
const options = args.nextEat();
if (path == .fd) {
if (path.* == .fd) {
return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
var blob = try constructS3FileInternalStore(globalThis, &path.path, options);
defer blob.deinit();

return S3BlobStatTask.size(globalThis, &blob);
Expand Down Expand Up @@ -217,12 +217,12 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS
}

switch (path_or_blob) {
.path => |path| {
.path => |*path| {
const options = args.nextEat();
if (path == .fd) {
if (path.* == .fd) {
return globalThis.throwInvalidArguments("Expected a S3 or path to check if it exists", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
var blob = try constructS3FileInternalStore(globalThis, &path.path, options);
defer blob.deinit();

return S3BlobStatTask.exists(globalThis, &blob);
Expand All @@ -235,7 +235,7 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS

fn constructS3FileInternalStore(
globalObject: *jsc.JSGlobalObject,
path: jsc.Node.PathLike,
path: *jsc.Node.PathLike,
options: ?jsc.JSValue,
) bun.JSError!Blob {
// get credentials from env
Expand All @@ -245,7 +245,7 @@ fn constructS3FileInternalStore(
/// if the credentials have changed, we need to clone it, if not we can just ref/deref it
pub fn constructS3FileWithS3CredentialsAndOptions(
globalObject: *jsc.JSGlobalObject,
path: jsc.Node.PathLike,
path: *jsc.Node.PathLike,
options: ?jsc.JSValue,
default_credentials: *S3.S3Credentials,
default_options: bun.S3.MultiPartUploadOptions,
Expand All @@ -258,11 +258,16 @@ pub fn constructS3FileWithS3CredentialsAndOptions(

const store = brk: {
if (aws_options.changed_credentials) {
break :brk bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, bun.default_allocator));
break :brk bun.handleOom(Blob.Store.initS3(path.*, null, aws_options.credentials, bun.default_allocator));
} else {
break :brk bun.handleOom(Blob.Store.initS3WithReferencedCredentials(path, null, default_credentials, bun.default_allocator));
break :brk bun.handleOom(Blob.Store.initS3WithReferencedCredentials(path.*, null, default_credentials, bun.default_allocator));
}
};
// initS3 took ownership of the PathLike (toThreadSafe transfers the
// underlying ref for string-backed paths, and the store now holds the
// only handle for the others); neuter the caller's copy so any outer
// `errdefer path.deinit()` doesn't double-free on a later error.
path.* = .{ .string = bun.PathString.empty };
errdefer store.deinit();
store.data.s3.options = aws_options.options;
store.data.s3.acl = aws_options.acl;
Expand Down Expand Up @@ -300,13 +305,16 @@ pub fn constructS3FileWithS3CredentialsAndOptions(

pub fn constructS3FileWithS3Credentials(
globalObject: *jsc.JSGlobalObject,
path: jsc.Node.PathLike,
path: *jsc.Node.PathLike,
options: ?jsc.JSValue,
existing_credentials: S3.S3Credentials,
) bun.JSError!Blob {
var aws_options = try S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalObject);
defer aws_options.deinit();
const store = bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, bun.default_allocator));
const store = bun.handleOom(Blob.Store.initS3(path.*, null, aws_options.credentials, bun.default_allocator));
// initS3 took ownership of the PathLike; neuter the caller's copy so any
// outer `errdefer path.deinit()` doesn't double-free on a later error.
path.* = .{ .string = bun.PathString.empty };
errdefer store.deinit();
store.data.s3.options = aws_options.options;
store.data.s3.acl = aws_options.acl;
Expand Down Expand Up @@ -343,7 +351,7 @@ pub fn constructS3FileWithS3Credentials(
}
fn constructS3FileInternal(
globalObject: *jsc.JSGlobalObject,
path: jsc.Node.PathLike,
path: *jsc.Node.PathLike,
options: ?jsc.JSValue,
) bun.JSError!*Blob {
return Blob.new(try constructS3FileInternalStore(globalObject, path, options));
Expand Down Expand Up @@ -568,12 +576,12 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr
}

switch (path_or_blob) {
.path => |path| {
.path => |*path| {
const options = args.nextEat();
if (path == .fd) {
if (path.* == .fd) {
return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
var blob = try constructS3FileInternalStore(globalThis, &path.path, options);
defer blob.deinit();

return S3BlobStatTask.stat(globalThis, &blob);
Expand All @@ -586,7 +594,7 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr

pub fn constructInternalJS(
globalObject: *jsc.JSGlobalObject,
path: jsc.Node.PathLike,
path: *jsc.Node.PathLike,
options: ?jsc.JSValue,
) bun.JSError!JSValue {
const blob = try constructS3FileInternal(globalObject, path, options);
Expand All @@ -609,10 +617,11 @@ pub fn constructInternal(
var args = jsc.CallFrame.ArgumentsSlice.init(vm, arguments);
defer args.deinit();

const path = (try jsc.Node.PathLike.fromJS(globalObject, &args)) orelse {
var path = (try jsc.Node.PathLike.fromJS(globalObject, &args)) orelse {
return globalObject.throwInvalidArguments("Expected file path string", .{});
};
return constructS3FileInternal(globalObject, path, args.nextEat());
errdefer path.deinit();
return constructS3FileInternal(globalObject, &path, args.nextEat());
}

pub fn construct(globalObject: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) callconv(jsc.conv) ?*Blob {
Expand Down
Loading
Loading