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 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-path-double-free.test.ts b/test/js/bun/s3/s3-path-double-free.test.ts new file mode 100644 index 000000000000..4fd5e6aa5393 --- /dev/null +++ b/test/js/bun/s3/s3-path-double-free.test.ts @@ -0,0 +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) 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", () => { + 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, + }); + expect({ stderr: stderr.toString(), exitCode, signalCode }).toEqual({ + stderr: "", + exitCode: 0, + signalCode: undefined, + }); + } + + test("S3Client.presign (static)", () => { + 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); + `); + }); + + test("S3Client#presign (instance)", () => { + 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); + `); + }); + + // 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", () => { + 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); + `); + }); + + test("S3Client#presign (instance) throwing options.type getter", () => { + 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); + `); + }); +});