fix(s3): double free of path when S3Client operation throws - #29656
fix(s3): double free of path when S3Client operation throws#29656robobun wants to merge 4 commits into
Conversation
|
Updated 5:06 PM PT - May 4th, 2026
❌ @robobun, your commit 988f170 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29656That installs a local version of the PR into your bun-29656 --bun |
WalkthroughRemoves broad Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/s3/s3-path-double-free.test.ts`:
- Around line 9-89: Add unit tests for S3Client.exists, S3Client.size,
S3Client.stat, and S3Client.unlink to mirror the existing presign/file coverage:
create tests that exercise both "throwing before blob creation" and, where
applicable, static variants, by passing option objects whose getter (e.g., get
type() or get method()) throws an Error("boom") to ensure the path ownership is
not double-freed; use the same pattern and nonAsciiPath variable and assertions
(expect(() => client.exists(...)).toThrow("boom"), etc.) and add analogous
static tests for Bun.S3Client.* where the API exposes static helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b3f9202-03fe-4f56-9f5b-a652591a6534
📒 Files selected for processing (3)
src/bun.js/webcore/S3Client.zigsrc/bun.js/webcore/S3File.zigtest/js/bun/s3/s3-path-double-free.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the ownership transfer looks correct across all call sites I traced — but this shifts PathLike ownership semantics across a module boundary and relies on the neutralize-the-errdefer pattern in six places, so it's worth a maintainer sanity-checking the memory model.
Extended reasoning...
Overview
Fixes an ASAN double-free in S3Client/S3File by changing constructS3FileWithS3Credentials[AndOptions] to unconditionally take ownership of the path argument. Callers in S3Client.zig drop their errdefer path.deinit(); static callers in S3File.zig overwrite path_or_blob with an empty .string sentinel before handing the captured path.path to the constructor, so their existing errdefer becomes a no-op once ownership is gone. A new test exercises both before-blob and after-blob throw paths with a non-ASCII key (forces an allocated encoded_slice).
Security risks
None. This is internal memory-lifetime management on error paths; no auth, crypto, or untrusted-input parsing is touched.
Level of scrutiny
Moderate-to-high. The diff is mechanically small, but it redefines ownership of a heap-backed value across ~13 call sites and two files. Zig errdefer/defer interactions are exactly where double-free and leak bugs hide, and the fix depends on every caller having no remaining error path between obtaining path and passing it to the constructor (I verified args.nextEat() is infallible and write's missing-data branch now frees explicitly). The switch-capture-by-value + reassign-the-original trick in S3File.zig is correct (PathLike.deinit on .string is a no-op per types.zig:544) but unusual enough that a maintainer should confirm it matches house style.
Other factors
- All callers of the two constructors (including
staticFile,constructInternal, and thelistObjectspaths that passPathString.empty) are consistent with the new "callee owns" contract. - New test covers the regression but cannot prove absence of leaks on the now-uncovered early-error branches; ASAN/valgrind in CI is the real backstop.
- No prior reviews from me or others; deferring rather than approving given the memory-safety blast radius.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/s3/s3-path-double-free.test.ts`:
- Around line 49-61: Replace the parameterized test block that uses
test.each(...) with describe.each(...) so it follows the repository convention:
change test.each(["exists","size","stat","unlink"] as const)( "instance %s()
throwing before blob creation", method => { ... }) into describe.each(...)(
"instance %s() throwing before blob creation", method => { it("throws before
blob creation", () => { const client = new Bun.S3Client(); expect(() =>
client[method](nonAsciiPath, { get type() { throw new Error("boom"); },
})).toThrow("boom"); }); }); do the same replacement for the other parameterized
block (the one referenced at 104-115) so both use describe.each and inner
it()/test() for the actual assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3658092f-a2cc-4355-9e07-8fd6341efeb8
📒 Files selected for processing (1)
test/js/bun/s3/s3-path-double-free.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/bun/s3/s3-path-double-free.test.ts (1)
49-58: 🧹 Nitpick | 🔵 TrivialSwitch
test.each()todescribe.each()in parameterized blocks.Line 49 and Line 101 still use
test.each(...). This should bedescribe.each(...)with nestedtest(...)per repo convention.🔧 Suggested refactor
- test.each(["exists", "size", "stat", "unlink"] as const)("instance %s() throwing before blob creation", method => { - const client = new Bun.S3Client(); - expect(() => - client[method](nonAsciiPath, { - get type() { - throw new Error("boom"); - }, - }), - ).toThrow("boom"); - }); + describe.each(["exists", "size", "stat", "unlink"] as const)("instance %s()", method => { + test("throwing before blob creation", () => { + const client = new Bun.S3Client(); + expect(() => + client[method](nonAsciiPath, { + get type() { + throw new Error("boom"); + }, + }), + ).toThrow("boom"); + }); + }); - test.each(["exists", "size", "stat", "unlink"] as const)("static %s() throwing before blob creation", method => { - expect(() => - Bun.S3Client[method](nonAsciiPath, { - get type() { - throw new Error("boom"); - }, - }), - ).toThrow("boom"); - }); + describe.each(["exists", "size", "stat", "unlink"] as const)("static %s()", method => { + test("throwing before blob creation", () => { + expect(() => + Bun.S3Client[method](nonAsciiPath, { + get type() { + throw new Error("boom"); + }, + }), + ).toThrow("boom"); + }); + });As per coding guidelines,
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}files should usedescribe.each()for parameterized tests.Also applies to: 101-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/bun/s3/s3-path-double-free.test.ts` around lines 49 - 58, Replace the top-level parameterized test.each(...) blocks with describe.each(...) blocks: wrap the array of methods (the current test.each([...]) that uses the parameter named method) in describe.each([...])("instance %s()", method => { and move the existing expect/assertion into a nested test("throwing before blob creation", () => { ... }) so each iteration creates a describe scope with a test inside; do the same for the other occurrence that currently uses test.each (the block that also iterates over method) so both parameterized sections use describe.each and nested test(...) per repo convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/js/bun/s3/s3-path-double-free.test.ts`:
- Around line 49-58: Replace the top-level parameterized test.each(...) blocks
with describe.each(...) blocks: wrap the array of methods (the current
test.each([...]) that uses the parameter named method) in
describe.each([...])("instance %s()", method => { and move the existing
expect/assertion into a nested test("throwing before blob creation", () => { ...
}) so each iteration creates a describe scope with a test inside; do the same
for the other occurrence that currently uses test.each (the block that also
iterates over method) so both parameterized sections use describe.each and
nested test(...) per repo convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5fdb4c5d-b5c2-45e0-b04c-151960a312e0
📒 Files selected for processing (1)
test/js/bun/s3/s3-path-double-free.test.ts
There was a problem hiding this comment.
I didn't find correctness issues, but this changes the ownership contract of constructS3FileWithS3Credentials* (now unconditionally consumes path) across ~13 call sites with subtle defer/errdefer interactions, and CI is showing build-zig/build-cpp failures on de5bcd3 — worth a human look before merging.
Extended reasoning...
Overview
This PR fixes an ASAN double-free in S3Client instance and static methods by changing constructS3FileWithS3Credentials and constructS3FileWithS3CredentialsAndOptions to unconditionally take ownership of the path argument (free on error, store in blob on success). Callers in S3Client.zig drop their errdefer path.deinit(), and static methods in S3File.zig neutralize path_or_blob to an empty sentinel before handing the path to the constructor so their existing errdefer becomes a no-op. A new test file exercises both before-blob and after-blob throw paths with a non-ASCII key (forces an allocated .encoded_slice).
Security risks
None. This is a memory-ownership fix on error paths; no auth, crypto, or input-handling surface is changed.
Level of scrutiny
Moderate-to-high. The change is small in line count but redefines an ownership contract used by ~13 call sites (7 instance methods, 6 static methods, plus Bun.file("s3://...") via constructBunFile and the S3File constructor). I traced each caller and the new semantics look correct: nextEat() cannot throw between path acquisition and ownership transfer, the switch capture |path| is by-value so reassigning path_or_blob doesn't invalidate it, and the empty-PathString sentinel makes the residual errdefer a safe no-op. However, this is exactly the class of Zig defer/errdefer reasoning that benefits from a second pair of eyes — four prior PRs (#28417, #28495, #28592, #29081) attempted the same fix, which suggests the area is subtle.
Other factors
- CI on de5bcd3 reports build-zig / build-cpp failures across most platforms. These may be infra-related (the header says "2 failures" but lists ~14), but should be confirmed green before merge.
- The latest commit 89aaad2 is an autofix; CI status for it isn't reflected yet.
- All inline review threads (CodeRabbit) are resolved.
- Test coverage is good and targets the exact ASAN repro.
|
CI on build 47626: all 60 jobs that ran passed, including |
| @@ -251,7 +245,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); | |||
There was a problem hiding this comment.
Doesn't this create a memory leak right here if it throws?
There was a problem hiding this comment.
No — constructS3FileWithS3CredentialsAndOptions now unconditionally takes ownership of path (see the other hunk in S3File.zig). Inside it:
var aws_options = S3.S3Credentials.getCredentialsWithOptions(...) catch |err| {
path.deinit();
return err;
};
defer aws_options.deinit();
const store = brk: { ... Blob.Store.initS3(path, ...) ... }; // store owns path from here
errdefer store.deinit(); // frees path on any later errorSo every error path inside the constructor frees path exactly once, and on success the returned blob owns it (freed by defer blob.detach()).
Between fromJS and the constructor call there is only args.nextEat(), which returns ?jsc.JSValue and cannot throw.
Keeping the old errdefer path.deinit() here is what caused the double free: when s3.unlink(blob.store.?, globalThis, options) (or getPresignUrlFrom in presign) threw after the blob existed, both defer blob.detach() (store → path) and errdefer path.deinit() fired on the same allocation.
When an S3Client method like presign() threw after constructing the blob
(e.g. missing credentials, invalid expiresIn, or a throwing option getter),
the path was freed twice: once by blob.detach() via the store, and again
by the caller's errdefer path.deinit(). This showed up as an ASAN
use-after-poison when the path was an allocated encoded_slice (non-ASCII
input).
constructS3FileWithS3Credentials{,AndOptions} now always take ownership of
path, freeing it if option parsing fails before the store is created.
Callers no longer keep an errdefer on a path whose ownership has been
transferred, and the static S3File helpers clear the captured path before
handing it off.
89aaad2 to
4f10b4c
Compare
|
Build 51239: 59 jobs passed (including 12
Buildkite queue is still backed up (recent builds have 8-32 scheduled jobs waiting); holding off on re-pushing until agents catch up. |
|
Superseded by #30495 (same fix against current main, minimal diff, deterministic test). |
2 participants
What does this PR do?
Fixes an ASAN use-after-poison (double free) in
S3Clientmethods when an operation throws after the internal blob has been constructed.Root cause
In methods like
S3Client.prototype.presign:constructS3FileWithS3CredentialsAndOptionsstorespathdirectly in the blob's store (no copy). IfgetPresignUrlFromthen throws (missing credentials,expiresIn: -1, a throwing option getter, etc.),defer blob.detach()frees the path via the store anderrdefer path.deinit()frees it again.This only manifests visibly when the
PathLikeis an allocated.encoded_slice, which happens for paths containing non-ASCII characters (UTF-16 → UTF-8 conversion allocates).The same pattern existed across
file/presign/exists/size/stat/write/unlinkon both the instance and the staticS3Client/S3Filecode paths.Fix
constructS3FileWithS3CredentialsandconstructS3FileWithS3CredentialsAndOptionsnow always take ownership ofpath: freed on error, stored in the returned blob on success.S3Clientinstance methods drop theerrdefer path.deinit()since ownership is unconditionally transferred.writeexplicitly freespathin the missing-data case before the constructor is reached.S3Filestatic methods clearpath_or_blobbefore handing the path to the constructor so theirerrdeferbecomes a no-op once ownership is gone.Repro
Before:
AddressSanitizer: use-after-poisoninPathLike.deinit.After: throws
expiresIn must be greather than 0cleanly.How did you verify your code works?
presignusing a non-ASCII path.test/js/bun/s3/s3-path-double-free.test.tscovering both the before-blob and after-blob error paths for instance and static methods.bun bd test test/js/bun/s3/s3-path-double-free.test.tspasses; the same test crashes under the previous ASAN build.