Skip to content

s3: fix path double-deinit when operation throws after store creation #30567

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 6 commits into from
Closed

s3: fix path double-deinit when operation throws after store creation #30567

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94b41ae
s3: fix path double-deinit when operation throws after store creation
robobun May 12, 2026
455c262
add test coverage for presign expiresIn and type getter throwing insi…
robobun May 12, 2026
f9cb610
test: use explicit credentials to avoid env-dependent presign behavior
robobun May 12, 2026
fb1a44c
test: cover Bun.file('s3://') and S3Client.file with throwing type ge…
robobun May 12, 2026
f03a4c2
ci: retrigger
robobun May 12, 2026
446fdf7
test: make type-getter test throw on second read so it fires after in…
robobun May 12, 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 @@ pub fn constructBunFile(

if (path == .path) {
if (strings.hasPrefixComptime(path.path.slice(), "s3://")) {
return try S3File.constructInternalJS(globalObject, path.path, options);
defer path.deinitAndUnprotect();
return try S3File.constructInternalJS(globalObject, &path.path, options);
}
}
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);
}
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
35 changes: 19 additions & 16 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,7 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J
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_or_blob.path.path, options);
defer blob.deinit();
return try getPresignUrlFrom(&blob, globalThis, options);
},
Expand Down Expand Up @@ -113,7 +113,7 @@ pub fn unlink(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS
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_or_blob.path.path, options);
defer blob.deinit();
return try blob.store.?.data.s3.unlink(blob.store.?, globalThis, options);
},
Expand Down Expand Up @@ -150,7 +150,7 @@ pub fn write(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSE
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_or_blob.path.path, options);
defer blob.deinit();

var blob_internal: PathOrBlob = .{ .blob = blob };
Expand Down Expand Up @@ -189,7 +189,7 @@ pub fn size(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr
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_or_blob.path.path, options);
defer blob.deinit();

return S3BlobStatTask.size(globalThis, &blob);
Expand Down Expand Up @@ -222,7 +222,7 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS
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_or_blob.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,12 @@ 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));
}
};
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 +301,14 @@ 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));
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 +345,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 @@ -573,7 +575,7 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr
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_or_blob.path.path, options);
defer blob.deinit();

return S3BlobStatTask.stat(globalThis, &blob);
Expand All @@ -586,7 +588,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 +611,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
29 changes: 29 additions & 0 deletions test/js/bun/s3/s3-write-throwing-data.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { expect, test } from "bun:test";

// When a later step of an S3 operation throws after the blob store has taken
// ownership of the path string, the caller's `errdefer path.deinit()` used to
// run as well, over-dereffing the path's WTFStringImpl (whose ref had already
// been transferred by `toThreadSafe()` inside `Store.initS3`).
const throwing = {
[Symbol.toPrimitive]() {
throw new Error("boom");
},
};

test("S3Client#write propagates data coercion errors without crashing", () => {
const s3 = new Bun.S3Client();
expect(() => s3.write("ab", throwing)).toThrow("boom");
expect(() => s3.write("some/longer/path", [throwing, throwing])).toThrow("boom");
});

test("S3Client.write (static) propagates data coercion errors without crashing", () => {
const opts = { accessKeyId: "a", secretAccessKey: "b", bucket: "c", endpoint: "http://localhost:1" };
expect(() => Bun.S3Client.write("ab", throwing, opts)).toThrow("boom");
expect(() => Bun.S3Client.write("some/longer/path", [throwing], opts)).toThrow("boom");
});

test("S3Client#presign with missing credentials does not crash", () => {
const s3 = new Bun.S3Client();
expect(() => s3.presign("ab")).toThrow("Missing S3 credentials");
expect(() => s3.presign("some/longer/path")).toThrow("Missing S3 credentials");
});

Check warning on line 29 in test/js/bun/s3/s3-write-throwing-data.test.ts

View check run for this annotation

Claude / Claude Code Review

presign test is environment-dependent 

This test is environment-dependent: `new Bun.S3Client()` with no options reads `AWS_ACCESS_KEY_ID`/`AWS_SECRET_ACCESS_KEY` (or the `S3_*` equivalents) from the process environment, and `signRequest` only throws `MissingCredentials` when those are empty. On any dev machine or CI agent with AWS env vars exported, `s3.presign("ab")` will succeed and return a URL, failing the `.toThrow("Missing S3 credentials")` assertion — pass `{ accessKeyId: "", secretAccessKey: "" }` to the constructor to make i
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Loading