Fix crash in S3 presign with missing credentials#28423
Conversation
|
Updated 12:33 PM PT - Mar 25th, 2026
❌ @robobun, your commit d230939 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 28423That installs a local version of the PR into your bun-28423 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughS3 presign logic now resolves credentials/options via S3Credentials.getCredentialsWithOptions and signs using a new exported S3File.presignFromCredentials; temporary detached Blob/S3File construction was removed and path cleanup uses unconditional defer. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/webcore/S3File.zig`:
- Line 490: Fix the typo in the invalid-arguments error message by changing
"greather" to "greater" in the globalThis.throwInvalidArguments calls; update
the occurrence shown (the call that currently reads
globalThis.throwInvalidArguments("expiresIn must be greather than 0", .{})) and
the identical message in getPresignUrlFrom so both functions emit "expiresIn
must be greater than 0".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0c70d15-6836-4c55-aecc-e76391a15893
📒 Files selected for processing (3)
src/bun.js/webcore/S3Client.zigsrc/bun.js/webcore/S3File.zigtest/js/bun/s3/s3-presign-missing-credentials.test.ts
|
✅ Code complete. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/webcore/S3File.zig`:
- Around line 477-513: presignFromCredentials was updated to include
content_encoding in the canonical request, but the other presign path
(getPresignUrlFrom / Blob.presign) still builds the signed fields inline and is
missing content_encoding, causing different presigned URLs; update
getPresignUrlFrom (and any Blob.presign implementation) to either call
presignFromCredentials directly or include content_encoding in the fields passed
to signRequest so both code paths sign the exact same set of options (match the
struct literal used in presignFromCredentials: .path, .method, .acl,
.storage_class, .request_payer, .content_disposition, .content_type,
.content_encoding) and remove duplicated signing logic to prevent future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7aee996-0950-4502-b494-53be874eb9bb
📒 Files selected for processing (2)
src/bun.js/webcore/S3File.zigtest/js/bun/s3/s3-presign-missing-credentials.test.ts
There was a problem hiding this comment.
LGTM — all critical issues from prior reviews have been addressed.
Extended reasoning...
Overview
This PR fixes a GC crash (SIGFPE / division by zero in StringImpl::costDuringGC) caused by calling throwSignError while a deferred blob cleanup was live. The fix is applied in S3Client.zig and S3File.zig: blob construction is bypassed entirely for path-based presign, with credentials resolved via getCredentialsWithOptions and the request signed via a new presignFromCredentials helper. A regression test spawns an isolated subprocess with all AWS/S3 env vars unset.
Prior Issues — All Resolved
- Path normalization (
s3://prefix stripping): fixed in commit 22482d8 — both call sites now applybun.URL.parse().s3Path()before passing topresignFromCredentials. - Memory leak (
errdefer→deferinS3File.presign): fixed in commit 22482d8 — path cleanup now unconditional. - Typo (
greather→greater): fixed in commit df71301 — corrected in bothpresignFromCredentialsandgetPresignUrlFrom. - All signRequest error paths (
InvalidPath,InvalidEndpoint, etc.): the new approach constructs no blob at all, sothrowSignErrorfor any error variant no longer fires with deferred blob cleanup live. - Redundant
getCredentialsWithOptionscalls: the new code calls it exactly once and passes the result topresignFromCredentials, which does not call it again.
Security Risks
No authentication bypass or credential exposure. The fix avoids constructing a temporary blob/store, which is the correct and minimal change to eliminate the crash.
Level of Scrutiny
Moderate — the change is in S3 Zig memory management code, but the approach is clean, focused, and consistent with the rest of the codebase. The regression test correctly exercises all three presign API surfaces under isolated env conditions and forces a GC cycle.
Other Factors
Remaining nits (missing expect(stderr).toBe("") assertion, pre-existing content_encoding omission in presignFromCredentials) are non-blocking and already covered by inline comments. CI failures are in bundler_compile.test.ts across multiple platforms and appear unrelated to the S3 changes (timeouts and code 1 on Linux x64/aarch64 builds).
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed.
Extended reasoning...
Overview
This PR fixes a SIGFPE crash during GC when calling Bun.s3.presign(), new Bun.S3Client().presign(), or Bun.S3Client.presign() with missing credentials. The fix touches S3Client.zig, S3File.zig, and adds a regression test. The core change introduces a new presignFromCredentials helper that bypasses blob construction entirely, eliminating the deferred-cleanup/error-throw interaction that corrupted the exception scope chain.
Security risks
No security-sensitive code paths are affected. The change is about cleanup ordering in Zig memory management, not signing logic or authentication.
Level of scrutiny
Moderate — this touches S3 signing infrastructure but the changes are well-scoped. The root cause analysis is sound: removing blob construction for the presign path eliminates ALL signRequest error interactions with blob defers, not just MissingCredentials.
Resolution of prior feedback
All issues I raised in previous review rounds have been resolved: errdefer→defer memory leak fix, s3Path() + leading-slash strip for correct path normalization (matching the two-step normalization Store.path() previously performed), content_encoding forwarded in both signing paths, typo corrected, test uses subprocess isolation with 12 AWS/S3 env vars cleared, uses idiomatic proc.stdout.text(), and asserts expect(stderr).toBe(""). The regression test correctly exercises all three presign API surfaces followed by Bun.gc(true) — it would SIGFPE on main.
There was a problem hiding this comment.
LGTM — all previously raised concerns have been addressed.
Extended reasoning...
Overview
This PR fixes a SIGFPE crash in S3 presign with missing credentials by refactoring S3File.presign and S3Client.presign to bypass blob construction entirely. A new presignFromCredentials helper resolves credentials via getCredentialsWithOptions and calls signRequest directly, eliminating the deferred blob-cleanup / error-throw interaction that corrupted the exception scope chain. Changes touch S3Client.zig, S3File.zig, and a new regression test.
Prior review resolution
All four inline comments I flagged in earlier iterations have been marked resolved and addressed in the current diff:
- Path normalization (
s3://prefix):bun.URL.parse().s3Path()+ leading-slash strip now matchesStore.S3.path()in both call sites. - errdefer → defer: Applied in both S3Client.zig and S3File.zig so the PathLike is freed on both success and error paths.
- content_encoding parity:
.content_encoding = credentialsWithOptions.content_encodingadded to bothpresignFromCredentialsandgetPresignUrlFrom. - Redundant getCredentialsWithOptions calls: New flow calls it exactly once per presign invocation; result is passed directly to
presignFromCredentialsrather than discarded and recomputed.
Security risks
No authentication bypass or data exposure concerns. The change narrows the surface of a memory-safety crash; it does not affect credential validation logic.
Level of scrutiny
Moderate. The fix is targeted (3 files, focused on presign path) and the root-cause analysis is sound — blob construction was the only source of the deferred cleanup that interacted with throwSignError. presignFromCredentials has no deferred blob cleanup, so throwSignError on any signRequest error (MissingCredentials, InvalidPath, InvalidEndpoint, etc.) is now safe. The regression test spawns an isolated subprocess with 12 credential env vars cleared and verifies all three presign APIs + Bun.gc(true) exit cleanly.
Other factors
CI failures on bundler_compile.test.ts are pre-existing infra/unrelated issues (confirmed across multiple builds). The latest commit is test-only (removed strict stderr check for ASAN compatibility). No TODO/FIXME/HACK in added lines.
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed across multiple iterations.
Extended reasoning...
Overview
This PR fixes a SIGFPE crash during GC by refactoring the path-based presign code path in S3Client.zig and S3File.zig to avoid blob construction entirely. It introduces presignFromCredentials, a new helper that resolves credentials via getCredentialsWithOptions and calls signRequest directly. A regression test spawns an isolated subprocess with all S3/AWS credential env vars cleared, exercises all three presign API surfaces, forces GC, and asserts clean exit.
Security risks
No security risks introduced. Credential handling follows the same patterns as the rest of the S3 subsystem. The fix reduces code surface rather than expanding it.
Level of scrutiny
This is a bug fix in the S3 presign code path, touching non-trivial Zig memory management. It went through 8 verification iterations and multiple rounds of review. The root cause analysis is sound: defer blob.deinit() running while a JS exception was active in the VM corrupted the exception scope chain. Replacing blob construction with direct credential resolution eliminates the problematic interaction structurally, not just symptomatically.
Other factors
All bugs I previously flagged have been resolved: path normalization via s3Path() + leading slash strip, errdefer→defer memory leak fix, content_encoding parity in both signing helpers, the greather typo, and test subprocess isolation. The one remaining nit (missing expect(stderr).toBe("") assertion) is cosmetic and does not affect correctness or crash coverage. CI failures on the prior builds were unrelated (bundler timeout, webview-chrome). This PR is ready to merge.
There was a problem hiding this comment.
LGTM — all previous review concerns have been addressed.
Extended reasoning...
Overview
This PR modifies S3Client.zig, S3File.zig, and adds a regression test to fix a SIGFPE crash during GC when Bun.s3.presign() is called without configured credentials. The core change avoids blob construction for the path-based presign case, instead resolving credentials via getCredentialsWithOptions and signing directly via a new presignFromCredentials helper.
Security risks
No security-sensitive changes. The fix restructures an internal code path to avoid a memory corruption pattern; it does not alter credential handling semantics, access control, or data exposure surface.
Level of scrutiny
The Zig memory management changes are the most sensitive part — specifically the errdefer → defer conversion and the removal of blob construction from the error path. These changes are correct: the old errdefer was only appropriate when ownership transferred to the blob store; after the refactor, no such transfer occurs, so unconditional defer is required. The path normalization (bun.URL.parse().s3Path() + leading-slash strip) faithfully replicates the two-step normalization previously done via Store.S3.path().
Other factors
All previously flagged issues from multiple review rounds have been resolved: content_encoding parity in both presignFromCredentials and getPresignUrlFrom, typo fix, test environment isolation via subprocess with explicit env var clearing, idiomatic proc.stdout.text() pattern, and the s3:// prefix double-slash normalization. CI failures are confined to bundler_compile.test.ts and webview-chrome.test.ts — unrelated to S3. The only remaining nit is the missing expect(stderr).toBe("") assertion in the test, which does not affect correctness.
There was a problem hiding this comment.
LGTM — all issues from prior review rounds have been addressed.
Extended reasoning...
Overview
This PR touches S3Client.zig, S3File.zig, and a new regression test. The core change bypasses blob construction for the path-based presign code path, resolving a GC crash caused by the interaction between deferred blob cleanup and throwSignError.
Security risks
No security-sensitive code paths are affected. The change manipulates S3 credential resolution and URL signing, both of which were already present.
Level of scrutiny
Moderate — the fix is a targeted refactor of the presign code path, not touching auth or crypto primitives directly. The new presignFromCredentials helper is clean and well-scoped.
Other factors
All issues I raised in prior rounds have been resolved: path normalization (s3:// stripping + leading slash), errdefer→defer for path cleanup across all S3 functions, content_encoding added to signRequest, typo fixed, test uses subprocess with clean env and proper stderr assertion. The pre-existing sessionToken label typo in credentials.zig:159 was explicitly flagged, Jarred requested a fix, and the comment is marked resolved — though the fix did not land in this PR, it is strictly pre-existing and out of scope.
|
Verification (robobun, iteration 2 review): HEAD 1c88c93 reverts the overly broad errdefer→defer changes from be37445 that caused S3 test segfaults/assertion failures in Build #41316. Zig source is now identical to 33ddd8a which ran Build #41219 with zero S3-related failures (only bundler_compile.test.ts timeouts). Build #41332 pending on HEAD. Diff: S3Client.zig and S3File.zig presign() bypass blob construction, resolve credentials via getCredentialsWithOptions, sign via new presignFromCredentials helper; errdefer→defer only in presign functions where ownership no longer transfers. content_encoding parity and greather→greater typo fixed. Regression test spawns isolated subprocess (12 AWS/S3 env vars cleared), exercises all 3 presign APIs + Bun.gc(true), asserts ERR_S3_MISSING_CREDENTIALS and exit 0. No TODO/FIXME/HACK. Both Jarred comments addressed. Both CodeRabbit actionable items resolved. |
There was a problem hiding this comment.
LGTM — all prior concerns addressed.
Extended reasoning...
Overview
This PR modifies S3Client.zig, S3File.zig, and adds a regression test in test/js/bun/s3/s3-presign-missing-credentials.test.ts. The core change restructures the presign code path to bypass Blob.Store construction entirely, resolving credentials directly via getCredentialsWithOptions and signing via a new presignFromCredentials helper.
Security Risks
None. The change does not touch auth logic — it consumes existing credential resolution unchanged. No new attack surface is introduced.
Level of Scrutiny
This is a targeted crash fix in S3 presign code. The blast radius is limited to three functions (S3Client.presign, S3File.presign, new presignFromCredentials) plus the blob-based getPresignUrlFrom which gets content_encoding parity. Not security-critical, not on a hot path.
Other Factors
All prior red and yellow issues I raised have been resolved in the current diff: (1) throwSignError is now called with no blob/store defers active — only aws_options.deinit() (credential slices) and result.deinit() (plain buffer) run on error unwind, eliminating the WTF StringImpl refcount corruption; (2) path normalization via s3Path() + trailing-backslash strip + leading-slash strip matches Store.S3.path() exactly; (3) errdefer → defer correctly applied in presign since path ownership never transfers; (4) content_encoding added to both signing paths; (5) typo fixed; (6) regression test spawns an isolated subprocess clearing 12 AWS/S3 env vars, exercises all three presign APIs + Bun.gc(true), and asserts ERR_S3_MISSING_CREDENTIALS plus a clean exit. CI failures are pre-existing bundler_compile.test.ts timeouts on Linux — zero S3-related failures across all builds.
There was a problem hiding this comment.
LGTM — all prior review concerns have been addressed.
Extended reasoning...
Overview
This PR fixes a SIGFPE crash during GC when presign() is called without S3 credentials. Three files changed: S3Client.zig, S3File.zig (presign logic refactored), and a new regression test. The fix bypasses blob construction in the presign path, instead resolving credentials via getCredentialsWithOptions and calling a new presignFromCredentials helper directly.
Security risks
The change touches S3 credential resolution and request signing. No new credential handling logic is introduced — it reuses the existing getCredentialsWithOptions and signRequest functions. The path normalization mirrors Store.S3.path() exactly (s3Path + trailing backslash strip + leading slash strip per commit a53d6a7). No security regressions identified.
Level of scrutiny
This required careful review due to Zig memory ownership semantics (errdefer vs defer, PathLike move semantics) and S3 signing correctness. Multiple iterations addressed: path normalization for s3:// prefixes, trailing backslash parity with Store.S3.path(), content_encoding parity between old and new signing paths, the typo fix, test isolation with cleared env vars, and stderr assertion. All issues were resolved across commits.
Other factors
The regression test is solid: subprocess spawned with 12 AWS/S3 env vars cleared, all three presign API surfaces exercised, Bun.gc(true) called (the original crash trigger), stderr asserted via not.toContain("error:"). CI failures across all builds are in unrelated test files (bundler_compile.test.ts timeouts, Darwin infra). Prior redundant getCredentialsWithOptions calls noted in earlier reviews were resolved by the approach of calling it once and passing &aws_options through to presignFromCredentials.
There was a problem hiding this comment.
LGTM — all prior issues resolved.
Extended reasoning...
Overview
This PR fixes a SIGFPE/division-by-zero crash during GC when calling Bun.s3.presign(), new Bun.S3Client().presign(), or Bun.S3Client.presign() without configured credentials. Three files are modified: S3Client.zig, S3File.zig, and a new regression test.
Root Cause and Fix
The root cause was that throwSignError was called while a stack-allocated blob with defer blob.deinit() / defer blob.detach() was live, corrupting the exception scope chain and leaving a StringImpl with zero refcount for GC to visit. The fix introduces presignFromCredentials, a new helper that signs the request directly from already-resolved credentials without constructing any temporary blob. Since no blob defers are live when throwSignError is called, the corruption cannot occur.
Path normalization in both S3Client.presign and S3File.presign correctly mirrors Store.S3.path(): bun.URL.parse().s3Path(), strip trailing backslash, strip leading slash — matching what the old blob-based path did.
Issue Resolution
All inline issues raised in prior review rounds are marked resolved:
errdefer→deferapplied only inpresign()(where ownership is no longer transferred); all other S3File/S3Client functions correctly retainerrdefercontent_encodingadded to bothpresignFromCredentialsandgetPresignUrlFromfor parity- "greather" typo fixed to "greater"
- Test uses isolated subprocess with 12 AWS/S3 env vars cleared,
proc.stdout.text()/proc.stderr.text()idiom, andexpect(stderr).not.toContain("error:") - Path normalization trailing backslash strip added in a53d6a7
Security Risks
No new security risks. Credentials are resolved via getCredentialsWithOptions and never persisted. The fix eliminates a crash path; it does not weaken authentication or authorization.
Level of Scrutiny
Moderate — S3 presign touches credential handling and URL generation, but the change is narrowly scoped to the presign path and the fix is mechanically sound. The regression test would have caught the SIGFPE on main.
Other Factors
CI failures across all recent builds (bundler_compile.test.ts timeouts, Darwin expired, ASAN) are infrastructure issues with zero S3-related failures. The pre-existing credentials.zig sessionToken error-message typo (line 159) was flagged as pre-existing and its thread marked resolved; it is not introduced by this PR.
There was a problem hiding this comment.
LGTM — all previously raised concerns are resolved in the current diff.
Extended reasoning...
Overview
This PR fixes a SIGFPE crash during GC when presign() is called without S3 credentials. It touches S3Client.zig, S3File.zig, src/s3/credentials.zig, and adds a regression test. The change is focused: 4 files, ~200 lines, no architectural changes.
Security risks
None. The change removes a code path (temporary blob construction during presign) and replaces it with a direct credential resolution + signing call. No new attack surface is introduced. The sessionToken error message fix is purely cosmetic.
Level of scrutiny
Medium — S3 signing is security-adjacent, but this PR only changes error handling and resource cleanup order, not the signing logic itself. signRequest is unchanged. The fix has been through 14+ iterations of review with multiple verifiers.
Other factors
All previous inline comments were addressed: path normalization now matches Store.S3.path() exactly (s3Path + trailing backslash strip + leading slash strip), content_encoding parity was added to both signing paths, errdefer→defer was correctly applied only to presign (where ownership never transfers), and the regression test properly spawns an isolated subprocess with 12 AWS/S3 env vars cleared. CI failures in prior builds were all in unrelated bundler/webview tests. The bug hunting system found no new issues.
There was a problem hiding this comment.
LGTM — all previously raised concerns have been addressed.
Extended reasoning...
Overview
This PR fixes a SIGFPE/GC crash in Bun.s3.presign(), new Bun.S3Client().presign(), and Bun.S3Client.presign() when S3 credentials are missing. The fix touches four files: S3Client.zig, S3File.zig, credentials.zig, and a new regression test.
Core Fix
The root cause was that throwSignError was called while a stack-allocated blob with defer blob.deinit() was still live, corrupting the exception scope chain. The fix bypasses blob construction entirely for all path-based presign calls: credentials are resolved via getCredentialsWithOptions first, then the new presignFromCredentials helper signs directly. This eliminates the problematic interaction for all signRequest errors, not just MissingCredentials.
Issues Addressed Since Initial Review
- Path normalization parity: New code applies
bun.URL.parse().s3Path()+ trailing backslash strip + leading slash strip, matchingStore.S3.path()exactly (including the two-strip requirement I flagged). content_encodingparity: Added to bothpresignFromCredentialsandgetPresignUrlFromso blob-based and path-based APIs produce identical signed URLs.sessionTokenerror message typo:credentials.zig:159corrected from"bucket"to"sessionToken".- Test isolation: Subprocess clears 12 AWS/S3 env vars; stderr asserted with
not.toContain("error:"). errdefervsdefer: Correctly applied only to presign paths where ownership no longer transfers; revert of the overly broad change confirmed correct behavior.
Security Risks
No security-sensitive code paths affected beyond the existing S3 credential handling. The fix does not change how credentials are validated or stored.
Level of Scrutiny
Moderate — this is a targeted bug fix in S3 presign logic with a regression test that exercises the exact crash path. The change is well-scoped and all review iterations have been addressed.
Other Factors
CI failures across all builds are unrelated to S3 (bundler_compile.test.ts timeouts, webview-chrome.test.ts). No S3-related failures in any build. No TODO/FIXME/HACK in added lines.
robobun
force-pushed
the
farm/380c9ed5/fix-s3-presign-crash
branch
from
a5e4342 to
29db14d
Compare
constructS3FileWithS3CredentialsAndOptions copies the path via toThreadSafe(), so the original PathLike must always be cleaned up. Using errdefer instead of defer leaked the path on the success path. Also adds defer path.deinit() to S3Client.staticFile which was missing cleanup entirely, and asserts on stderr in the test.
PathLike uses move semantics: after toThreadSafe() in initS3, the original PathLike is invalid. Calling deinit() on the success path (via defer) causes use-after-free. The errdefer is correct — it only runs when blob construction fails, before ownership transfers.
The sessionToken type validation was reporting "bucket" instead of "sessionToken" due to a copy-paste error from the bucket validation block above it.
The old blob-based path initialized acl and storage_class to null in getPresignUrlFrom, only populating them from explicit options. Pass null instead of ptr.acl/ptr.storage_class to match.
The old getPresignUrlFrom only called s3.getCredentialsWithOptions (which used this.acl as default) when extra_options was non-null. Without options, acl/storage_class defaulted to null. Match this by conditionally passing ptr.acl/ptr.storage_class.
robobun
force-pushed
the
farm/380c9ed5/fix-s3-presign-crash
branch
from
7aee613 to
d230939
Compare
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed.
Extended reasoning...
Overview
This PR fixes a SIGFPE crash in Bun.s3.presign() / S3Client.presign() by replacing the temporary blob construction + deferred cleanup path with a direct credential resolution and signing flow (presignFromCredentials). Files changed: S3Client.zig, S3File.zig, s3/credentials.zig, and a new regression test.
Security risks
No security concerns. The change does not affect authentication logic — it routes around a crash in the error path, not the signing path itself. All credential resolution still goes through the same getCredentialsWithOptions + signRequest functions.
Level of scrutiny
This required close scrutiny across multiple review iterations because the fix restructures ownership semantics and path normalization in Zig. All issues I raised have been resolved: errdefer → defer for path cleanup (ownership never transfers in the new path), conditional acl/storage_class defaulting to match the old getPresignUrlFrom behavior (only passed when has_options is true), complete path normalization matching Store.S3.path() (s3Path() + trailing backslash strip + leading slash strip), content_encoding parity in both signing paths, the sessionToken error message copy-paste fix in credentials.zig, typo fix (greather → greater), and test isolation via subprocess with 12 AWS/S3 env vars cleared.
Other factors
CI shows no S3-related failures across any build in this PR. The regression test correctly exercises all three presign API surfaces with Bun.gc(true) to trigger the crash on unfixed code. All inline comments from prior review rounds are resolved.
|
Superseded by #30495 (same fix against current main, minimal diff, deterministic test). |
2 participants
Calling
Bun.s3.presign(),new Bun.S3Client().presign(), orBun.S3Client.presign()without configured S3 credentials caused a crash (SIGFPE / division by zero inStringImpl::costDuringGC) during garbage collection.The root cause: when
signRequestreturnederror.MissingCredentials, the error was thrown viathrowSignErrorwhile a stack-allocated blob with deferred cleanup was still alive. The interaction between the error throw path and the deferred blob/store cleanup corrupted the exception scope chain, leaving aStringImplwith a zero refcount that was later visited during GC.The fix avoids blob construction entirely for the path-based presign case: credentials are resolved via
getCredentialsWithOptionsfirst, thenpresignFromCredentialssigns the request directly. This eliminates the problematic interaction between deferred blob cleanup and error throwing fromsignRequest.Minimal repro:
Verification (robobun, iteration 11): Build #41219 on commit 33ddd8a is pending. Prior Build #41193 failures were all in bundler_compile.test.ts (Linux timeouts/code 1) and webview-chrome.test.ts — completely unrelated to S3 (PR only touches S3Client.zig, S3File.zig, and the new S3 test). No TODO/FIXME/HACK in added lines. Diff reviewed: S3Client.zig errdefer→defer path leak fix + direct credential resolution bypassing blob construction; S3File.zig same pattern + new presignFromCredentials helper including content_encoding, typo fix greather→greater, content_encoding added to getPresignUrlFrom for parity. Test spawns isolated subprocess clearing 12 AWS/S3 env vars, exercises all 3 presign APIs + Bun.gc(true), asserts ERR_S3_MISSING_CREDENTIALS and exit 0 — would SIGFPE on main. All 4 CodeRabbit actionable items resolved.
Verification (robobun, final review): CI Build #41316 on latest commit be37445 is in progress (Lint JavaScript passed, Format/Buildkite pending). Prior Build #41219 on 33ddd8a had failures in darwin/linux test-bun jobs, not in any S3 test. No TODO/FIXME/HACK in added lines. Diff verified: S3Client.zig presign() bypasses blob construction, resolves credentials via getCredentialsWithOptions + signs via presignFromCredentials directly; all errdefer→defer for PathLike cleanup is correct (initS3 copies path via toThreadSafe, original must always free). S3File.zig same direct-credential pattern + new presignFromCredentials helper with content_encoding, getPresignUrlFrom gets content_encoding for parity, greather→greater typo fixed. Test is a genuine regression test: subprocess clears 12 AWS/S3 env vars, exercises Bun.s3.presign + new S3Client().presign + S3Client.presign, asserts ERR_S3_MISSING_CREDENTIALS for each, forces Bun.gc(true) (the crash trigger on main), asserts exit 0 and no error in stderr. Both CodeRabbit actionable items (typo + content_encoding) resolved in current diff.
Verification (robobun, iteration 7): Commit 66a46dd (CI retry, zero code diff from dae75e4). Build #41372 pending; prior Build #41357 failures were all bundler_compile.test.ts timeouts/code-1 on Linux — completely unrelated to S3. Lint JavaScript passed. No TODO/FIXME/HACK in added lines. Diff: S3Client.zig and S3File.zig presign() bypass blob construction, resolve credentials via getCredentialsWithOptions, sign via new presignFromCredentials helper; errdefer→defer correct since path is never consumed. content_encoding added to both signing paths, greather→greater typo fixed. Regression test spawns subprocess with 12 AWS/S3 env vars cleared, exercises all 3 presign APIs + Bun.gc(true), asserts ERR_S3_MISSING_CREDENTIALS and exit 0 — would SIGFPE on main.
Verification (robobun, iteration 7 final): CI Build #41378 on commit a53d6a7 is pending. Prior builds (41357, 41372) had failures only in generic test-bun steps (bundler_compile.test.ts timeouts on Linux aarch64/x64, Darwin expired) — zero S3-related failures across all builds. Lint JavaScript and Format passed on prior commits. No TODO/FIXME/HACK in added lines. Diff (3 files, 190 lines): S3Client.zig and S3File.zig presign() bypass blob construction and resolve credentials directly via getCredentialsWithOptions + new presignFromCredentials helper, eliminating the deferred-blob-cleanup/error-throw interaction that caused SIGFPE. errdefer→defer correct since path ownership never transfers. content_encoding added to both signing paths for parity. greather→greater typo fixed. Latest commit adds trailing backslash stripping to match Store.S3.path() normalization. Regression test spawns isolated subprocess with 12 AWS/S3 env vars cleared, exercises all 3 presign APIs (Bun.s3, new S3Client(), S3Client.presign) + Bun.gc(true), asserts ERR_S3_MISSING_CREDENTIALS and exit 0 — would SIGFPE on main. CodeRabbit reviews resolved.
Verification (robobun, iteration 14): CI on commit eace518 — Lint JavaScript passed, Format and Buildkite Build #41470 pending. Prior builds across this PR had zero S3-related failures; all failures were in bundler_compile.test.ts (Linux timeouts) and webview-chrome.test.ts — completely unrelated. No TODO/FIXME/HACK in added lines. Diff (4 files, 203 lines): S3Client.zig and S3File.zig presign() bypass blob construction entirely, resolving credentials via getCredentialsWithOptions and signing via new presignFromCredentials helper — eliminates deferred-blob-cleanup/error-throw interaction for ALL signRequest errors, not just MissingCredentials. errdefer→defer correct since path ownership never transfers. content_encoding added to both signing paths. greather→greater typo fixed. credentials.zig fixes pre-existing copy-paste bug (bucket→sessionToken in error message). Regression test spawns isolated subprocess with 12 AWS/S3 env vars cleared, exercises all 3 presign APIs + Bun.gc(true), asserts ERR_S3_MISSING_CREDENTIALS and exit 0 — would SIGFPE on main. CodeRabbit actionable items resolved. Claude reviews all LGTM.
Verification (robobun, iteration 21): CI Build #41543 on HEAD 45e975d pending (Lint JavaScript passed). Prior Build #41535 on 29db14d completed with 3 failures: webview-chrome.test.ts and regression/issue/24364.test.ts — zero S3-related failures. No TODO/FIXME/HACK in added lines. Diff (4 files): S3Client.zig and S3File.zig presign() bypass blob construction, resolve credentials via getCredentialsWithOptions, sign via presignFromCredentials helper; errdefer→defer correct since path never transfers ownership. content_encoding added to both signing paths. greather→greater typo fixed. credentials.zig fixes copy-paste bug (bucket→sessionToken). Regression test spawns subprocess with 12 AWS/S3 env vars cleared, exercises all 3 presign APIs + Bun.gc(true), asserts ERR_S3_MISSING_CREDENTIALS and exit 0 — would SIGFPE on main. CodeRabbit actionable items resolved.
Verification (robobun, iteration 24): CI Build #41572 on HEAD a33b019 in progress (Lint JavaScript passed). Prior Build #41559 on 2d52389 (identical code) failures: node-http-backpressure-max, webview, test-stdin-pipe-large, regression/issue/24364, bun-types — zero S3-related. Diff verified: S3Client.zig and S3File.zig presign() bypass blob construction, resolve credentials via getCredentialsWithOptions, sign via presignFromCredentials; errdefer→defer correct since path never transfers ownership. credentials.zig fixes copy-paste bug. Regression test exercises all 3 presign APIs + Bun.gc(true), asserts ERR_S3_MISSING_CREDENTIALS and exit 0. No TODO/FIXME/HACK.