Skip to content

fix(s3): avoid double-freeing path when presign throws after store creation #30495

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
Closed

fix(s3): avoid double-freeing path when presign throws after store creation #30495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6439d14
fix(s3): avoid double-freeing path when presign/unlink/etc throw afte…
robobun May 11, 2026
1136af3
also leave path ownership with caller when constructor throws after i…
robobun May 11, 2026
27af857
add missing errdefer path.deinit() to S3 file constructors
robobun May 11, 2026
ec8bbc1
ci: retrigger (GitHub 500 during git fetch)
robobun May 11, 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
1 change: 1 addition & 0 deletions src/runtime/webcore/Blob.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2069,6 +2069,7 @@ pub fn constructBunFile(

if (path == .path) {
if (strings.hasPrefixComptime(path.path.slice(), "s3://")) {
errdefer path.deinit();
return try S3File.constructInternalJS(globalObject, path.path, options);
}
}
Expand Down
20 changes: 13 additions & 7 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 };
Comment thread
claude[bot] marked this conversation as resolved.
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 Expand Up @@ -299,7 +305,7 @@ pub const S3Client = struct {
const 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());
}
pub fn staticStat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSError!JSValue {
Expand Down
17 changes: 15 additions & 2 deletions src/runtime/webcore/S3File.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J
}
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 getPresignUrlFrom(&blob, globalThis, options);
},
Expand Down Expand Up @@ -114,6 +115,7 @@ pub fn unlink(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS
}
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 @@ pub fn write(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSE
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 @@ pub fn size(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr
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 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS
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 @@ -263,7 +268,10 @@ pub fn constructS3FileWithS3CredentialsAndOptions(
break :brk bun.handleOom(Blob.Store.initS3WithReferencedCredentials(path, null, default_credentials, bun.default_allocator));
}
};
errdefer store.deinit();
errdefer {
store.data.s3.pathlike = .{ .string = bun.PathString.empty };
store.deinit();
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
store.data.s3.options = aws_options.options;
store.data.s3.acl = aws_options.acl;
store.data.s3.storage_class = aws_options.storage_class;
Expand Down Expand Up @@ -307,7 +315,10 @@ pub fn constructS3FileWithS3Credentials(
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));
errdefer store.deinit();
errdefer {
store.data.s3.pathlike = .{ .string = bun.PathString.empty };
store.deinit();
}
store.data.s3.options = aws_options.options;
store.data.s3.acl = aws_options.acl;
store.data.s3.storage_class = aws_options.storage_class;
Expand Down Expand Up @@ -574,6 +585,7 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr
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 Expand Up @@ -612,6 +624,7 @@ pub fn constructInternal(
const path = (try jsc.Node.PathLike.fromJS(globalObject, &args)) orelse {
return globalObject.throwInvalidArguments("Expected file path string", .{});
};
errdefer path.deinit();
return constructS3FileInternal(globalObject, path, args.nextEat());
}

Expand Down
52 changes: 52 additions & 0 deletions test/js/bun/s3/s3-presign-error-path-free.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { describe, expect, test } from "bun:test";

// When an S3 operation throws after the blob store has been constructed
// (e.g. invalid presign options), the path was being freed twice: once by
// the blob's defer cleanup and again by the caller's errdefer. This tripped
// a refcount assertion in debug builds.

describe("S3 presign error after store construction does not double-free path", () => {
test("static S3Client.presign", () => {
expect(() => Bun.S3Client.presign("some/path", { expiresIn: -1 })).toThrow("expiresIn");
Bun.gc(true);
});

test("static S3Client.presign with non-latin1 path", () => {
expect(() => Bun.S3Client.presign("\u{1F600}/path", { expiresIn: -1 })).toThrow("expiresIn");
Bun.gc(true);
});

test("Bun.s3.presign", () => {
expect(() => Bun.s3.presign("some/path", { expiresIn: -1 })).toThrow("expiresIn");
Bun.gc(true);
});

test("instance presign", () => {
const client = new Bun.S3Client({
accessKeyId: "x",
secretAccessKey: "y",
bucket: "b",
endpoint: "http://localhost",
});
expect(() => client.presign("some/path", { expiresIn: -1 })).toThrow("expiresIn");
Bun.gc(true);
});

test("options.type getter throws after store is created", () => {
const client = new Bun.S3Client({
accessKeyId: "x",
secretAccessKey: "y",
bucket: "b",
endpoint: "http://localhost",
});
let n = 0;
const opts = {
get type() {
if (n++ === 0) return undefined;
throw new Error("boom");
},
};
expect(() => client.presign("\u{1F600}/k", opts)).toThrow("boom");
Bun.gc(true);
});
});
Loading