Fix double-free of path string in S3Client methods on error - #30465
Closed
robobun wants to merge 1 commit into
Closed
Fix double-free of path string in S3Client methods on error#30465robobun wants to merge 1 commit into
robobun wants to merge 1 commit into
Claude / Claude Code Review
completed
May 10, 2026 in 14m 23s
Code review found 1 important issue
Found 5 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/webcore/S3File.zig:86-87 |
Double-free still reachable when construct* throws after initS3 |
| 🟡 Nit | test/js/bun/s3/s3-path-double-free.test.ts:52 |
Test checks stderr for "panic" — forbidden by repo guidelines |
Annotations
Check failure on line 87 in src/runtime/webcore/S3File.zig
claude / Claude Code Review
Double-free still reachable when construct* throws after initS3
This fix is incomplete: the same double-free still occurs if `constructS3FileWithS3CredentialsAndOptions` / `constructS3FileInternalStore` throws *after* `Blob.Store.initS3` has taken ownership of the path — e.g. when `try opts.getTruthyComptime(globalObject, "type")` or `try file_type.toSlice(...)` throws. In that case the inner `errdefer store.deinit()` frees the path, the error propagates, and the caller's `errdefer path.deinit()` fires again because this neutralization line is never reached.
Check warning on line 52 in test/js/bun/s3/s3-path-double-free.test.ts
claude / Claude Code Review
Test checks stderr for "panic" — forbidden by repo guidelines
nit: per the root `CLAUDE.md` testing guidelines, tests should not assert that stderr does not contain `"panic"` — these checks never fail in CI. The `signalCode`, `stdout == "ok"`, and `exitCode === 0` assertions below already catch the crash, so this line can be dropped.
Loading