Skip to content

S3Client: don't double-free path on error after store takes ownership - #30011

Closed
robobun wants to merge 2 commits into
mainfrom
farm/b9b32ef7/s3-path-double-free
Closed

S3Client: don't double-free path on error after store takes ownership#30011
robobun wants to merge 2 commits into
mainfrom
farm/b9b32ef7/s3-path-double-free

Conversation

@robobun

@robobun robobun commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a debug-assertion crash / use-after-free in S3Client and S3File methods when a JS exception is thrown after the internal S3 blob store has been created.

Found by Fuzzilli (fingerprint f824199979cdeedf).

Root cause

Blob.Store.initS3*() takes ownership of the PathLike it is given: for a .slice_with_underlying_string it calls toThreadSafe(), which — when the underlying WTFStringImpl is not already thread-safe (not an atom) — creates an isolated copy and derefs the original. The caller's (shallow) copy of the PathLike is left pointing at an impl whose ref was just consumed.

All S3Client / static S3File entry points had errdefer path.deinit() that continued to fire on any error after the blob store had already taken ownership (e.g. the data argument's toPrimitive throwing inside Blob.writeFileInternal, or an options type getter throwing). That deref'd the same WTFStringImpl a second time and tripped hasAtLeastOneRef() in debug builds.

The misleading crash backtrace (string.String.fromJS at string.zig:543) is what Bun's panic handler printed, but the actual trap was in WTFStringImpl.deref called from the errdefer during unwind:

string.String.deref
string.SliceWithUnderlyingString.deinit
bun.js.node.types.PathLike.deinit
bun.js.webcore.S3Client.S3Client.write  (errdefer path.deinit())

Fix

Make constructS3FileWithS3Credentials / constructS3FileWithS3CredentialsAndOptions unconditionally own path: if getCredentialsWithOptions throws before the store is created, they now release path themselves. Callers drop their errdefer path.deinit() (with explicit cleanup only for error paths before the constructor is reached, e.g. missing data argument in write).

How did you verify your code works?

Deterministic repro that crashed before and passes after:

const obj = { [Symbol.toPrimitive]() { throw new Error("boom"); } };
new Bun.S3Client().write("some-fresh-path-string", obj);

Added test/js/bun/s3/s3-path-error-double-free.test.ts covering instance + static write/presign/exists/size/stat/unlink/file with exceptions thrown both before and after the store is created.

@robobun
S3Client: don't double-free path on error after store takes ownership
695a705 
Blob.Store.initS3*() takes ownership of the PathLike it is given: for a
.slice_with_underlying_string it calls toThreadSafe(), which for a
non-atomic StringImpl creates an isolated copy and derefs the original.
The caller's (shallow) copy of the PathLike is left pointing at an impl
whose ref was just consumed.

S3Client/S3File methods all had `errdefer path.deinit()` that would run
after the blob store had already taken ownership, deref'ing the same
WTFStringImpl a second time and tripping the hasAtLeastOneRef()
assertion in debug builds (use-after-free in release).

Make constructS3FileWithS3Credentials[AndOptions] unconditionally own
`path` (releasing it if getCredentialsWithOptions throws before the
store is created) and drop the errdefer from callers.
@robobun

robobun commented Apr 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 11:59 AM PT - Apr 30th, 2026

❌ @autofix-ci[bot], your commit e75fb77 has 1 failures in Build #49462 (All Failures):

🧪   To try this PR locally:

bunx bun-pr 30011

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

bun-30011 --bun

@github-actions

github-actions Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. fix(s3): double free of path when S3Client operation throws #29656 - Fixes the same S3 path double-free by removing errdefer path.deinit() from S3Client.zig callers and adding cleanup in the constructor error paths
  2. fix(s3): don't double-free path when S3Client static ops throw after blob creation #29081 - Fixes the same double-free by nullifying path_or_blob after constructS3FileInternalStore succeeds in S3File.zig static methods
  3. Fix double-free in S3 static methods when path is passed as string #28592 - Fixes the same double-free by reassigning path_or_blob to .blob after store creation to prevent deferred cleanup from double-freeing
  4. Fix double-free of path in S3 static methods on error paths #28495 - Fixes the same double-free by nullifying path/path_or_blob after ownership transfer, keeping errdefer but making it a no-op
  5. Fix use-after-free in S3 Store.initS3 PathLike refcounting #28417 - Fixes the same use-after-free by moving ownership into the constructor functions with explicit cleanup, closest approach to this PR

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Apr 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Removes automatic errdefer-based path cleanup from several S3 methods, replaces it with targeted explicit path.deinit() calls only on specific error paths, switches credential acquisition in constructors from try to catch with explicit cleanup, and adds a test suite for path ownership on errors.

Changes

Cohort / File(s) Summary
S3Client implementation
src/bun.js/webcore/S3Client.zig		 Removed errdefer path.deinit() cleanup across methods (file, presign, exists, size, stat, unlink, write); only specific error paths now explicitly call path.deinit() (notably the missing-data case in write).
S3File implementation
src/bun.js/webcore/S3File.zig		 Removed errdefer-based path cleanup in several methods and changed credential acquisition from try to catch, explicitly deinitializing path on credential acquisition failure; retained existing AWS options and store cleanup logic.
Tests
test/js/bun/s3/s3-path-error-double-free.test.ts		 Adds tests that provoke throwing during path/string coercion and missing-argument cases to assert S3 methods (instance and static) fail as expected and do not double-free path resources.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main fix: preventing double-free of path on error after store ownership transfer.
Description check ✅ Passed The description comprehensively covers both required sections: detailed explanation of what the PR does (root cause, fix, and impact) and thorough verification through deterministic repro and new test suite.
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.

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

Review rate limit: 4/5 reviews remaining, refill in 12 minutes.

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

@robobun

robobun commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #29656 (and #28417, #28495, #28592, #29081) — same S3 path double-free fix.

@robobun robobun closed this Apr 30, 2026
@robobun
robobun deleted the farm/b9b32ef7/s3-path-double-free branch April 30, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

claude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@robobun