Fix double-free in S3 static methods when path is passed as string #28592
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,7 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J | |
| } | ||
| const options = args.nextEat(); | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| path_or_blob = .{ .blob = blob }; // path ownership transferred to store | ||
|
Comment on lines
86
to
+87
Contributor
There was a problem hiding this comment. 🟣 🟣 Pre-existing: Extended reasoning...What the bug is
// types.zig:564-566
.buffer => {
this.buffer.buffer.value.protect();
},i.e. it adds a JSC GC root on the underlying typed array's // Store.zig:454-459 (S3.deinit)
if (this.pathlike == .string) {
allocator.free(@constCast(this.pathlike.slice()));
} else {
this.pathlike.deinit(); // PathLike.deinit: .string, .buffer => {} — no-op for .buffer
}
Reachability from the PR-modified code
Why existing safeguards do not prevent itThere is a first protect on this same buffer taken by Step-by-step proof —
|
||
| defer blob.deinit(); | ||
|
Comment on lines
+87
to
88
Contributor
There was a problem hiding this comment. 🟣 Pre-existing: when fixing the Extended reasoning...What the bug isThe seven The specific code path that triggers it
if (globalObject.bunVM().mimeType(str.slice())) |entry| {
blob.content_type = entry.value; // static string, no allocation
break :inner;
}
const content_type_buf = bun.handleOom(allocator.alloc(u8, slice.len));
blob.content_type = strings.copyLowercase(slice, content_type_buf);
blob.content_type_allocated = true; // ← heap-owned, must be freed by Blob.deinit()So whenever the user passes a Why existing code does not prevent itThe Contrast with the structurally identical S3File.zig static methods that this PR modifies: at the line this comment is anchored to (S3File.zig:88) and at lines 119, 157, 197, 231, 583, the static-method side uses ImpactA small heap leak ( Step-by-step proofconst client = new Bun.S3Client({ bucket: "b" });
client.presign("key", { type: "application/x-custom-thing" });
The same trace applies to How to fixChange Relationship to this PRThis is pre-existing — the current PR diff does not touch |
||
| return try getPresignUrlFrom(&blob, globalThis, options); | ||
|
Contributor
There was a problem hiding this comment. 🟣 Pre-existing: Extended reasoning...What the bug is
var credentialsWithOptions: S3.S3CredentialsWithOptions = .{
.credentials = s3.getCredentials().*,
.request_payer = s3.request_payer,
};
Observable inconsistency
The fact that ReachabilityThis is reached from three places: (1) Step-by-step proofconst client = new Bun.S3Client({ bucket: "b", accessKeyId: "x", secretAccessKey: "y", acl: "public-read" });
client.presign("key"); // presigned URL has NO X-Amz-Acl
client.presign("key", {}); // presigned URL DOES have X-Amz-Acl=public-read
Same trace applies to How to fixEither initialize the literal with the store's values: var credentialsWithOptions: S3.S3CredentialsWithOptions = .{
.credentials = s3.getCredentials().*,
.options = s3.options,
.acl = s3.acl,
.storage_class = s3.storage_class,
.request_payer = s3.request_payer,
};or drop the literal entirely and unconditionally do Companion typoWhile editing this function: S3File.zig:497 reads Relationship to this PRBoth are pre-existing — the PR does not modify
Contributor
There was a problem hiding this comment. 🟣 Pre-existing companion to #3200790776 in the same function: the Extended reasoning...What the bug is and how it manifests
const result = credentialsWithOptions.credentials.signRequest(.{
.path = path,
.method = method,
.acl = credentialsWithOptions.acl,
.storage_class = credentialsWithOptions.storage_class,
.request_payer = credentialsWithOptions.request_payer,
.content_disposition = credentialsWithOptions.content_disposition,
.content_type = credentialsWithOptions.content_type,
}, false, .{ .expires = expires }) catch |sign_err| { ... };It passes The specific code path that triggers itAny presign call — Why existing safeguards don't prevent it
The asymmetry vs. Impact
Step-by-step proof —
|
||
| }, | ||
|
Comment on lines
84
to
90
Contributor
There was a problem hiding this comment. 🔴 The double-free fix was applied to Extended reasoning...What the bug is and how it manifestsThe PR fixes a double-free in The specific code path that triggers itIn each
Why existing code does not prevent itThe ImpactAll six How to fixIn each var blob = try constructS3FileWithS3CredentialsAndOptions(...);
path_neutralized = blob; // or equivalent tag-flip to disable errdefer
defer blob.detach();Step-by-step proof (concrete trigger for
|
||
|
|
@@ -114,6 +115,7 @@ pub fn unlink(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS | |
| } | ||
| const options = args.nextEat(); | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| path_or_blob = .{ .blob = blob }; // path ownership transferred to store | ||
|
Contributor
There was a problem hiding this comment. 🟣 🟣 Pre-existing companion to #3194798988: Extended reasoning...What the bug is
// Store.zig:415-430
const promise = jsc.JSPromise.Strong.init(globalThis); // heap-allocates Bun__StrongRef, GC-roots a JSPromise
const value = promise.value();
...
var aws_options = try this.getCredentialsWithOptions(extra_options, globalThis); // ← (1) can throw JSError
defer aws_options.deinit();
const options = try bun.S3.getListObjectsOptionsFromJS(globalThis, listOptions); // ← (2) can throw JSError
store.ref(); // ← no errdefer store.deref()
try bun.S3.listObjects(..., bun.new(Wrapper, .{
.promise = promise, // ← only sink for promise
.store = store,
.resolvedlistOptions = options,
...
}), proxy);There is no The specific code path that triggers itBoth fallible calls read user-controlled JS properties:
If either propagates Why existing code does not prevent itThe only sink for ImpactEach
Repeat in a loop and both native and JS heap grow unboundedly. Reachable via Step-by-step proofnew Bun.S3Client({ bucket: "b" }).list({ get prefix() { throw new Error("boom"); } });
The same trace applies with How to fixEither:
Relationship to this PR / not a duplicateThis is pre-existing — the PR diff does not touch
Contributor
There was a problem hiding this comment. 🟣 Pre-existing companion to #3199168443: when fixing Extended reasoning...What the bug isInside const str = try bun.String.fromJS(val, globalThis);
if (str.tag != .Empty and str.tag != .Dead) {
listObjectsOptions._X = str.toUTF8(bun.default_allocator);
...
}with no
Why this is the buggy block, not the canonical patternThe structurally identical option-parsing blocks in ImpactOne WTFStringImpl ref leaked per string option provided, on every call — success path included. Reachable via both This is distinct from #3199168443, which covers the wrapper-side Step-by-step proof —
|
||
| defer blob.deinit(); | ||
| return try blob.store.?.data.s3.unlink(blob.store.?, globalThis, options); | ||
|
Comment on lines
+118
to
120
Contributor
There was a problem hiding this comment. 🟣 Pre-existing: Extended reasoning...What the bug is
// Store.zig:357-368
const promise = jsc.JSPromise.Strong.init(globalThis); // heap-allocates Bun__StrongRef, roots promise in GC
const value = promise.value();
const proxy_url = ...;
const proxy = ...;
var aws_options = try this.getCredentialsWithOptions(extra_options, globalThis); // ← can throw JSError
defer aws_options.deinit();
store.ref();
try bun.S3.delete(&aws_options.credentials, this.path(), ..., Wrapper.new(.{
.promise = promise, // ← only sink for promise — never reached on error
...
}), ...);
The specific code path that triggers it
There is also a secondary leak: if Why existing code does not prevent itThe only sink for ImpactEach call to
This is reachable from both branches of the PR-modified Step-by-step proofconst f = Bun.file("s3://bucket/key");
await f.unlink({ get accessKeyId() { throw new Error("boom"); } }).catch(() => {});
Repeat in a loop and both native heap and JS heap grow unboundedly. How to fixEither:
Relationship to this PRThis is pre-existing — the PR diff does not modify
Contributor
There was a problem hiding this comment. 🟣 🟣 Pre-existing: the S3 response handler reached from every PR-modified static method (S3File.zig:120/160/199/233/585 → Extended reasoning...What the bug is and how it manifestsThe S3 response handlers parse error XML by independently scanning the whole response body for the open and close tags, then slicing between them with no bounds check ensuring the close tag was found after the open tag: // simple_request.zig:163-165 (errorWithBody)
if (strings.indexOf(bytes, "<Code>")) |start| {
if (strings.indexOf(bytes, "</Code>")) |end| {
code = bytes[start + "<Code>".len .. end];Both Affected sitesThe same unguarded pattern appears at:
Secondary: operator-precedence bug at line 213
if (!has_error and status == 200 or status == 206) {Per Zig precedence rules, Reachability from the PR-modified code
Step-by-step proofawait Bun.S3Client.unlink("key", { endpoint: "http://attacker.example", bucket: "b", accessKeyId: "x", secretAccessKey: "y" });The endpoint returns HTTP 500 with body
The same trace applies with ImpactA malicious or buggy S3-compatible endpoint (or a MITM on a non-TLS endpoint) can crash any Bun process that issues an S3 request to it, with a single malformed response body. Real AWS/R2/MinIO would not normally emit close-before-open XML, so the practical exposure is limited to user-configured/untrusted endpoints — but How to fixSearch for the closing tag starting after the opening tag and use the relative offset: if (strings.indexOf(bytes, "<Code>")) |start| {
const after = start + "<Code>".len;
if (strings.indexOf(bytes[after..], "</Code>")) |rel_end| {
code = bytes[after .. after + rel_end];applied at simple_request.zig:163-173/201-210, multipart.zig:496-498, and download_stream.zig:101-112; and parenthesise line 213 as Relationship to this PR / not a duplicatePre-existing — the PR diff does not touch simple_request.zig, multipart.zig, or download_stream.zig, and the trigger requires a malformed server response (close-tag-before-open-tag), not anything the PR's tag-flip affects. Flagged because (a) it is the response-side counterpart of the request-side resource bugs already on this PR (#3194798988 unlink, #3199168443 listObjects, #3200790769 small-upload Wrapper) — those comments already direct the author into
Contributor
There was a problem hiding this comment. 🟣 🟣 Pre-existing companion to #3200173422 (which already targets the Extended reasoning...What the bug is and how it manifests
if (str.tag != .Empty and str.tag != .Dead) {
new_credentials._sessionTokenSlice = str.toUTF8(bun.default_allocator);
new_credentials.credentials.sessionToken = new_credentials._sessionTokenSlice.?.slice();
new_credentials.changed_credentials = true;
}
if (session_token) |token| {
const session_token_value = bun.handleOom(bun.default_allocator.dupe(u8, token));
result.session_token = session_token_value;
result._headers[result._headers_len] = .{ .name = "x-amz-security-token", .value = session_token_value };— a plain Why this is the buggy block, not intentionalThe three siblings establish the pattern: any user-supplied option that becomes a raw Reachability from the PR-modified code
Threat model / impactHeader injection / request smuggling when an application forwards Two honest caveats: (a) Step-by-step proofBun.S3Client.unlink("key", {
sessionToken: "valid-token\r\nX-Injected: 1",
bucket: "b", accessKeyId: "x", secretAccessKey: "y",
});
How to fixIn the same block #3200173422 already directs the author to edit (credentials_jsc.zig:120-124), add the guard the three siblings have: new_credentials._sessionTokenSlice = str.toUTF8(bun.default_allocator);
const slice = new_credentials._sessionTokenSlice.?.slice();
if (containsNewlineOrCR(slice)) {
return globalObject.throwInvalidArguments("sessionToken must not contain newline characters (CR/LF)", .{});
}
new_credentials.credentials.sessionToken = slice;Relationship to this PR / not a duplicatePre-existing — the PR diff (six tag-flips in S3File.zig + a test) does not touch credentials_jsc.zig or credentials.zig, does not change what flows to |
||
| }, | ||
|
|
@@ -151,6 +153,7 @@ pub fn write(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSE | |
| return globalThis.throwInvalidArguments("Expected a S3 or path to upload", .{}); | ||
| } | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| path_or_blob = .{ .blob = blob }; // path ownership transferred to store | ||
|
Contributor
There was a problem hiding this comment. 🟣 🟣 Pre-existing companion to the early-return-resource class on this PR's Extended reasoning...What the bug is
In the Response/Request Why
|
||
| defer blob.deinit(); | ||
|
|
||
| var blob_internal: PathOrBlob = .{ .blob = blob }; | ||
|
|
@@ -190,6 +193,7 @@ pub fn size(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr | |
| return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); | ||
| } | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| path_or_blob = .{ .blob = blob }; // path ownership transferred to store | ||
|
Contributor
There was a problem hiding this comment. 🟣 Pre-existing: in this PR-modified function, the Extended reasoning...What the bug is and how it manifests
Why this is the buggy arm, not the canonical patternThe two structurally identical sibling functions handle the
Only Step-by-step proof —
|
||
| defer blob.deinit(); | ||
|
|
||
| return S3BlobStatTask.size(globalThis, &blob); | ||
|
|
@@ -223,6 +227,7 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS | |
| return globalThis.throwInvalidArguments("Expected a S3 or path to check if it exists", .{}); | ||
| } | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| path_or_blob = .{ .blob = blob }; // path ownership transferred to store | ||
|
Contributor
There was a problem hiding this comment. 🟣 🟣 Pre-existing: every S3 env-proxy lookup hardcodes Extended reasoning...What the bug is and how it manifestsEvery S3 call site that derives an HTTP proxy from the environment hardcodes Separately, the third/second arguments Affected sitesAll 16 sites hardcode the identical
By contrast, Why existing safeguards do not prevent itThere is no later re-derivation of the proxy from the actual scheme/host. The Impact
Step-by-step proofexport HTTPS_PROXY=http://corp-proxy:3128
# (no HTTP_PROXY)await Bun.S3Client.exists("key", { bucket: "b", accessKeyId: "x", secretAccessKey: "y" });
NO_PROXY case: How to fixDerive Relationship to this PR / not a duplicatePre-existing — the PR diff (six |
||
| defer blob.deinit(); | ||
|
|
||
| return S3BlobStatTask.exists(globalThis, &blob); | ||
|
|
@@ -574,6 +579,7 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr | |
| return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); | ||
| } | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| path_or_blob = .{ .blob = blob }; // path ownership transferred to store | ||
| defer blob.deinit(); | ||
|
|
||
| return S3BlobStatTask.stat(globalThis, &blob); | ||
|
Contributor
There was a problem hiding this comment. 🟣 Pre-existing: Extended reasoning...What the bug is
// S3File.zig:399
pub fn onS3StatResolved(result: S3.S3StatResult, this: *S3BlobStatTask) bun.JSError!void {while its two structurally identical siblings use the narrower set: pub fn onS3ExistsResolved(result: S3.S3StatResult, this: *S3BlobStatTask) bun.JSTerminated!void { ... }
pub fn onS3SizeResolved (result: S3.S3StatResult, this: *S3BlobStatTask) bun.JSTerminated!void { ... }All three are passed to // simple_request.zig:85
stat: *const fn (S3StatResult, *anyopaque) bun.JSTerminated!void,
The specific code path that triggers it
try this.promise.resolve(globalThis, (try S3Stat.init(
stat_result.size, stat_result.etag, stat_result.contentType,
stat_result.lastModified, globalThis,
)).toJS(globalThis));
Why existing safeguards don't prevent itThe ImpactIn Zig, error codes are global integers; returning
Practical reachability today is narrow (JS Step-by-step proof
How to fixChange pub fn onS3StatResolved(result: S3.S3StatResult, this: *S3BlobStatTask) bun.JSTerminated!void {
defer this.deinit();
const globalThis = this.global;
switch (result) {
.success => |stat_result| {
const stat_js = (S3Stat.init(...) catch |e| switch (e) {
error.JSTerminated => return error.JSTerminated,
error.JSError, error.OutOfMemory => {
try this.promise.reject(globalThis, globalThis.takeException(e));
return;
},
}).toJS(globalThis);
try this.promise.resolve(globalThis, stat_js);
},
.not_found, .failure => |err| { ... },
}
}This matches the sibling pattern and removes the unsafe error-set widening through Relationship to this PR / not a duplicateThis is pre-existing — the PR diff does not touch |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import { expect, test } from "bun:test"; | ||
|
claude[bot] marked this conversation as resolved.
|
||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| test("S3Client static methods should not crash with string path arguments", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| async function main() { | ||
| const calls = [ | ||
| () => Bun.S3Client.presign(Date()), | ||
| () => Bun.S3Client.presign(Date(), Date()), | ||
| () => Bun.S3Client.unlink(Date()), | ||
| () => Bun.S3Client.size(Date()), | ||
| () => Bun.S3Client.exists(Date()), | ||
| () => Bun.S3Client.stat(Date()), | ||
| () => Bun.S3Client.write(Date(), "data"), | ||
|
Comment on lines
+12
to
+18
Contributor
There was a problem hiding this comment. 🟡 The regression test uses Extended reasoning...What the bug is and how it manifests In JavaScript, The specific code path that triggers Path 2 The PR description identifies two double-free paths. Path 2 occurs when Why the current test does not prevent it All entries in the Addressing the refutation One verifier argued that Store.zig was not modified in commit 9538183, so Path 2 was never actually fixed — and therefore not testing it is consistent. This is a fair observation about implementation completeness. However, it cuts both ways: if Path 2 remains broken, a test that attempts to trigger it (with Impact The coverage gap means the described Path 2 scenario remains unverified by CI. Under an ASAN build the double-free would manifest as a use-after-poison crash if Path 2 is genuinely unfixed. The missing test would catch any regression reintroducing the bug in either path. How to fix Add a const throwingOptions = { get type() { throw new Error("boom"); } };
// then add:
() => Bun.S3Client.presign(Date(), throwingOptions),
() => Bun.S3Client.unlink(Date(), throwingOptions),
// etc.This deterministically exercises the post-
Comment on lines
+14
to
+18
Contributor
There was a problem hiding this comment. 🟡 nit: of these 7 entries, only the two Extended reasoning...What this is aboutThe test's inline comment at line 31-32 says credentials are stripped "so presign fails synchronously and exercises the errdefer cleanup path" — which is accurate for Why only
|
||
| ]; | ||
| for (const call of calls) { | ||
| try { await call(); } catch(e) {} | ||
| } | ||
| Bun.gc(true); | ||
| console.log("OK"); | ||
| } | ||
| main(); | ||
| `, | ||
| ], | ||
| env: { | ||
| ...bunEnv, | ||
| // Strip any ambient S3/AWS credentials so presign fails synchronously | ||
| // and exercises the errdefer cleanup path. | ||
| AWS_ACCESS_KEY_ID: undefined, | ||
| AWS_SECRET_ACCESS_KEY: undefined, | ||
| AWS_SESSION_TOKEN: undefined, | ||
| AWS_REGION: undefined, | ||
| AWS_BUCKET: undefined, | ||
| AWS_ENDPOINT: undefined, | ||
| S3_ACCESS_KEY_ID: undefined, | ||
| S3_SECRET_ACCESS_KEY: undefined, | ||
| S3_SESSION_TOKEN: undefined, | ||
| S3_REGION: undefined, | ||
| S3_BUCKET: undefined, | ||
| S3_ENDPOINT: undefined, | ||
| }, | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe("OK\n"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
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 diff no longer matches the PR description — only the original v1 tag-flip in
S3File.zigand the test file remain. Theencoded_sliceclone inStore.initS3/initS3WithReferencedCredentials, theerrdefer→deferconversions inS3File.zig/S3Client.zig, and theBlob.zigdefers described in the PR body and CodeRabbit walkthrough are all absent (likely lost when rebasing onto thesrc/bun.js/→src/runtime/restructure in c8b4c36/e643d7b). Concretely, the Path-2 double-free that was marked "✅ Addressed in commit 9538183" is reachable again, and the earlier review comments atStore.zig:74andBlob.zig:2123now give advice predicated on a clone that no longer exists — applying them as-is would introduce new double-frees.Extended reasoning...
What the bug is
The PR description's Fix section states: "Clone
encoded_sliceininitS3/initS3WithReferencedCredentialsso the store always owns an independent copy" and "Changeerrdefertodeferfor path cleanup in S3File functions". The CodeRabbit walkthrough additionally lists changes toS3Client.zig(8 methods),Blob.zig(constructBunFile+findOrCreateFileFromPath), andstat()error-message corrections. However, the actual diff contains only two files:src/runtime/webcore/S3File.zig(sixpath_or_blob = .{ .blob = blob }tag-flips) and the test. Verified against the current tree at the post-restructure paths:src/runtime/webcore/blob/Store.zig:68-124—initS3/initS3WithReferencedCredentialscontain onlyvar path = pathlike; path.toThreadSafe();with notoOwned/clone logic.src/runtime/webcore/S3Client.zig:138,154,172,189,206,220,254— all still useerrdefer path.deinit(), notdefer.src/runtime/webcore/S3File.zig— the sixerrdefer { if (path_or_blob == .path) ... }blocks are unchanged (not converted todefer);stat()still says"get size"at both branches.git logonStore.zigshows it was only touched by the puregit mvrestructure (c8b4c36), never by this PR's fix commits. The edits to the oldsrc/bun.js/webcore/paths almost certainly did not survive the rebase onto thesrc/runtime/webcore/restructure.Concrete consequence: the Path-2 double-free is back
Inline comment 2996090258 was marked "✅ Addressed in commit 9538183", but with the Store.zig clone gone, the tag-flip alone cannot fix Path 2 because ownership transfers inside the
tryexpression before the tag is flipped. Step-by-step, forBun.S3Client.presign(<path>, <opts>)whereoptshas atypegetter that throws on its second read (the first read happens earlier ingetCredentialsWithOptions) and<path>produces an.encoded_slice:PathOrBlob.fromJSNoCopy→path_or_blob = .pathwithencoded_slicebuffer A.errdefer { if (path_or_blob == .path) path_or_blob.path.deinit() }armed.try constructS3FileInternalStore(...)→constructS3FileWithS3Credentials:Blob.Store.initS3(path, ...)runs. Store.zig:97-122 doesvar path = pathlike; path.toThreadSafe();—PathLike.toThreadSafe()(types.zig:557-569) falls through toelse => {}for.encoded_slice, a no-op, sostore.data.s3.pathlikealiases buffer A.errdefer store.deinit()armed.try opts.getTruthyComptime(globalObject, "type")invokes the getter → throws →error.JSError.errdefer store.deinit()fires →Store.deinit()(Store.zig:179-201) →s3.deinit()(Store.zig:454-459) →pathlike.deinit()→ frees A.tryat S3File.zig:86. Line 87 (path_or_blob = .{ .blob = blob }) never executes.errdeferfires:path_or_blob == .pathis still true →path_or_blob.path.deinit()→ frees A again → use-after-poison.Why the existing change does not prevent it
The only Zig change in the diff is the tag-flip placed after
try constructS3FileInternalStore(...). By Zig semantics, an assignment after atrydoes not run when thetrypropagates an error. So the tag-flip helps Path 1 (constructor succeeds, downstream op fails) but cannot help Path 2 (constructor fails afterinitS3). The intended fix for Path 2 was the Store.zig clone, which is missing.Secondary consequence: stale review comments now give dangerous advice
Earlier comments at
Store.zig:74(#2996283354) andBlob.zig:2123(#2996546999) instruct addingdefer path.deinit()/ movingdefer path.deinitAndUnprotect()on the assumption thatinitS3clones the path and the caller must free its own copy. With the clone gone,initS3once again takes ownership of the caller's buffer; applying those suggestions as written would make both the caller and the store free the same buffer — a new double-free. Those comments only make sense once the Store.zig clone is restored.Addressing the "duplicate" objection
One reviewer noted overlap with existing comments: the S3Client instance-method double-free is already tracked in #2997528729, and the
stat()error message in #2996283366. That is true, but neither of those captures the root cause here — that the diff has regressed to its v1 state and no longer contains what the PR description and "✅ Addressed" markers claim. Without surfacing this explicitly, the author may believe commits 9538183+ are present when they are not, and may apply the now-dangerous follow-up suggestions on top of the wrong base. The actionable fix is different in kind ("restore the dropped commits at the newsrc/runtime/webcore/paths") from any individual open comment.How to fix
Re-apply the dropped changes at the post-restructure paths: (1) clone
encoded_sliceviatoOwned(allocator)inStore.initS3/initS3WithReferencedCredentials; (2) convert the sixerrdeferblocks inS3File.zigtodefer; (3) changeerrdefer path.deinit()→defer path.deinit()across theS3Client.ziginstance/static methods; (4) add theBlob.zigdefers (constructBunFile,findOrCreateFileFromPath); (5) fix thestat()error strings. Then re-evaluate the still-open follow-up comments against the restored base.