Fix double-free of path in S3Client static methods on error by robobun · Pull Request #29110 · oven-sh/bun

robobun · 2026-04-10T09:18:39Z

What does this PR do?

Fixes a crash (debug assert / double-free) in Bun.S3Client static methods (presign, unlink, write, size, exists, stat) when called with a string path and the subsequent operation throws.

// no credentials configured
Bun.S3Client.presign("some-path", 123);

panic(main thread): reached unreachable code
...
#5  bun.assert (ok=false)
#6  string.wtf.WTFStringImplStruct.deref
#7  string.String.deref
#8  string.SliceWithUnderlyingString.deinit
#9  bun.js.node.types.PathLike.deinit
#10 bun.js.node.types.PathOrFileDescriptor.deinit
#11 bun.js.webcore.S3File.presign (errdefer)

How did you verify your code works?

Minimal repro no longer crashes under the ASAN debug build; it now throws ERR_S3_MISSING_CREDENTIALS as expected.
Original Fuzzilli crash script runs cleanly.
Added test/js/bun/s3/s3-static-path-error.test.ts.

Root cause

constructS3FileInternalStore stores the PathLike directly in the blob's S3 store without adding a ref (despite the "this actually protects/refs the pathlike" comment, PathLike.toThreadSafe does not add a ref in the common case). So once the blob is constructed, it owns the path.

If the subsequent call (e.g. getPresignUrlFrom) throws:

defer blob.deinit() runs → store deinit → pathlike.deinit() → deref (refcount → 0)
outer errdefer path_or_blob.path.deinit() runs → deref again → bun.assert(self.hasAtLeastOneRef()) fails

Fix: after constructS3FileInternalStore succeeds, reset path_or_blob to an empty PathString so the errdefer becomes a no-op.

Fuzzilli fingerprint: 641d23ceb14c14b2

When S3Client static methods (presign, unlink, write, size, exists, stat) are called with a string path, constructS3FileInternalStore takes ownership of the path and stores it in the blob. If the subsequent operation throws, defer blob.deinit() releases the path, then the outer errdefer path_or_blob.path.deinit() releases it again, tripping the refcount assert in WTFStringImpl.deref. Clear path_or_blob after ownership transfers so the errdefer becomes a no-op.

robobun · 2026-04-10T09:18:52Z

^{Updated 3:10 AM PT - Apr 10th, 2026}

❌ @autofix-ci[bot], your commit bd99738 has 2 failures in Build #44859 (All Failures):

test/js/bun/util/v8-heap-snapshot.test.ts - SIGKILL on 🐧 25.04 x64-baseline
test/js/bun/webview/webview-chrome.test.ts - code 1 on 🍎 14 aarch64
test/js/bun/webview/webview-chrome.test.ts - code 1 on 🍎 13 aarch64

🧪 To try this PR locally:

bunx bun-pr 29110

That installs a local version of the PR into your bun-29110 executable, so you can run:

bun-29110 --bun

coderabbitai · 2026-04-10T09:20:38Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ddbbe084-cc3a-4012-8b62-f33efe81f0d6

📥 Commits

Reviewing files that changed from the base of the PR and between 8e65e47 and bd99738.

📒 Files selected for processing (2)

src/bun.js/webcore/S3File.zig
test/js/bun/s3/s3-static-path-error.test.ts

Walkthrough

This change modifies S3File.zig to explicitly reassign path_or_blob to an empty .path representation after constructing S3-backed blobs in several methods. A new test verifies that Bun.S3Client.presign with a static path argument properly throws an ERR_S3_MISSING_CREDENTIALS error when credentials are unavailable.

Changes

Cohort / File(s)	Summary
S3File Implementation `src/bun.js/webcore/S3File.zig`	Explicitly reassigns `path_or_blob` to empty `.path` after blob construction in `presign`, `unlink`, `write` (path upload), `size`, `exists`, and `stat` methods, affecting post-switch cleanup behavior.
S3 Error Handling Test `test/js/bun/s3/s3-static-path-error.test.ts`	New test verifying that `Bun.S3Client.presign` with static path argument throws `ERR_S3_MISSING_CREDENTIALS` error when AWS credentials are absent, including GC invocation and clean process exit validation.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically describes the main change: fixing a double-free bug in S3Client static methods when errors occur.
Description check	✅ Passed	The description follows the template with both required sections completed: it clearly explains what the PR does and provides comprehensive verification details.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-04-10T09:23:17Z

This PR may be a duplicate of:

fix(s3): don't double-free path when S3Client static ops throw after blob creation #29081 - Same fix: resets path_or_blob after constructS3FileInternalStore in the same 6 S3File.zig static methods to prevent double-free on error
Fix double-free in S3 static methods when path is passed as string #28592 - Same double-free fix at the same 6 sites in S3File.zig (using a slightly different reset idiom), also extends to S3Client.zig instance methods
Fix double-free of path in S3 static methods on error paths #28495 - Same double-free fix at the same 6 sites in S3File.zig, also extends to S3Client.zig instance methods and helper functions

🤖 Generated with Claude Code

robobun · 2026-04-10T09:24:01Z

Duplicate of #29081 (also #28592, #28495) — same fix at the same 6 sites. Closing in favor of the earlier PR.

claude

LGTM — clean targeted fix for the double-free with good test coverage.

Extended reasoning...

Overview

This PR fixes a memory safety crash in 6 S3Client static methods (, , , , , ). Each function has an guard, but after succeeds the blob's S3 store takes ownership of the same . If the subsequent operation throws, both the blob's and the outer run, causing a double-free / debug assert failure.

Fix correctness

The fix — resetting to immediately after the store is constructed — neutralizes the without touching the blob or its store. carries no allocation, so on it is a no-op. This is a minimal, idiomatic pattern in Zig for transferring ownership out from under an errdefer.

Security risks

No new security surface. The change only affects error-path cleanup; the happy path and all credential/auth logic are untouched.

Level of scrutiny

Low-to-medium. The change is a 1-line addition per function, mechanically applied to 6 call sites. The root cause is clearly documented and the fix is straightforward. A new test subprocess validates the error-path behavior without requiring live S3 credentials.

Other factors

Pre-existing minor issue (wrong error message "to get size" vs "to get stat" in the function) is flagged as an inline comment; it does not affect correctness.
No CODEOWNER-flagged paths are touched.
CI build was triggered at the time of review.

claude · 2026-04-10T09:30:30Z

                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);


🟣 The stat() function contains two error messages copy-pasted from size() that say "Expected a S3 or path to get size" instead of "Expected a S3 or path to get stat". This is a pre-existing issue, but since this PR directly modifies the stat() function, it's a good opportunity to fix these misleading messages.

Extended reasoning...

What the bug is: In the stat() function in src/bun.js/webcore/S3File.zig, both input-validation error messages were copy-pasted from the size() function and still read "Expected a S3 or path to get size" instead of "to get stat".

Specific code paths: There are two affected locations in the stat() function:

The blob-type guard check near line 572: return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); — triggered when the caller passes a non-S3 blob.

The .fd check inside the .path branch near line 579: return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); — triggered when the caller passes a file descriptor instead of a path.

Why existing code doesn't prevent it: Both messages are valid Zig string literals that compile without error; the incorrect wording is invisible to the compiler. The size() function at lines ~186 and ~193 has the same two-guard pattern with the correct "to get size" wording, and those strings were simply cloned when stat() was written without updating the verb.

Impact: Any user who calls S3Client.stat(nonS3Blob) or S3Client.stat(fd) receives the error message "Expected a S3 or path to get size", which is confusing because they called stat(), not size(). This makes debugging harder but does not affect correctness — the error is still thrown and the function still rejects invalid input.

How to fix it: Change both occurrences in stat() from "Expected a S3 or path to get size" to "Expected a S3 or path to get stat".

Step-by-step proof:

A user calls Bun.S3Client.stat(new Blob(["hello"])) (a plain Blob, not an S3Blob).

PathOrBlob.fromJSNoCopy returns .blob since it is a Blob.

The guard if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data \!= .s3)) is true.

The function throws with message "Expected a S3 or path to get size".

The user is confused — they called stat(), not size(), and the error says "size".

github-actions Bot added the claude label Apr 10, 2026

[autofix.ci] apply automated fixes

bd99738

robobun closed this Apr 10, 2026

robobun deleted the farm/cb846d37/fix-s3-static-path-double-free branch April 10, 2026 09:24

claude Bot reviewed Apr 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix double-free of path in S3Client static methods on error#29110

Fix double-free of path in S3Client static methods on error#29110
robobun wants to merge 2 commits into
mainfrom
farm/cb846d37/fix-s3-static-path-double-free

robobun commented Apr 10, 2026

Uh oh!

robobun commented Apr 10, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Apr 10, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 10, 2026

Uh oh!

robobun commented Apr 10, 2026

Uh oh!

claude Bot left a comment

Uh oh!

claude Bot Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

robobun commented Apr 10, 2026

What does this PR do?

How did you verify your code works?

Root cause

Uh oh!

robobun commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

github-actions Bot commented Apr 10, 2026

Uh oh!

robobun commented Apr 10, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Fix correctness

Security risks

Level of scrutiny

Other factors

Uh oh!

claude Bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Apr 10, 2026 •

edited

Loading

coderabbitai Bot commented Apr 10, 2026 •

edited

Loading