s3: don't double-free path when presign throws after blob creation by robobun · Pull Request #30467 · oven-sh/bun

robobun · 2026-05-10T23:25:59Z

Fuzzilli found a flaky SIGFPE in the REPRL loop. The minimal repro is:

try { Bun.s3.presign("X"); } catch (e) {}
Bun.gc(true);

What

Blob.Store.initS3 takes ownership of the PathLike it receives: toThreadSafe() transfers the underlying StringImpl ref for string-backed paths, and for .encoded_slice / .threadsafe_string the store ends up holding the only handle to the same allocation.

The S3Client / S3File entry points passed path by value and kept an errdefer path.deinit() active across the subsequent getPresignUrlFrom() / stat() / write() call. When that call threw (e.g. signRequest → error.MissingCredentials), the store's copy was torn down via blob.detach() and the caller's errdefer freed the same underlying path a second time.

The extra deref under-flows the StringImpl refcount. On the next collection StringImpl::costDuringGC() does divideRoundedUp(size, refCount()) with refCount() == 0 → SIGFPE. In debug builds it surfaces earlier as a reached unreachable code panic from the throw-scope assertion.

Fix

Thread *PathLike through the constructS3File* helpers and neuter the caller's copy (path.* = .{ .string = bun.PathString.empty }) once the store has taken ownership. The outer errdefer path.deinit() now:

still cleans up if getCredentialsWithOptions throws before the store is built, and
is a no-op on every post-construction error path, because the PathLike has been emptied.

This is the same neuter-after-transfer pattern findOrCreateFileFromPath already uses at Blob.zig:2119-2121.

Test

test/js/bun/s3/s3-presign-missing-credentials.test.ts spawns a subprocess with all AWS/S3 env vars cleared, exercises Bun.s3.presign, new S3Client().presign, S3Client.presign, and Bun.file("s3://…").presign(), then Bun.gc(true). On current main the subprocess aborts after the first call; with this patch it prints ERR_S3_MISSING_CREDENTIALS ×4 and exits 0.

Supersedes #28423 with a narrower change that fixes the ownership bug instead of avoiding the blob construction.

Verification (robobun, build #53198 complete): All error-level failures are pre-existing on other PRs and unrelated to S3 path ownership:

test-http-should-emit-close-when-connection-is-aborted.ts (Windows ×3) — fails on 10/10 recent PR builds (#53072–53194).
s3-storage-class.test.ts "writer + options on big file" S3Error: UnknownError (macOS 14 aarch64) — pre-existing S3 upload flake in 10/11 recent PR builds; this test exercises multipart upload, not the presign/construct path touched here. Same test passed on retry on macOS 14 x64 in this build.
node-http-backpressure-max.test.ts timeout (macOS 14 x64) — HTTP test, also seen in #53107.
2× alpine jobs expired (infra).

The new s3-presign-missing-credentials.test.ts passed on every platform that ran tests. Not re-rolling since the HTTP-close failure is 10/10 deterministic across PRs.

Blob.Store.initS3 takes ownership of the PathLike it receives: toThreadSafe() transfers the underlying StringImpl ref for string-backed paths, and for the other variants the store ends up holding the only handle to the same allocation. The S3Client / S3File callers passed path by value and kept an 'errdefer path.deinit()' active across the subsequent presign()/stat()/write() call, so when signRequest failed (e.g. missing credentials) the store was torn down via blob.detach() and then the caller's errdefer freed the same path a second time. The extra deref under-flowed the StringImpl refcount, which later shows up as a SIGFPE in StringImpl::costDuringGC (division by refCount()==0) or as a 'reached unreachable code' panic in debug builds. Thread *PathLike through the constructS3File* helpers and neuter the caller's copy once the store has taken ownership, so the outer errdefer becomes a no-op on every post-construction error path while still cleaning up if getCredentialsWithOptions throws before the store is built.

robobun · 2026-05-10T23:26:09Z

^{Updated 6:11 PM PT - May 10th, 2026}

❌ @robobun, your commit 6319bc1 has 3 failures in Build #53198 (All Failures):

🧪 To try this PR locally:

bunx bun-pr 30467

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

bun-30467 --bun

coderabbitai · 2026-05-10T23:30:35Z

Walkthrough

Refactor: pass S3 PathLike by pointer through S3File constructors so Blob.Store can take ownership and caller PathLike copies are neutered; update all S3Client and JS call sites accordingly and add a regression test verifying missing-credentials error handling.

Changes

S3 PathLike Ownership Refactoring

Layer / File(s)	Summary
Constructor API signatures `src/runtime/webcore/S3File.zig`	`constructInternalJS`, `constructS3FileWithS3CredentialsAndOptions`, `constructS3FileWithS3Credentials`, and `constructS3FileInternal` change `path` parameter from value to `*jsc.Node.PathLike`.
Ownership transfer and cleanup `src/runtime/webcore/S3File.zig`	Constructors initialize the S3 store from `path.`, then neuter the caller's `PathLike` (`path. = empty`) after ownership transfer to avoid double-free on error paths.
JS operations refactored `src/runtime/webcore/S3File.zig`	Operations `presign`, `unlink`, `write`, `size`, `exists`, `stat` destructure `.path` as pointer (`
S3Client method updates `src/runtime/webcore/S3Client.zig`	Methods `file`, `presign`, `exists`, `size`, `stat`, `write`, `unlink`, `listObjects`, `staticFile`, `staticListObjects` create mutable `path` variables and pass `&path` to updated constructors; some call sites add `errdefer path.deinit()` and list helpers use an `empty_path`.
Blob.zig S3 fast path `src/runtime/webcore/Blob.zig`	The `s3://` branch in `constructBunFile` now calls `S3File.constructInternalJS` with `&path.path` and adds `errdefer path.deinit()` for error cleanup.
Missing credentials test `test/js/bun/s3/s3-presign-missing-credentials.test.ts`	Regression test spawns a child process with cleared AWS/S3 environment, exercises multiple presign call sites, forces GC, and verifies printed error codes and successful exit.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: addressing a double-free bug in S3 path handling when presign operations fail after blob creation.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The PR description is comprehensive and directly addresses the problem, root cause, fix, and test validation with clear technical detail.

_{✏️ 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.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/webcore/S3Client.zig`:
- Around line 157-159: Temporary S3File blobs created via
S3File.constructS3FileWithS3CredentialsAndOptions (and the other
constructS3FileWithS3Credentials* variants) are leaking heap allocations because
blob.detach() only drops the store ref; replace those detach() calls with
blob.deinit() for each temporary blob (e.g., the blob used before calling
S3File.getPresignUrlFrom and the other occurrences listed) so the heap-owned
fields (like content_type) are released; keep the same usage pattern (deinit
after the call or via defer) to preserve async stat/exists/size behavior since
tasks take their own store ref.

🪄 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: 0f41ba7f-24f0-4f1f-966f-6b7d4ddcb287

📥 Commits

Reviewing files that changed from the base of the PR and between 03ebdf8 and ffe2a78.

📒 Files selected for processing (4)

src/runtime/webcore/Blob.zig
src/runtime/webcore/S3Client.zig
src/runtime/webcore/S3File.zig
test/js/bun/s3/s3-presign-missing-credentials.test.ts

github-actions · 2026-05-10T23:32:19Z

This PR may be a duplicate of:

Fix crash in S3 presign with missing credentials #28423 - Fix crash in S3 presign with missing credentials (explicitly superseded by this PR)
S3Client: don't double-deref path when a method fails after blob construction #30419 - Fix S3Client double-deref of path when a method fails after blob construction
s3: fix path double-free when presign throws after blob creation #30351 - Fix S3 path double-free when presign throws after blob creation
fix(s3): double free of path when S3Client operation throws #29656 - Fix S3 double-free of path when S3Client operation throws
fix(s3): don't double-free path when S3Client static ops throw after blob creation #29081 - Fix S3 path double-free when S3Client static ops throw after blob creation
Fix double-free in S3 static methods when path is passed as string #28592 - Fix S3 double-free in static methods when path is passed as string
Fix double-free of path in S3 static methods on error paths #28495 - Fix S3 double-free of path in static methods on error paths
Fix use-after-free in S3 Store.initS3 PathLike refcounting #28417 - Fix use-after-free in S3 Store.initS3 PathLike refcounting

🤖 Generated with Claude Code

args.deinit() already unprotects .buffer paths via the protectEat bitset; deinitAndUnprotect() here would unprotect a second time on the pre-initS3 error path. deinit() still frees the string-backed variants and matches the other S3 call sites.

robobun · 2026-05-11T13:35:56Z

Superseded by #30495 (same fix against current main, minimal diff, deterministic test).

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

coderabbitai Bot reviewed May 10, 2026

View reviewed changes

Comment thread src/runtime/webcore/S3Client.zig

claude Bot reviewed May 10, 2026

View reviewed changes

Comment thread src/runtime/webcore/Blob.zig Outdated

github-actions Bot mentioned this pull request May 11, 2026

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

Closed

robobun closed this May 11, 2026

robobun deleted the farm/d3e5cd36/fix-s3-path-double-deref branch May 11, 2026 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

s3: don't double-free path when presign throws after blob creation#30467

s3: don't double-free path when presign throws after blob creation#30467
robobun wants to merge 2 commits into
mainfrom
farm/d3e5cd36/fix-s3-path-double-deref

robobun commented May 10, 2026 •

edited

Loading

Uh oh!

robobun commented May 10, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 10, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented May 10, 2026

Uh oh!

Uh oh!

robobun commented May 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

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

Uh oh!

What

Fix

Test

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented May 10, 2026

Uh oh!

Uh oh!

robobun commented May 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented May 10, 2026 •

edited

Loading

robobun commented May 10, 2026 •

edited

Loading

coderabbitai Bot commented May 10, 2026 •

edited

Loading