Fix double-free of path string on S3 error paths #30437
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,62 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
|
|
||
| // When an S3 operation fails after the blob store is created (e.g. missing | ||
| // credentials in signRequest), the path string's ownership has already been | ||
| // transferred to the store via toThreadSafe(). The caller's errdefer must not | ||
| // deinit it again or the underlying StringImpl gets double-freed. | ||
| describe("S3 methods do not double-free path on error", () => { | ||
| const paths = ["abc", "foo/bar", "xyz" + Math.random().toString(36).slice(2)]; | ||
|
|
||
| describe.each(paths)("path=%s", p => { | ||
| test("S3Client instance presign", () => { | ||
| expect(() => Bun.s3.presign(p)).toThrow(); | ||
| expect(() => Bun.s3.presign(p, new SharedArrayBuffer(16))).toThrow(); | ||
| expect(() => new Bun.S3Client({}).presign(p)).toThrow(); | ||
| }); | ||
|
|
||
| test("S3Client static presign", () => { | ||
| expect(() => Bun.S3Client.presign(p)).toThrow(); | ||
| }); | ||
|
|
||
| test("S3Client unlink", () => { | ||
| const a = Bun.s3.unlink(p); | ||
| const b = Bun.S3Client.unlink(p); | ||
| expect(a).rejects.toThrow(); | ||
| expect(b).rejects.toThrow(); | ||
| }); | ||
|
|
||
| test("S3Client write", () => { | ||
| const a = Bun.s3.write(p, "x"); | ||
| const b = Bun.S3Client.write(p, "x"); | ||
| expect(a).rejects.toThrow(); | ||
| expect(b).rejects.toThrow(); | ||
| }); | ||
| }); | ||
|
|
||
| test("valid presign still works", () => { | ||
| const client = new Bun.S3Client({ | ||
| accessKeyId: "a", | ||
| secretAccessKey: "b", | ||
| bucket: "c", | ||
| endpoint: "http://localhost", | ||
| }); | ||
| expect(client.presign("abc")).toStartWith("http://localhost/c/abc?"); | ||
| }); | ||
|
|
||
| test("early failure before store creation still cleans up path", () => { | ||
| expect(() => Bun.s3.presign("abc", { accessKeyId: 123 })).toThrow(); | ||
| expect(() => new Bun.S3Client({}).presign("abc", { accessKeyId: 123 })).toThrow(); | ||
| }); | ||
|
|
||
| test("repeated calls under GC", () => { | ||
| for (let i = 0; i < 50; i++) { | ||
| try { | ||
| Bun.s3.presign("k" + i); | ||
| } catch {} | ||
| try { | ||
| Bun.S3Client.presign("k" + i); | ||
| } catch {} | ||
| } | ||
| Bun.gc(true); | ||
| }); | ||
| }); |
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
path_consumedflag is set afterconstructS3FileWithS3CredentialsAndOptionsreturns, but ownership ofpathactually transfers inside that function at theBlob.Store.initS3*call — and there is still fallible code after it (try opts.getTruthyComptime(globalObject, "type")/try file_type.toSlice(...)). If that post-initS3read throws,errdefer store.deinit()frees the store, the error propagates withpath_consumedstillfalse, and the caller'serrdeferderefs the already-released originalWTFStringImpl— the same double-free this PR is fixing. Consider passingpathby pointer and clearing it right afterinitS3, or hoisting the"type"handling before store creation; this applies to every call site touched in S3Client.zig and S3File.zig.Extended reasoning...
What the bug is
This PR's fix moves the ownership boundary to "after
constructS3FileWithS3CredentialsAndOptions/constructS3FileInternalStorereturns successfully". But the real ownership-transfer point is inside those functions, at theBlob.Store.initS3/initS3WithReferencedCredentialscall. Between that transfer point and the function's successful return, there is still fallible JS-visible code. If that code throws, we end up in exactly the state the PR set out to fix: the store has consumed the caller's path refs, yet the caller'serrdefer if (!path_consumed) path.deinit()still fires.The specific code path
In
constructS3FileWithS3CredentialsAndOptions(and the near-identicalconstructS3FileWithS3Credentials):Blob.Store.initS3*is called (Store.zig:68-71 / 97-100). It takespathlikeby value, copies it locally, and callspath.toThreadSafe(). For.slice_with_underlying_stringthis reachesBunString__toThreadSafe(BunString.cpp:211-224), which installs an isolated copy and callsexisting->deref()on the originalWTFStringImpl. The caller'spathvariable now points at an impl whose refs have been released — this is the PR's own stated premise.errdefer store.deinit()is registered.try opts.getTruthyComptime(globalObject, "type")andtry file_type.toSlice(...)run. These perform a JS property access on a user-supplied options object and can throw.If step 3 throws:
errdefer store.deinit()frees the store (and its thread-safe copy of the path), the error propagates back to e.g.S3Client.presign, and since the construct function never returned,path_consumedis stillfalse. The caller'serrdefer if (!path_consumed) path.deinit()then derefs the already-released original impl →hasAtLeastOneRef()assertion in debug / double-free in release.Why existing code doesn't prevent it
The PR's flag is set on the line after the
try, so it cannot observe partial progress inside the callee. The callee's ownerrdefer store.deinit()correctly cleans up the store but does nothing to tell the caller that the original path refs were already consumed. Note thatgetCredentialsWithOptions(credentials_jsc.zig:203) also reads"type"once beforeinitS3, so a plain throwing getter would fail early (which is handled correctly); a stateful getter or Proxy that succeeds the first time and throws the second is what reaches the post-initS3read.Step-by-step proof
S3Client.presignparsespath = .slice_with_underlying_stringfor"abc", setspath_consumed = false, registerserrdefer if (!path_consumed) path.deinit().constructS3FileWithS3CredentialsAndOptions.getCredentialsWithOptionsreadstype(n=0 → returns"text/plain", n becomes 1).Blob.Store.initS3WithReferencedCredentials(path, …)runs →toThreadSafe()derefs the original impl behind the caller'spath.opts.getTruthyComptime(globalObject, "type")readstypeagain (n=1 → throws).error.JSErroris returned.errdefer store.deinit()frees the store; error propagates topresign.path_consumedis stillfalse→path.deinit()runs → derefs the already-released impl → crash.Impact and scope
This is the same crash class (Fuzzilli fingerprint
1ebbda306723eeb6) the PR targets, just on a window the patch doesn't cover. Fuzzilli routinely generates exactly these adversarial getters/Proxies, so this remaining window is likely to be rediscovered. It applies to every call site this PR modified:S3Client.{file,presign,exists,size,stat,write,unlink}andS3File.{presign,unlink,write,size,exists,stat}.How to fix
Move the ownership signal to the actual transfer point. Two straightforward options:
constructS3FileWithS3CredentialsAndOptions/constructS3FileWithS3Credentialstakepath: *jsc.Node.PathLikeand setpath.* = .{ .string = bun.PathString.empty }(or equivalent) immediately afterinitS3*returns. Callers can then unconditionallyerrdefer path.deinit()since deinit on an empty path is a no-op."type"handling to beforeinitS3*, so there is no fallible code between ownership transfer and return.