From dc475c87cebb3ae7f639e97bb05b454b078fe8b7 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 10 May 2026 22:47:24 +0000 Subject: [PATCH] Fix double-free of path string in S3Client methods on error When an S3 operation (presign, exists, size, stat, write, unlink) given a path string throws after constructing the internal blob store (e.g. missing credentials), ownership of the path has already transferred to the blob. Both defer blob.deinit() and the outer errdefer path.deinit() would then fire, over-releasing the underlying StringImpl and crashing. Neutralize the caller's path variable once the blob takes ownership so the errdefer becomes a no-op on later errors. --- src/runtime/webcore/S3Client.zig | 18 ++++--- src/runtime/webcore/S3File.zig | 6 +++ test/js/bun/s3/s3-path-double-free.test.ts | 56 ++++++++++++++++++++++ 3 files changed, 74 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..18530a834fe7 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 = .{ .fd = bun.invalid_fd } }; 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 = .{ .fd = bun.invalid_fd } }; 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 = .{ .fd = bun.invalid_fd } }; 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 = .{ .fd = bun.invalid_fd } }; 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 = .{ .fd = bun.invalid_fd } }; 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 = .{ .fd = bun.invalid_fd } }; 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..0e220757f52b --- /dev/null +++ b/test/js/bun/s3/s3-path-double-free.test.ts @@ -0,0 +1,56 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; + +// When an S3 operation given a path string throws after constructing the +// internal blob (e.g. missing credentials), the path string must not be +// dereferenced twice. Previously both `defer blob.deinit()` and the outer +// `errdefer path.deinit()` fired, over-releasing the underlying StringImpl. +test("S3Client methods do not double-free the path string when they throw", () => { + const { exitCode, stdout, stderr, signalCode } = Bun.spawnSync({ + cmd: [ + bunExe(), + "-e", + ` + process.on("unhandledRejection", () => {}); + const methods = ["presign", "exists", "size", "stat", "unlink", "delete"]; + + for (const m of methods) { + for (let i = 0; i < 3; i++) { + try { Bun.S3Client[m]("some/key/here.txt"); } catch {} + try { Bun.S3Client[m]("some/key/here.txt", "not an object"); } catch {} + } + } + for (let i = 0; i < 3; i++) { + try { Bun.S3Client.write("some/key/here.txt", "data", "not an object")?.catch?.(() => {}); } catch {} + } + + const client = new Bun.S3Client({}); + for (const m of methods) { + for (let i = 0; i < 3; i++) { + try { client[m]("some/key/here.txt"); } catch {} + try { client[m]("some/key/here.txt", "not an object"); } catch {} + } + } + for (let i = 0; i < 3; i++) { + try { client.write("some/key/here.txt", "data", "not an object")?.catch?.(() => {}); } catch {} + } + + Bun.gc(true); + console.log("ok"); + `, + ], + env: { + ...bunEnv, + AWS_ACCESS_KEY_ID: "", + AWS_SECRET_ACCESS_KEY: "", + S3_ACCESS_KEY_ID: "", + S3_SECRET_ACCESS_KEY: "", + }, + stdout: "pipe", + stderr: "pipe", + }); + expect(stderr.toString()).not.toContain("panic"); + expect(signalCode).toBeFalsy(); + expect(stdout.toString().trim()).toBe("ok"); + expect(exitCode).toBe(0); +});