diff --git a/src/bun.js/webcore/S3Client.zig b/src/bun.js/webcore/S3Client.zig index 941b8afec671..2a8934df7d94 100644 --- a/src/bun.js/webcore/S3Client.zig +++ b/src/bun.js/webcore/S3Client.zig @@ -135,7 +135,6 @@ pub const S3Client = struct { } 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)); return blob.toJS(globalThis); @@ -151,7 +150,6 @@ pub const S3Client = struct { } return globalThis.throwInvalidArguments("Expected a path to presign", .{}); }; - 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); @@ -169,7 +167,6 @@ pub const S3Client = struct { } 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); defer blob.detach(); @@ -186,7 +183,6 @@ pub const S3Client = struct { } 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); defer blob.detach(); @@ -203,7 +199,6 @@ pub const S3Client = struct { } 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); defer blob.detach(); @@ -217,8 +212,8 @@ pub const S3Client = struct { const 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(); const data = args.nextEat() orelse { + path.deinit(); return globalThis.ERR(.MISSING_ARGS, "Expected a Blob-y thing to write", .{}).throw(); }; @@ -251,7 +246,6 @@ pub const S3Client = struct { const 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); defer blob.detach(); diff --git a/src/bun.js/webcore/S3File.zig b/src/bun.js/webcore/S3File.zig index 41e900fb987f..8fbaf0f0028a 100644 --- a/src/bun.js/webcore/S3File.zig +++ b/src/bun.js/webcore/S3File.zig @@ -67,11 +67,6 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J // accept a path or a blob var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args); - errdefer { - if (path_or_blob == .path) { - path_or_blob.path.deinit(); - } - } if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) { return globalThis.throwInvalidArguments("Expected a S3 or path to presign", .{}); @@ -97,12 +92,7 @@ pub fn unlink(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS defer args.deinit(); // accept a path or a blob - var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args); - errdefer { - if (path_or_blob == .path) { - path_or_blob.path.deinit(); - } - } + const path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args); if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) { return globalThis.throwInvalidArguments("Expected a S3 or path to delete", .{}); } @@ -130,17 +120,13 @@ pub fn write(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSE // accept a path or a blob var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args); - errdefer { - if (path_or_blob == .path) { - path_or_blob.path.deinit(); - } - } if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) { return globalThis.throwInvalidArguments("Expected a S3 or path to upload", .{}); } const data = args.nextEat() orelse { + if (path_or_blob == .path) path_or_blob.path.deinit(); return globalThis.ERR(.MISSING_ARGS, "Expected a Blob-y thing to upload", .{}).throw(); }; @@ -173,11 +159,6 @@ pub fn size(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr // accept a path or a blob var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args); - errdefer { - if (path_or_blob == .path) { - path_or_blob.path.deinit(); - } - } if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) { return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); @@ -206,11 +187,6 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS // accept a path or a blob var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args); - errdefer { - if (path_or_blob == .path) { - path_or_blob.path.deinit(); - } - } if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) { return globalThis.throwInvalidArguments("Expected a S3 or path to check if it exists", .{}); @@ -253,7 +229,12 @@ pub fn constructS3FileWithS3CredentialsAndOptions( default_storage_class: ?bun.S3.StorageClass, default_request_payer: bool, ) bun.JSError!Blob { - var aws_options = try S3.S3Credentials.getCredentialsWithOptions(default_credentials.*, default_options, options, default_acl, default_storage_class, default_request_payer, globalObject); + // This function always takes ownership of `path`. The store created below consumes it; + // if we fail before the store is created, we must release it ourselves. + var aws_options = S3.S3Credentials.getCredentialsWithOptions(default_credentials.*, default_options, options, default_acl, default_storage_class, default_request_payer, globalObject) catch |err| { + path.deinit(); + return err; + }; defer aws_options.deinit(); const store = brk: { @@ -304,7 +285,12 @@ pub fn constructS3FileWithS3Credentials( options: ?jsc.JSValue, existing_credentials: S3.S3Credentials, ) bun.JSError!Blob { - var aws_options = try S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalObject); + // This function always takes ownership of `path`. The store created below consumes it; + // if we fail before the store is created, we must release it ourselves. + var aws_options = S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalObject) catch |err| { + path.deinit(); + return err; + }; defer aws_options.deinit(); const store = bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, bun.default_allocator)); errdefer store.deinit(); @@ -557,11 +543,6 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr // accept a path or a blob var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args); - errdefer { - if (path_or_blob == .path) { - path_or_blob.path.deinit(); - } - } if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) { return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); diff --git a/test/js/bun/s3/s3-path-error-double-free.test.ts b/test/js/bun/s3/s3-path-error-double-free.test.ts new file mode 100644 index 000000000000..6ff69b4d6abb --- /dev/null +++ b/test/js/bun/s3/s3-path-error-double-free.test.ts @@ -0,0 +1,114 @@ +import { S3Client } from "bun"; +import { describe, expect, test } from "bun:test"; + +// Regression test: S3Client methods would double-free the path string when an +// error was thrown after the internal S3 blob store had already taken ownership +// of the path (via toThreadSafe()). The `errdefer path.deinit()` in the caller +// then tried to release the same underlying WTFStringImpl again, tripping a +// refcount assertion in debug builds. The bug only reproduced when the path +// string was not already a pre-existing atom (so isolatedCopy() produced a new +// StringImpl and deref'd the original). + +describe("S3Client path ownership on error", () => { + const throwingData = { + [Symbol.toPrimitive]() { + throw new Error("boom"); + }, + }; + + const throwingCredOptions = { + get accessKeyId(): string { + throw new Error("cred-boom"); + }, + }; + + const throwingTypeOptions = { + get type(): string { + throw new Error("type-boom"); + }, + }; + + // Use a path string that is unlikely to already exist as an atom in the VM. + let counter = 0; + const freshPath = () => `zzz-unique-s3-path-${process.pid}-${counter++}`; + + describe("instance methods", () => { + const client = new S3Client(); + + test("write() with data whose string coercion throws", () => { + expect(() => client.write(freshPath(), throwingData)).toThrow("boom"); + }); + + test("write() with throwing options.type", () => { + expect(() => client.write(freshPath(), throwingData, throwingTypeOptions)).toThrow(); + }); + + test("file() with throwing credentials option", () => { + expect(() => client.file(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("presign() with throwing credentials option", () => { + expect(() => client.presign(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("presign() with throwing options.type", () => { + expect(() => client.presign(freshPath(), throwingTypeOptions)).toThrow("type-boom"); + }); + + test("exists() with throwing credentials option", () => { + expect(() => client.exists(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("size() with throwing credentials option", () => { + expect(() => client.size(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("stat() with throwing credentials option", () => { + expect(() => client.stat(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("unlink() with throwing credentials option", () => { + expect(() => client.unlink(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("write() with missing data argument", () => { + // @ts-expect-error + expect(() => client.write(freshPath())).toThrow(); + }); + }); + + describe("static methods", () => { + test("write() with data whose string coercion throws", () => { + expect(() => S3Client.write(freshPath(), throwingData)).toThrow("boom"); + }); + + test("presign() with throwing options.type", () => { + expect(() => S3Client.presign(freshPath(), throwingTypeOptions)).toThrow("type-boom"); + }); + + test("exists() with throwing credentials option", () => { + expect(() => S3Client.exists(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("size() with throwing credentials option", () => { + expect(() => S3Client.size(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("stat() with throwing credentials option", () => { + expect(() => S3Client.stat(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("unlink() with throwing credentials option", () => { + expect(() => S3Client.unlink(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("file() with throwing credentials option", () => { + expect(() => S3Client.file(freshPath(), throwingCredOptions)).toThrow("cred-boom"); + }); + + test("write() with missing data argument", () => { + // @ts-expect-error + expect(() => S3Client.write(freshPath())).toThrow(); + }); + }); +});