fix(s3): double deref of path string on error in S3Client static methods - #29643
Closed
robobun wants to merge 1 commit into
Closed
fix(s3): double deref of path string on error in S3Client static methods#29643robobun wants to merge 1 commit into
robobun wants to merge 1 commit into
Claude / Claude Code Review
completed
Apr 23, 2026 in 9m 33s
Code review found 1 important issue
Found 4 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 0 |
| 🟣 Pre-existing | 1 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/bun.js/webcore/S3File.zig:86-87 |
Double-deref still possible if constructS3FileInternalStore throws after storing path |
| 🟣 Pre-existing | src/bun.js/webcore/S3File.zig:86-87 |
Same double-deref crash exists in S3Client instance methods |
Annotations
Check failure on line 87 in src/bun.js/webcore/S3File.zig
claude / Claude Code Review
Double-deref still possible if constructS3FileInternalStore throws after storing path
This fix is incomplete: the same double-deref still occurs if `constructS3FileWithS3Credentials` throws *after* `Blob.Store.initS3` has stored the path (e.g. when the post-`initS3` `try opts.getTruthyComptime(globalObject, "type")` throws). In that case `errdefer store.deinit()` derefs the path, the error propagates before the new neutralization line runs, and the caller's `errdefer path_or_blob.path.deinit()` derefs it again. Consider neutralizing `path` inside `constructS3FileWithS3Credentials
Check notice on line 87 in src/bun.js/webcore/S3File.zig
claude / Claude Code Review
Same double-deref crash exists in S3Client instance methods
The same double-deref pattern exists in the **instance** methods in `src/bun.js/webcore/S3Client.zig` (`presign`, `exists`, `size`, `stat`, `write`, `unlink`, lines ~144-259): `errdefer path.deinit()` followed by `constructS3FileWithS3CredentialsAndOptions` (which transfers path ownership into the store) followed by `defer blob.detach()`. This is pre-existing — the PR doesn't touch that file — but e.g. `new Bun.S3Client({accessKeyId:'x',secretAccessKey:'y',bucket:'b'}).presign('key', {expiresIn:
Loading