-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Fix double-deref of path when S3Client.presign throws #29279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| // When S3Client.presign() (and friends) throws after the temporary S3 | ||
| // blob has been constructed, the errdefer that cleans up the path used | ||
| // to double-deref the underlying WTFStringImpl (ownership had already | ||
| // transferred to the blob's store). That tripped a debug assertion / | ||
| // SIGFPE in release builds. | ||
| test("S3Client.presign with missing credentials throws instead of crashing", async () => { | ||
| const script = ` | ||
| let caught = 0; | ||
| try { Bun.S3Client.presign("foo"); } catch { caught++; } | ||
| try { Bun.S3Client.presign("foo", {}); } catch { caught++; } | ||
| try { Bun.s3.presign("foo"); } catch { caught++; } | ||
| try { new Bun.S3Client({}).presign("foo"); } catch { caught++; } | ||
| if (caught !== 4) throw new Error("expected all presign calls to throw, got " + caught); | ||
| console.log("ok"); | ||
| `; | ||
|
|
||
| // Make sure no ambient S3 credentials make the presign succeed. | ||
| const env: Record<string, string> = { ...bunEnv }; | ||
| for (const k of Object.keys(env)) { | ||
| if (k.startsWith("S3_") || k.startsWith("AWS_")) delete env[k]; | ||
| } | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", script], | ||
| env, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout.trim()).toBe("ok"); | ||
| expect(exitCode).toBe(0); | ||
| }); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 The
S3Client.file()instance method was missed by this PR's double-deref fix — it still usesconst pathwitherrdefer path.deinit()and callsconstructS3FileWithS3CredentialsAndOptions(), which can throw afterinitS3has already consumed the path's WTFStringImpl reference, triggering the same use-after-free crash. Apply the same fix as the sibling methods: changeconst pathtovar pathand addpath = .{ .string = bun.PathString.empty };after the successfulBlob.new(...)call.Extended reasoning...
What the bug is and how it manifests
The PR correctly identifies and fixes the double-deref pattern in six instance methods of
S3Client(presign,exists,size,stat,write,unlink) and six static functions inS3File.zig. However, thefile()instance method atS3Client.ziglines 128–142 was left unfixed. It still declaresconst path: jsc.Node.PathLikewitherrdefer path.deinit()in scope, and callsconstructS3FileWithS3CredentialsAndOptions()without clearingpathafter construction — the exact pattern this PR set out to eliminate.The specific code path that triggers it
constructS3FileWithS3CredentialsAndOptions()does the following in order:bun.handleOom(Blob.Store.initS3(path, ...))orinitS3WithReferencedCredentials(path, ...). Both functions copy thePathLikestruct by value and calltoThreadSafe()on the copy, which may callorig.deref()on the originalWTFStringImpl(when a new thread-safe impl is created). At this point the caller'spathis stale.errdefer store.deinit().try opts.getTruthyComptime(globalObject, "type")— can throw aJSError(e.g., if a getter throws).try file_type.toSlice(globalObject, ...)— also can throw aJSError.Why existing code doesn't prevent it
bun.handleOomcannot return an error, so step 1 always completes and ownership of the path reference transfers to the store before thetry-able operations in steps 3–4 run. The PR fixed all other methods by clearingpathafter step 1, but missedfile(). Becausepathis declaredconst, the clearing assignment cannot be written without first changing it tovar.What the impact would be
If step 3 or 4 throws a
JSError, the innererrdefer store.deinit()runs (decrementing and freeing the path ref held by the store), thenfile()'serrdefer path.deinit()runs on the already-freedWTFStringImpl— a use-after-free. In debug builds this tripshasAtLeastOneRef(); in release builds it is a SIGFPE or silent heap corruption, exactly as described in the PR.How to fix it
Same pattern as every other fixed method:
const path: jsc.Node.PathLike→var path: jsc.Node.PathLikeat line 132.path = .{ .string = bun.PathString.empty };immediately after the successfulBlob.new(...)call (beforereturn blob.toJS(globalThis)).Step-by-step proof
new Bun.S3Client({}).file(jsStringVar, { get type() { throw new Error('oops') } })wherejsStringVaris backed by a non-thread-safe JS string (a WTFStringImplwith refcount 1).file()parses the path →pathholds aslice_with_underlying_stringPathLike referencing thatStringImpl(refcount 2).constructS3FileWithS3CredentialsAndOptions()is called;getCredentialsWithOptionssucceeds.Blob.Store.initS3(path, ...)copiespath, callstoThreadSafe()on the copy → allocates a new thread-safeStringImpl, callsorig.deref()→StringImplrefcount drops to 1. The store now holds the new ref. The caller'spathpoints to the now-stale original.errdefer store.deinit()is armed insideconstructS3FileWithS3CredentialsAndOptions.opts.getTruthyComptime(globalObject, "type")invokes the JS getter → throws →JSErrorpropagates.errdefer store.deinit()fires →StringImplrefcount → 0 → freed.file()→errdefer path.deinit()fires → callsderef()on the already-freedStringImpl→ use-after-free / crash.