Skip to content

S3Client: don't double-deref path when a method fails after blob construction #30419

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

S3Client: don't double-deref path when a method fails after blob construction #30419

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
83c898a
S3Client.presign: don't double-deref path on sign error
robobun May 9, 2026
e2c0455
S3File: reuse parsed content_type instead of re-reading options.type
robobun May 9, 2026
c8149ec
S3Client: clear path handle after blob construction in all siblings
robobun May 9, 2026
a044067
ci: retrigger
robobun May 9, 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 };
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
80 changes: 28 additions & 52 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);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
},
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 @@ -256,45 +261,23 @@ pub fn constructS3FileWithS3CredentialsAndOptions(
var aws_options = try S3.S3Credentials.getCredentialsWithOptions(default_credentials.*, default_options, options, default_acl, default_storage_class, default_request_payer, globalObject);
defer aws_options.deinit();

// initS3 transfers ownership of `path` to the store. Nothing below may
// return an error: the caller's `errdefer path.deinit()` still aliases
// the same string the store now owns.
const store = brk: {
if (aws_options.changed_credentials) {
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));
}
};
errdefer 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;
store.data.s3.request_payer = aws_options.request_payer;

var blob = Blob.initWithStore(store, globalObject);
if (options) |opts| {
if (opts.isObject()) {
if (try opts.getTruthyComptime(globalObject, "type")) |file_type| {
inner: {
if (file_type.isString()) {
var allocator = bun.default_allocator;
var str = try file_type.toSlice(globalObject, bun.default_allocator);
defer str.deinit();
const slice = str.slice();
if (!strings.isAllASCII(slice)) {
break :inner;
}
blob.content_type_was_set = true;
if (globalObject.bunVM().mimeType(str.slice())) |entry| {
blob.content_type = entry.value;
break :inner;
}
const content_type_buf = bun.handleOom(allocator.alloc(u8, slice.len));
blob.content_type = strings.copyLowercase(slice, content_type_buf);
blob.content_type_allocated = true;
}
}
}
}
}
setBlobContentType(&blob, aws_options.content_type, globalObject);
return blob;
}

Expand All @@ -306,41 +289,33 @@ pub fn constructS3FileWithS3Credentials(
) bun.JSError!Blob {
var aws_options = try S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalObject);
defer aws_options.deinit();
// initS3 transfers ownership of `path` to the store. Nothing below may
// return an error: the caller's `errdefer path.deinit()` still aliases
// the same string the store now owns.
const store = bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, bun.default_allocator));
errdefer 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;
store.data.s3.request_payer = aws_options.request_payer;

var blob = Blob.initWithStore(store, globalObject);
if (options) |opts| {
if (opts.isObject()) {
if (try opts.getTruthyComptime(globalObject, "type")) |file_type| {
inner: {
if (file_type.isString()) {
var allocator = bun.default_allocator;
var str = try file_type.toSlice(globalObject, bun.default_allocator);
defer str.deinit();
const slice = str.slice();
if (!strings.isAllASCII(slice)) {
break :inner;
}
blob.content_type_was_set = true;
if (globalObject.bunVM().mimeType(str.slice())) |entry| {
blob.content_type = entry.value;
break :inner;
}
const content_type_buf = bun.handleOom(allocator.alloc(u8, slice.len));
blob.content_type = strings.copyLowercase(slice, content_type_buf);
blob.content_type_allocated = true;
}
}
}
}
}
setBlobContentType(&blob, aws_options.content_type, globalObject);
return blob;
}

fn setBlobContentType(blob: *Blob, content_type: ?[]const u8, globalObject: *jsc.JSGlobalObject) void {
const slice = content_type orelse return;
if (!strings.isAllASCII(slice)) return;
blob.content_type_was_set = true;
if (globalObject.bunVM().mimeType(slice)) |entry| {
blob.content_type = entry.value;
return;
}
const content_type_buf = bun.handleOom(bun.default_allocator.alloc(u8, slice.len));
blob.content_type = strings.copyLowercase(slice, content_type_buf);
blob.content_type_allocated = true;
}

fn constructS3FileInternal(
globalObject: *jsc.JSGlobalObject,
path: jsc.Node.PathLike,
Expand Down Expand Up @@ -574,6 +549,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
51 changes: 51 additions & 0 deletions test/js/bun/s3/s3-presign-missing-credentials.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";

// The path string's ref is transferred to the blob store; when the operation
// fails synchronously after that point the errdefer cleanup must not deref it
// a second time.
test("S3Client path methods throw instead of crashing on synchronous errors", async () => {
const env = { ...bunEnv };
for (const k of [
"S3_ACCESS_KEY_ID",
"S3_SECRET_ACCESS_KEY",
"S3_BUCKET",
"S3_ENDPOINT",
"S3_REGION",
"S3_SESSION_TOKEN",
"AWS_ACCESS_KEY_ID",
"AWS_SECRET_ACCESS_KEY",
"AWS_BUCKET",
"AWS_ENDPOINT",
"AWS_REGION",
"AWS_SESSION_TOKEN",
]) {
delete env[k];
}

await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const out = [];
try { Bun.S3Client.presign("myfile"); } catch (e) { out.push(e.code); }
try { new Bun.S3Client({}).presign("myfile"); } catch (e) { out.push(e.code); }
try { Bun.S3Client.write("myfile", null); } catch (e) { out.push(e.code); }
try { new Bun.S3Client({}).write("myfile", null); } catch (e) { out.push(e.code); }
console.log(out.join(":"));
`,
],
env,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
expect(stdout.trim()).toBe(
"ERR_S3_MISSING_CREDENTIALS:ERR_S3_MISSING_CREDENTIALS:ERR_INVALID_ARG_TYPE:ERR_INVALID_ARG_TYPE",
);
expect(exitCode).toBe(0);
});
Loading