Fix double-free of path in S3 static methods on error paths #28495
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import { expect, test } from "bun:test"; | ||
|
|
||
| // Regression test for double-free of path in S3 static and instance methods. | ||
| // The bug crashes under ASAN/debug builds when signRequest fails after | ||
| // constructS3FileInternalStore/constructS3FileWithS3CredentialsAndOptions | ||
| // transfers path ownership to the blob store via toThreadSafe(). | ||
| // Both errdefer and defer would deref the same WTFStringImpl. | ||
|
|
||
| // Instance method path (S3Client.zig) — uses explicit empty credentials | ||
| // to ensure missing-credentials path regardless of ambient AWS/S3 env vars. | ||
| test("S3Client instance presign does not crash on missing credentials", () => { | ||
| const client = new Bun.S3Client({ accessKeyId: "", secretAccessKey: "" }); | ||
| expect(() => client.presign("a")).toThrow("Missing S3 credentials"); | ||
| expect(() => client.presign("some-key", {})).toThrow("Missing S3 credentials"); | ||
| }); | ||
|
Comment on lines
+1
to
+15
Contributor
There was a problem hiding this comment. 🟡 The test file only contains one subprocess-based crash regression test (for Extended reasoning...The final committed test file The double-free bug this PR fixes occurs at the Zig level: when The PR description states "Tests cover all 6 methods" but this does not match the committed code. The timeline shows there were earlier iterations with tests for the other 5 methods using The impact is that if anyone refactors A subprocess test following the same pattern as the presign test would provide the needed coverage for each missing method. For example: test("s3 static write with missing credentials does not crash", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", `Bun.S3Client.write("a", "data").catch(e => console.log(e.code))`],
env,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout.trim()).toBe("ERR_S3_MISSING_CREDENTIALS");
expect(exitCode).toBe(0);
});The same pattern applies to No verifiers refuted this finding. The Zig fix itself is correct and complete; this is purely a test coverage gap for 5 of the 6 patched call sites. |
||
|
|
||
| // Static method path (S3File.zig) — exercises the separate static entrypoint. | ||
| test("S3Client static presign does not crash on missing credentials", () => { | ||
| expect(() => Bun.S3Client.presign("a")).toThrow("Missing S3 credentials"); | ||
| }); | ||
|
Comment on lines
+9
to
+20
Contributor
There was a problem hiding this comment. 🟡 Both tests in this file are non-hermetic on machines with Extended reasoning...What the bug isBoth tests in
Why nothing prevents it
This is a follow-on to resolved comment #2981748440, which flagged the env-dependence and suggested the empty-string-credentials approach. That fix was applied to the instance test, but (a) it doesn't actually work because of Step-by-step proof
ImpactCI is unaffected — Bun's test runners use How to fixSince empty-string options don't override env credentials, the only reliable approach is to strip the env vars. For example, at the top of the file: for (const k of ["AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "AWS_REGION", "AWS_SESSION_TOKEN", "S3_ACCESS_KEY_ID", "S3_SECRET_ACCESS_KEY", "S3_REGION", "S3_SESSION_TOKEN", "S3_BUCKET", "AWS_BUCKET", "S3_ENDPOINT", "AWS_ENDPOINT"]) {
delete process.env[k];
}(or run the calls in a |
||
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.
🟣 This PR correctly fixes the double-free when constructS3FileInternalStore succeeds but the subsequent operation fails. However, a pre-existing edge case remains: if constructS3FileInternalStore fails internally after Store.initS3 has already run (e.g., the "type" option getter throws), the errdefer store.deinit() inside constructS3FileWithS3Credentials and the outer errdefer path_or_blob.path.deinit() in presign/unlink/etc. both fire on the same WTFStringImpl. The sentinel assignment is only reached on successful return from constructS3FileInternalStore, providing no protection for this internal failure path.
Extended reasoning...
Remaining double-free inside constructS3FileInternalStore failures
What the bug is
The PR fixes the case where constructS3FileInternalStore returns successfully but the subsequent S3 operation fails. However, constructS3FileInternalStore calls constructS3FileWithS3Credentials, which can itself fail after Blob.Store.initS3 has already run, leaving the caller holding a stale reference to a WTFStringImpl that the store also owns.
The specific code path
In constructS3FileWithS3Credentials (visible in the modified file context):
try-- if this fails, initS3 has not yet run, so no ownership issue.bun.handleOom(Blob.Store.initS3(path, ...))runs successfully. Inside initS3 (Store.zig:98-100):var path = pathlike; path.toThreadSafe();-- a shallow copy of the PathLike is made, then toThreadSafe() is called on the local copy.errdefer store.deinit()is registered after initS3.opts.getTruthyComptime(globalObject, "type")is atrycall -- if the options object has a getter for "type" that throws a JS exception, this propagates as JSError.errdefer store.deinit()fires -> s3.deinit(allocator) -> this.pathlike.deinit() -> decrefs the WTFStringImpl.path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } }is on the line AFTER thetry constructS3FileInternalStore(...)call and is never reached.errdefer path_or_blob.path.deinit()fires -> second deref on the same WTFStringImpl.Why existing code does not prevent it
For already-thread-safe WTFStrings: String.toThreadSafe() is documented as "Does not increment the reference count unless the StringImpl is cloned" -- BunString__toThreadSafe calls isolatedCopy() which returns the same ptr for already-thread-safe strings. So the store copy and path_or_blob.path share the same WTFStringImpl with no extra refcount. Two deref() calls -> double-free.
For not-yet-thread-safe strings: SliceWithUnderlyingString.toThreadSafe() creates a clone Y for the store and calls orig.deref() on the original ptr X (inside the local copy which is a bitwise copy of the WTFStringImpl also held by path_or_blob.path). This consumes a reference that the caller's path_or_blob.path also held. The outer errdefer then calls deref() on the potentially freed X -> use-after-free. The refutation's argument that "different ptrs means no double-free" overlooks that X's refcount was decremented during initS3, potentially freeing it before the outer errdefer fires.
The PR sentinel is placed correctly for the success path but provides zero protection when constructS3FileInternalStore fails mid-execution.
Step-by-step proof
Trigger:
Bun.S3Client.presign("some-key", { get type() { throw new Error("oops") } })errdefer path_or_blob.path.deinit()registered.try constructS3FileInternalStore(globalThis, path.path, options)called.path_or_blob = .{ .path = .{ .fd = bun.invalid_fd } }is never executed.How to fix
The cleanest fix is to have initS3 (or the caller before passing to initS3) increment the refcount so both the store and the caller hold independent references. Alternatively, the sentinel assignment could be placed inside constructS3FileWithS3Credentials itself right after initS3 succeeds. As-written, the PR's approach of nullifying after a successful
try constructS3FileInternalStore(...)cannot cover failures that occur inside that function.Addressing the refutation
The refutation claims regular JS string literals are not already thread-safe, so toThreadSafe() always clones and the paths are independent. This is true for the strict same-ptr double-deref scenario, but misses the use-after-free path for non-thread-safe strings: initS3 decrements the refcount of the original WTFStringImpl via orig.deref() inside toThreadSafe(), and the outer errdefer then fires on the same now-freed object. Both string-safety states lead to memory unsafety on this failure path. The trigger (throwing type getter) is unusual but valid JS, and the bug predates this PR.