From 97cc97eace1723014920e734536de8d87617f297 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 7 May 2026 06:32:00 +0000 Subject: [PATCH 1/4] s3: fix path double-free when presign throws after blob creation Blob.Store.initS3 takes ownership of the PathLike, so once the temporary blob is constructed the caller's errdefer on the path must become a no-op. Otherwise a synchronous throw from the subsequent operation (e.g. signRequest failing with missing credentials, or an invalid expiresIn) triggers both defer blob.deinit() and errdefer path.deinit(), underflowing the string refcount and asserting in debug builds. Neutralize the caller's path after ownership transfer in all the S3Client static/instance helpers that follow this pattern. --- src/runtime/webcore/S3Client.zig | 18 ++++++++++------ src/runtime/webcore/S3File.zig | 6 ++++++ test/js/bun/s3/s3-path-double-free.test.ts | 25 ++++++++++++++++++++++ 3 files changed, 43 insertions(+), 6 deletions(-) create mode 100644 test/js/bun/s3/s3-path-double-free.test.ts diff --git a/src/runtime/webcore/S3Client.zig b/src/runtime/webcore/S3Client.zig index 941b8afec671..966db81d0167 100644 --- a/src/runtime/webcore/S3Client.zig +++ b/src/runtime/webcore/S3Client.zig @@ -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(); } @@ -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); } @@ -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(); } @@ -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); } @@ -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(); } @@ -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); } @@ -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(); } @@ -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); } @@ -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(); @@ -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, .{ @@ -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); } diff --git a/src/runtime/webcore/S3File.zig b/src/runtime/webcore/S3File.zig index 41e900fb987f..b5450a125197 100644 --- a/src/runtime/webcore/S3File.zig +++ b/src/runtime/webcore/S3File.zig @@ -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); }, @@ -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); }, @@ -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 }; @@ -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); @@ -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); @@ -574,6 +579,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); diff --git a/test/js/bun/s3/s3-path-double-free.test.ts b/test/js/bun/s3/s3-path-double-free.test.ts new file mode 100644 index 000000000000..b703877be88a --- /dev/null +++ b/test/js/bun/s3/s3-path-double-free.test.ts @@ -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); + }); +}); From 8fbfed409032e17b62d2ad020f6ec356f2ca419f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 7 May 2026 06:51:26 +0000 Subject: [PATCH 2/4] s3: neutralize path inside constructor once initS3 takes ownership The constructors do fallible JS work (reading options.type) after Blob.Store.initS3 has already taken ownership of the path. If that work throws, errdefer store.deinit() releases the path and the caller's errdefer releases it again. Pass the path by pointer and clear it immediately after initS3 succeeds so any caller errdefer becomes a no-op once ownership has transferred, covering both post-construct errors and errors inside the constructor itself. Also add the previously-missing errdefer on the path in constructInternal, staticFile, and the Bun.file s3:// branch. --- src/runtime/webcore/Blob.zig | 3 +- src/runtime/webcore/S3Client.zig | 33 ++++++++--------- src/runtime/webcore/S3File.zig | 41 ++++++++++------------ test/js/bun/s3/s3-path-double-free.test.ts | 32 +++++++++++++++++ 4 files changed, 68 insertions(+), 41 deletions(-) diff --git a/src/runtime/webcore/Blob.zig b/src/runtime/webcore/Blob.zig index 6cf5c0475545..e40bac0dfc48 100644 --- a/src/runtime/webcore/Blob.zig +++ b/src/runtime/webcore/Blob.zig @@ -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); + errdefer path.deinit(); + return try S3File.constructInternalJS(globalObject, &path.path, options); } } defer path.deinitAndUnprotect(); diff --git a/src/runtime/webcore/S3Client.zig b/src/runtime/webcore/S3Client.zig index 966db81d0167..77fb6c97b34f 100644 --- a/src/runtime/webcore/S3Client.zig +++ b/src/runtime/webcore/S3Client.zig @@ -129,7 +129,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 ", .{}).throw(); } @@ -137,7 +137,7 @@ pub const S3Client = struct { }; 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); } @@ -154,8 +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); - path = .{ .string = bun.PathString.empty }; + 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); } @@ -172,8 +171,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 }; + 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); } @@ -190,8 +188,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 }; + 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); } @@ -208,8 +205,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 }; + 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); } @@ -227,8 +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); - path = .{ .string = bun.PathString.empty }; + 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, .{ @@ -243,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); @@ -258,8 +254,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 }; + 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); } @@ -302,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); @@ -320,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); diff --git a/src/runtime/webcore/S3File.zig b/src/runtime/webcore/S3File.zig index b5450a125197..6a9b59bbc0e1 100644 --- a/src/runtime/webcore/S3File.zig +++ b/src/runtime/webcore/S3File.zig @@ -83,8 +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); - path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); return try getPresignUrlFrom(&blob, globalThis, options); }, @@ -114,8 +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); - path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; + 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); }, @@ -152,8 +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); - path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); var blob_internal: PathOrBlob = .{ .blob = blob }; @@ -192,8 +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); - path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); return S3BlobStatTask.size(globalThis, &blob); @@ -226,8 +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); - path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); return S3BlobStatTask.exists(globalThis, &blob); @@ -240,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 @@ -250,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, @@ -263,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; @@ -305,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; @@ -348,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)); @@ -578,8 +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); - path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); return S3BlobStatTask.stat(globalThis, &blob); @@ -592,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); @@ -615,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 { diff --git a/test/js/bun/s3/s3-path-double-free.test.ts b/test/js/bun/s3/s3-path-double-free.test.ts index b703877be88a..2ee37974bb79 100644 --- a/test/js/bun/s3/s3-path-double-free.test.ts +++ b/test/js/bun/s3/s3-path-double-free.test.ts @@ -22,4 +22,36 @@ describe("S3 path ownership on error", () => { expect(() => client.presign(path, badOptions)).toThrow(); Bun.gc(true); }); + + // The constructor does fallible work (reading options.type) after initS3 + // has already taken ownership of the path. A getter that throws on its + // second invocation triggers that error path. + test("S3Client.presign (static) throwing options.type getter", () => { + const path = ["some", "path", Math.random()].join("-"); + let calls = 0; + expect(() => + Bun.S3Client.presign(path, { + get type() { + if (++calls > 1) throw new Error("boom"); + return undefined; + }, + }), + ).toThrow("boom"); + Bun.gc(true); + }); + + test("S3Client#presign (instance) throwing options.type getter", () => { + const client = new Bun.S3Client({}); + const path = ["some", "path", Math.random()].join("-"); + let calls = 0; + expect(() => + client.presign(path, { + get type() { + if (++calls > 1) throw new Error("boom"); + return undefined; + }, + }), + ).toThrow("boom"); + Bun.gc(true); + }); }); From 861406ace0aec66b0d7bc70f9e2c80e99561ebd3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 7 May 2026 07:06:46 +0000 Subject: [PATCH 3/4] test: run S3 path double-free cases in subprocesses The previous test crashed the test runner process directly, so the junit reporter saw zero tests instead of a failure. Spawn each case in a child process and fail the test on non-zero exit / signal so the regression is recorded as a proper test failure. --- test/js/bun/s3/s3-path-double-free.test.ts | 110 +++++++++++++-------- 1 file changed, 71 insertions(+), 39 deletions(-) diff --git a/test/js/bun/s3/s3-path-double-free.test.ts b/test/js/bun/s3/s3-path-double-free.test.ts index 2ee37974bb79..d76d452799ee 100644 --- a/test/js/bun/s3/s3-path-double-free.test.ts +++ b/test/js/bun/s3/s3-path-double-free.test.ts @@ -1,57 +1,89 @@ import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; // 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. +// underflow) and was a silent use-after-free in release builds. +// +// Run each case in a subprocess so a crash is reported as a test failure +// instead of aborting the whole test runner before any result is recorded. 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" }; + function run(body: string) { + const { exitCode, signalCode, stderr } = Bun.spawnSync({ + cmd: [bunExe(), "-e", body], + env: { ...bunEnv, BUN_DEBUG_QUIET_LOGS: "1" }, + stderr: "pipe", + stdout: "ignore", + timeout: 30_000, + }); + const err = stderr.toString(); + if (exitCode !== 0 || signalCode) { + throw new Error(`exit=${exitCode} signal=${signalCode}\n${err}`); + } + expect(err).not.toContain("panic"); + } 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); - }); + run(` + const path = ["some", "path", Math.random()].join("-"); + try { + Bun.S3Client.presign(path, { expiresIn: -1, accessKeyId: "x", secretAccessKey: "y" }); + throw new Error("expected presign to throw"); + } catch (e) { + if (!String(e.message).includes("expiresIn")) throw e; + } + Bun.gc(true); + `); + }, 60_000); 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); - }); + run(` + const client = new Bun.S3Client({}); + const path = ["some", "path", Math.random()].join("-"); + try { + client.presign(path, { expiresIn: -1, accessKeyId: "x", secretAccessKey: "y" }); + throw new Error("expected presign to throw"); + } catch (e) { + if (!String(e.message).includes("expiresIn")) throw e; + } + Bun.gc(true); + `); + }, 60_000); // The constructor does fallible work (reading options.type) after initS3 // has already taken ownership of the path. A getter that throws on its // second invocation triggers that error path. test("S3Client.presign (static) throwing options.type getter", () => { - const path = ["some", "path", Math.random()].join("-"); - let calls = 0; - expect(() => - Bun.S3Client.presign(path, { - get type() { - if (++calls > 1) throw new Error("boom"); - return undefined; - }, - }), - ).toThrow("boom"); - Bun.gc(true); - }); + run(` + const path = ["some", "path", Math.random()].join("-"); + let calls = 0; + try { + Bun.S3Client.presign(path, { + get type() { if (++calls > 1) throw new Error("boom"); return undefined; }, + }); + throw new Error("expected presign to throw"); + } catch (e) { + if (e.message !== "boom") throw e; + } + Bun.gc(true); + `); + }, 60_000); test("S3Client#presign (instance) throwing options.type getter", () => { - const client = new Bun.S3Client({}); - const path = ["some", "path", Math.random()].join("-"); - let calls = 0; - expect(() => - client.presign(path, { - get type() { - if (++calls > 1) throw new Error("boom"); - return undefined; - }, - }), - ).toThrow("boom"); - Bun.gc(true); - }); + run(` + const client = new Bun.S3Client({}); + const path = ["some", "path", Math.random()].join("-"); + let calls = 0; + try { + client.presign(path, { + get type() { if (++calls > 1) throw new Error("boom"); return undefined; }, + }); + throw new Error("expected presign to throw"); + } catch (e) { + if (e.message !== "boom") throw e; + } + Bun.gc(true); + `); + }, 60_000); }); From a5092c4253da6f0cee17874f320e18bd12d36eff Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 7 May 2026 07:57:56 +0000 Subject: [PATCH 4/4] test: drop per-test timeouts and redundant panic assertion --- test/js/bun/s3/s3-path-double-free.test.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/js/bun/s3/s3-path-double-free.test.ts b/test/js/bun/s3/s3-path-double-free.test.ts index d76d452799ee..4fd5e6aa5393 100644 --- a/test/js/bun/s3/s3-path-double-free.test.ts +++ b/test/js/bun/s3/s3-path-double-free.test.ts @@ -17,11 +17,11 @@ describe("S3 path ownership on error", () => { stdout: "ignore", timeout: 30_000, }); - const err = stderr.toString(); - if (exitCode !== 0 || signalCode) { - throw new Error(`exit=${exitCode} signal=${signalCode}\n${err}`); - } - expect(err).not.toContain("panic"); + expect({ stderr: stderr.toString(), exitCode, signalCode }).toEqual({ + stderr: "", + exitCode: 0, + signalCode: undefined, + }); } test("S3Client.presign (static)", () => { @@ -35,7 +35,7 @@ describe("S3 path ownership on error", () => { } Bun.gc(true); `); - }, 60_000); + }); test("S3Client#presign (instance)", () => { run(` @@ -49,7 +49,7 @@ describe("S3 path ownership on error", () => { } Bun.gc(true); `); - }, 60_000); + }); // The constructor does fallible work (reading options.type) after initS3 // has already taken ownership of the path. A getter that throws on its @@ -68,7 +68,7 @@ describe("S3 path ownership on error", () => { } Bun.gc(true); `); - }, 60_000); + }); test("S3Client#presign (instance) throwing options.type getter", () => { run(` @@ -85,5 +85,5 @@ describe("S3 path ownership on error", () => { } Bun.gc(true); `); - }, 60_000); + }); });