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); +});