diff --git a/src/runtime/webcore/Blob.zig b/src/runtime/webcore/Blob.zig index 6cf5c0475545..1763662bfbe1 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); + defer path.deinitAndUnprotect(); + 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 941b8afec671..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); } @@ -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(); } @@ -154,7 +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); + 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); } @@ -163,7 +163,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(); } @@ -171,7 +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); + 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); } @@ -180,7 +180,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(); } @@ -188,7 +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); + 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); } @@ -197,7 +197,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(); } @@ -205,7 +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); + 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); } @@ -214,7 +214,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(); @@ -223,7 +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); + 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, .{ @@ -238,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); @@ -248,12 +249,12 @@ 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); + 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); } @@ -296,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); @@ -314,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 41e900fb987f..6a9b59bbc0e1 100644 --- a/src/runtime/webcore/S3File.zig +++ b/src/runtime/webcore/S3File.zig @@ -83,7 +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); + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); return try getPresignUrlFrom(&blob, globalThis, options); }, @@ -113,7 +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); + 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); }, @@ -150,7 +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); + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); var blob_internal: PathOrBlob = .{ .blob = blob }; @@ -189,7 +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); + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); return S3BlobStatTask.size(globalThis, &blob); @@ -222,7 +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); + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); return S3BlobStatTask.exists(globalThis, &blob); @@ -235,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 @@ -245,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, @@ -258,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; @@ -300,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; @@ -343,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)); @@ -573,7 +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); + var blob = try constructS3FileInternalStore(globalThis, &path_or_blob.path.path, options); defer blob.deinit(); return S3BlobStatTask.stat(globalThis, &blob); @@ -586,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); @@ -609,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-write-throwing-data.test.ts b/test/js/bun/s3/s3-write-throwing-data.test.ts new file mode 100644 index 000000000000..22b21c48263a --- /dev/null +++ b/test/js/bun/s3/s3-write-throwing-data.test.ts @@ -0,0 +1,51 @@ +import { expect, test } from "bun:test"; + +// When a later step of an S3 operation throws after the blob store has taken +// ownership of the path string, the caller's `errdefer path.deinit()` used to +// run as well, over-dereffing the path's WTFStringImpl (whose ref had already +// been transferred by `toThreadSafe()` inside `Store.initS3`). +const throwing = { + [Symbol.toPrimitive]() { + throw new Error("boom"); + }, +}; + +const opts = { accessKeyId: "a", secretAccessKey: "b", bucket: "c", endpoint: "http://localhost:1" }; + +test("S3Client#write propagates data coercion errors without crashing", () => { + const s3 = new Bun.S3Client(opts); + expect(() => s3.write("ab", throwing)).toThrow("boom"); + expect(() => s3.write("some/longer/path", [throwing, throwing])).toThrow("boom"); +}); + +test("S3Client.write (static) propagates data coercion errors without crashing", () => { + expect(() => Bun.S3Client.write("ab", throwing, opts)).toThrow("boom"); + expect(() => Bun.S3Client.write("some/longer/path", [throwing], opts)).toThrow("boom"); +}); + +test("S3Client presign with invalid expiresIn does not crash", () => { + expect(() => Bun.S3Client.presign("some/path", { ...opts, expiresIn: -1 })).toThrow("expiresIn"); + expect(() => Bun.S3Client.presign("\u{1F600}/path", { ...opts, expiresIn: -1 })).toThrow("expiresIn"); + const s3 = new Bun.S3Client(opts); + expect(() => s3.presign("some/path", { expiresIn: -1 })).toThrow("expiresIn"); + expect(() => s3.presign("ab", { expiresIn: -1 })).toThrow("expiresIn"); +}); + +test("S3 file construction with options.type getter throwing after initS3 does not crash", () => { + // `getCredentialsWithOptions` reads `type` before `initS3`, and the constructor + // reads it again after. Return a value on the first read and throw on the second + // so the throw happens after the path has been moved into the store. + function throwingTypeAfterFirst() { + let n = 0; + return { + get type() { + if (n++ === 0) return undefined; + throw new Error("type-boom"); + }, + }; + } + const s3 = new Bun.S3Client(opts); + expect(() => s3.file("some/path", throwingTypeAfterFirst())).toThrow("type-boom"); + expect(() => Bun.file("s3://bucket/some/path", throwingTypeAfterFirst())).toThrow("type-boom"); + expect(() => Bun.S3Client.file("some/path", throwingTypeAfterFirst())).toThrow("type-boom"); +});