s3: fix path double-free when presign throws after blob creation#30351
Closed
robobun wants to merge 4 commits into
Closed
s3: fix path double-free when presign throws after blob creation#30351robobun wants to merge 4 commits into
robobun wants to merge 4 commits into
Conversation
Blob.Store.initS3 takes ownership of the PathLike, so once the temporary blob is constructed the caller's errdefer on the path must become a no-op. Otherwise a synchronous throw from the subsequent operation (e.g. signRequest failing with missing credentials, or an invalid expiresIn) triggers both defer blob.deinit() and errdefer path.deinit(), underflowing the string refcount and asserting in debug builds. Neutralize the caller's path after ownership transfer in all the S3Client static/instance helpers that follow this pattern.
Collaborator
Author
|
Updated 4:35 AM PT - May 7th, 2026
❌ @robobun, your commit a5092c4 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30351That installs a local version of the PR into your bun-30351 --bun |
Contributor
WalkthroughThis PR refactors S3 path ownership semantics to prevent double-free errors during error handling. S3Client methods now explicitly acquire paths from JavaScript, transfer ownership to blob objects, and clear the local path variable. S3File implements matching logic to neutralize ChangesS3 Path Ownership Lifecycle Refactor
🚥 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}
Contributor
There was a problem hiding this comment.
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/S3File.zig`:
- Around line 86-87: The transferred path must be neutralized inside the
constructor helpers immediately after Blob.Store.initS3* succeeds to avoid
double-deinit on later fallible work; update constructS3FileInternalStore and
all constructS3FileWithS3Credentials* variants so that right after calling
Blob.Store.initS3* (and upon its success) the helper clears/neutralizes the
original path (the same operation currently done by assigning
bun.PathString.empty to path_or_blob.path) before performing any further
fallible operations (e.g., reading options.type), and remove the external
reassignment in the caller so only the constructor owns the responsibility for
clearing the transferred path.
🪄 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: cec82f97-97bd-4d48-a9bf-7bc90de89185
📒 Files selected for processing (3)
src/runtime/webcore/S3Client.zigsrc/runtime/webcore/S3File.zigtest/js/bun/s3/s3-path-double-free.test.ts
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
The constructors do fallible JS work (reading options.type) after Blob.Store.initS3 has already taken ownership of the path. If that work throws, errdefer store.deinit() releases the path and the caller's errdefer releases it again. Pass the path by pointer and clear it immediately after initS3 succeeds so any caller errdefer becomes a no-op once ownership has transferred, covering both post-construct errors and errors inside the constructor itself. Also add the previously-missing errdefer on the path in constructInternal, staticFile, and the Bun.file s3:// branch.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The previous test crashed the test runner process directly, so the junit reporter saw zero tests instead of a failure. Spawn each case in a child process and fail the test on non-zero exit / signal so the regression is recorded as a proper test failure.
Contributor
There was a problem hiding this comment.
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/Blob.zig`:
- Around line 2072-2073: The early-return branch for s3:// currently uses
errdefer path.deinit() and calls S3File.constructInternalJS(&path.path, ...)
which only takes a PathLike and cannot release the JS protection on the outer
PathOrFileDescriptor; change the cleanup so that deinitAndUnprotect() is invoked
on the PathOrFileDescriptor (not just path.deinit()) on both success and error
paths before returning or propagating errors from S3File.constructInternalJS;
ensure you call PathOrFileDescriptor.deinitAndUnprotect() (or the equivalent
method) instead of path.deinit() and maintain the errdefer semantics so the
wrapped JS path is unprotected on all code paths.
🪄 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: d60c5688-8a45-437e-8fed-d1c5b5653521
📒 Files selected for processing (4)
src/runtime/webcore/Blob.zigsrc/runtime/webcore/S3Client.zigsrc/runtime/webcore/S3File.zigtest/js/bun/s3/s3-path-double-free.test.ts
Contributor
There was a problem hiding this comment.
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 `@test/js/bun/s3/s3-path-double-free.test.ts`:
- Around line 12-25: The test's run function redundantly overrides the
BUN_DEBUG_QUIET_LOGS env var even though bunEnv already contains it; update the
Bun.spawnSync call inside run to stop setting BUN_DEBUG_QUIET_LOGS explicitly
(replace env: { ...bunEnv, BUN_DEBUG_QUIET_LOGS: "1" } with simply bunEnv or {
...bunEnv }), leaving the rest of the spawnSync options untouched.
🪄 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: d4c333da-d394-4e90-a703-38cf704ac22b
📒 Files selected for processing (1)
test/js/bun/s3/s3-path-double-free.test.ts
Comment on lines
+12
to
+25
| function run(body: string) { | ||
| const { exitCode, signalCode, stderr } = Bun.spawnSync({ | ||
| cmd: [bunExe(), "-e", body], | ||
| env: { ...bunEnv, BUN_DEBUG_QUIET_LOGS: "1" }, | ||
| stderr: "pipe", | ||
| stdout: "ignore", | ||
| timeout: 30_000, | ||
| }); | ||
| expect({ stderr: stderr.toString(), exitCode, signalCode }).toEqual({ | ||
| stderr: "", | ||
| exitCode: 0, | ||
| signalCode: undefined, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm bunEnv already sets BUN_DEBUG_QUIET_LOGS
rg -n "BUN_DEBUG_QUIET_LOGS" --type tsRepository: oven-sh/bun
Length of output: 5953
Remove redundant BUN_DEBUG_QUIET_LOGS override
bunEnv from the harness module already includes BUN_DEBUG_QUIET_LOGS: "1", so the explicit override can be removed:
Diff
- env: { ...bunEnv, BUN_DEBUG_QUIET_LOGS: "1" },
+ env: bunEnv,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function run(body: string) { | |
| const { exitCode, signalCode, stderr } = Bun.spawnSync({ | |
| cmd: [bunExe(), "-e", body], | |
| env: { ...bunEnv, BUN_DEBUG_QUIET_LOGS: "1" }, | |
| stderr: "pipe", | |
| stdout: "ignore", | |
| timeout: 30_000, | |
| }); | |
| expect({ stderr: stderr.toString(), exitCode, signalCode }).toEqual({ | |
| stderr: "", | |
| exitCode: 0, | |
| signalCode: undefined, | |
| }); | |
| } | |
| function run(body: string) { | |
| const { exitCode, signalCode, stderr } = Bun.spawnSync({ | |
| cmd: [bunExe(), "-e", body], | |
| env: bunEnv, | |
| stderr: "pipe", | |
| stdout: "ignore", | |
| timeout: 30_000, | |
| }); | |
| expect({ stderr: stderr.toString(), exitCode, signalCode }).toEqual({ | |
| stderr: "", | |
| exitCode: 0, | |
| signalCode: undefined, | |
| }); | |
| } |
🤖 Prompt for 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.
In `@test/js/bun/s3/s3-path-double-free.test.ts` around lines 12 - 25, The test's
run function redundantly overrides the BUN_DEBUG_QUIET_LOGS env var even though
bunEnv already contains it; update the Bun.spawnSync call inside run to stop
setting BUN_DEBUG_QUIET_LOGS explicitly (replace env: { ...bunEnv,
BUN_DEBUG_QUIET_LOGS: "1" } with simply bunEnv or { ...bunEnv }), leaving the
rest of the spawnSync options untouched.
This was referenced May 9, 2026
Collaborator
Author
|
Superseded by #30495 (same fix against current main, minimal diff, deterministic test). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
S3Client.presign(both static and instance) crashed in debug builds with areached unreachable codepanic (string refcount assertion) when the presign operation threw synchronously — for example with missing credentials or an invalidexpiresIn.Why
Blob.Store.initS3takes ownership of thePathLikeit receives. AfterconstructS3FileInternalStore/constructS3FileWithS3CredentialsAndOptionssucceed, the temporary blob owns the path anddefer blob.deinit()will release it.But the caller still has an
errdefer path.deinit()in scope. When a later call (getPresignUrlFrom→signRequest) returnserror.JSError, both thedefer blob.deinit()and theerrdefer path.deinit()fire, deref'ing the sameWTFStringImpltwice and trippingbun.assert(self.hasAtLeastOneRef())inwtf.zig.How
After the blob takes ownership of the path, neutralize the caller's path variable to an empty
PathLike.stringso theerrdeferbecomes a no-op. This matches the existing pattern inBlob.findOrCreateFileFromPath.Applied to all S3 helpers that share this
errdefer path.deinit()+defer blob.deinit()shape (presign,unlink,write,size,exists,stat— both theS3Filestatics and theS3Clientinstance methods), since they all have the same latent double-free on any synchronous error after blob construction.Fuzzer fingerprint:
5cc454d012677426