Skip to content

fix(s3): avoid double-freeing path when presign throws after store creation #30495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 4 commits into from
Closed

fix(s3): avoid double-freeing path when presign throws after store creation #30495

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6439d14
fix(s3): avoid double-freeing path when presign/unlink/etc throw afte…
robobun May 11, 2026
1136af3
also leave path ownership with caller when constructor throws after i…
robobun May 11, 2026
27af857
add missing errdefer path.deinit() to S3 file constructors
robobun May 11, 2026
ec8bbc1
ci: retrigger (GitHub 500 during git fetch)
robobun May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/runtime/webcore/S3Client.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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 };
Comment thread
claude[bot] marked this conversation as resolved.
defer blob.detach();
return S3File.getPresignUrlFrom(&blob, globalThis, options);
}
Expand All @@ -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();
}
Expand All @@ -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);
}
Expand All @@ -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();
}
Expand All @@ -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);
}
Expand All @@ -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();
}
Expand All @@ -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);
}
Expand All @@ -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();
Expand All @@ -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, .{
Expand All @@ -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);
}
Expand Down
16 changes: 14 additions & 2 deletions src/runtime/webcore/S3File.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
}
const options = args.nextEat();
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } };
defer blob.deinit();
return try getPresignUrlFrom(&blob, globalThis, options);
},
Expand Down Expand Up @@ -114,6 +115,7 @@
}
const options = args.nextEat();
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } };
defer blob.deinit();
return try blob.store.?.data.s3.unlink(blob.store.?, globalThis, options);
},
Expand Down Expand Up @@ -151,6 +153,7 @@
return globalThis.throwInvalidArguments("Expected a S3 or path to upload", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } };
defer blob.deinit();

var blob_internal: PathOrBlob = .{ .blob = blob };
Expand Down Expand Up @@ -190,6 +193,7 @@
return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } };
defer blob.deinit();

return S3BlobStatTask.size(globalThis, &blob);
Expand Down Expand Up @@ -223,6 +227,7 @@
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 = .{ .path = .{ .string = bun.PathString.empty } } };
defer blob.deinit();

return S3BlobStatTask.exists(globalThis, &blob);
Expand Down Expand Up @@ -263,7 +268,10 @@
break :brk bun.handleOom(Blob.Store.initS3WithReferencedCredentials(path, null, default_credentials, bun.default_allocator));
}
};
errdefer store.deinit();
errdefer {
store.data.s3.pathlike = .{ .string = bun.PathString.empty };
store.deinit();
}

Check warning on line 274 in src/runtime/webcore/S3File.zig

View check run for this annotation

Claude / Claude Code Review

Constructor errdefer overwrite can leak pathlike (toThreadSafe clone, and errdefer-less callers) 

The new `errdefer { store.data.s3.pathlike = empty; store.deinit(); }` assumes the store's pathlike still aliases the caller's `path` and that the caller has an `errdefer path.deinit()` — neither always holds. `initS3` calls `toThreadSafe()` on a *local copy*, so for `.slice_with_underlying_string` the store may hold a freshly-cloned impl that is now overwritten without being dereffed; and `S3Client.staticFile` / `S3File.constructInternal` have no `errdefer path.deinit()`, so for them the path i
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
store.data.s3.options = aws_options.options;
store.data.s3.acl = aws_options.acl;
store.data.s3.storage_class = aws_options.storage_class;
Expand Down Expand Up @@ -307,7 +315,10 @@
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));
errdefer store.deinit();
errdefer {
store.data.s3.pathlike = .{ .string = bun.PathString.empty };
store.deinit();
}
store.data.s3.options = aws_options.options;
store.data.s3.acl = aws_options.acl;
store.data.s3.storage_class = aws_options.storage_class;
Expand Down Expand Up @@ -574,6 +585,7 @@
return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{});
}
var blob = try constructS3FileInternalStore(globalThis, path.path, options);
path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } };
defer blob.deinit();

return S3BlobStatTask.stat(globalThis, &blob);
Expand Down
52 changes: 52 additions & 0 deletions test/js/bun/s3/s3-presign-error-path-free.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { describe, expect, test } from "bun:test";

// When an S3 operation throws after the blob store has been constructed
// (e.g. invalid presign options), the path was being freed twice: once by
// the blob's defer cleanup and again by the caller's errdefer. This tripped
// a refcount assertion in debug builds.

describe("S3 presign error after store construction does not double-free path", () => {
test("static S3Client.presign", () => {
expect(() => Bun.S3Client.presign("some/path", { expiresIn: -1 })).toThrow("expiresIn");
Bun.gc(true);
});

test("static S3Client.presign with non-latin1 path", () => {
expect(() => Bun.S3Client.presign("\u{1F600}/path", { expiresIn: -1 })).toThrow("expiresIn");
Bun.gc(true);
});

test("Bun.s3.presign", () => {
expect(() => Bun.s3.presign("some/path", { expiresIn: -1 })).toThrow("expiresIn");
Bun.gc(true);
});

test("instance presign", () => {
const client = new Bun.S3Client({
accessKeyId: "x",
secretAccessKey: "y",
bucket: "b",
endpoint: "http://localhost",
});
expect(() => client.presign("some/path", { expiresIn: -1 })).toThrow("expiresIn");
Bun.gc(true);
});

test("options.type getter throws after store is created", () => {
const client = new Bun.S3Client({
accessKeyId: "x",
secretAccessKey: "y",
bucket: "b",
endpoint: "http://localhost",
});
let n = 0;
const opts = {
get type() {
if (n++ === 0) return undefined;
throw new Error("boom");
},
};
expect(() => client.presign("\u{1F600}/k", opts)).toThrow("boom");
Bun.gc(true);
});
});
Loading