Repository navigation
Fix missing log.deinit() in TOML.parse - #28492
Conversation
Add defer log.deinit() to TOMLObject.parse to free the logger's message list after use. All peer parsers (JSONC, JSON5, YAML) already do this. Without it, the log's internal ArrayList leaks on every TOML.parse call that produces parse errors.
WalkthroughReplace argument slicing with 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}
There was a problem hiding this comment.
LGTM — straightforward memory leak fix matching the established pattern of peer parsers.
Extended reasoning...
Overview
This PR modifies src/bun.js/api/TOMLObject.zig to add defer log.deinit() after Log.init in TOML.parse, and updates argument access from the deprecated callframe.arguments_old(1).slice() to callframe.argument(0). A regression test in test/js/bun/resolve/toml/toml-parse.test.ts is included.
Security Risks
No security risks. The change is purely about resource cleanup (memory deallocation) and a mechanical API modernization. No auth, crypto, or permissions code is touched.
Level of Scrutiny
This is a low-risk, targeted fix. The pattern (defer log.deinit()) is already present in all peer parsers (JSONC, JSON5, YAML) as noted in the PR description — this is simply bringing TOML.parse in line with them. The argument access change is a direct 1:1 simplification with no behavioral difference.
Other Factors
The only bug flagged is pre-existing (the throwValue(log.toJS()) path on printJSON failure returning undefined), not introduced by this PR. It is surfaced via an inline comment for the author to consider. The PR itself is correct and includes a test that validates the crash-free behavior after the fix.
| defer ast_scope.exit(); | ||
|
|
||
| var log = logger.Log.init(default_allocator); | ||
| const arguments = callframe.arguments_old(1).slice(); | ||
| if (arguments.len == 0 or arguments[0].isEmptyOrUndefinedOrNull()) { | ||
| defer log.deinit(); | ||
| const input_value = callframe.argument(0); | ||
| if (input_value.isEmptyOrUndefinedOrNull()) { | ||
| return globalThis.throwInvalidArguments("Expected a string to parse", .{}); | ||
| } | ||
|
|
||
| var input_slice = try arguments[0].toSlice(globalThis, bun.default_allocator); | ||
| var input_slice = try input_value.toSlice(globalThis, bun.default_allocator); | ||
| defer input_slice.deinit(); | ||
| const source = &logger.Source.initPathString("input.toml", input_slice.slice()); | ||
| const parse_result = TOML.parse(source, &log, allocator, false) catch |err| { |
There was a problem hiding this comment.
🟣 Pre-existing bug: when printJSON fails, log.toJS() returns js_undefined (because the log has no messages after a successful parse), so throwValue(undefined) throws JavaScript undefined instead of a meaningful error. The string "Failed to print toml" is never surfaced to the user. Fix by using globalThis.throwValue(globalThis.createErrorInstance("Failed to print toml", .{})) instead.
Extended reasoning...
Analysis
What the bug is: In , the failure path (line 58-59) calls . This silently throws JavaScript instead of a real error object.
The specific code path: When fails (e.g., due to OOM), execution reaches the catch block at line 58. At this point, has already succeeded — any parse errors would have been caught by the earlier catch block at line 46. Since the log only accumulates messages during parsing, at the print failure point.
Why existing code does not prevent it: in has an explicit early return at line 731: when there are no messages. This is designed for the parse-error path where messages are expected. In the print-error path there are never any messages, so it always returns . The "Failed to print toml" string is only used as the AggregateError message in the multi-message branch, which is never reached here.
Impact: Any user hitting an OOM or other failure during the JSON serialization of a parsed TOML document will receive as the thrown error value rather than an object with a useful message. Code like would print instead of "Failed to print toml", making debugging difficult.
Step-by-step proof:
- Call under memory pressure so fails.
- Execution enters the catch block at line 58: .
- because succeeded — no parse errors were recorded.
- hits the branch in logger.zig:731 and returns .
- throws in JavaScript.
- The caught error is ; is also .
How to fix it: Replace the print-failure throw with a direct error instance:
return globalThis.throwValue(globalThis.createErrorInstance("Failed to print toml", .{}));This is a pre-existing bug not introduced by this PR (the PR only adds and modernizes argument access), but the PR modifies this exact function, making it an appropriate time to address it.
|
Good catch on the pre-existing printJSON failure path — that's a separate issue though, keeping this PR scoped to the log leak fix. |
There was a problem hiding this comment.
LGTM — the defer log.deinit() fix is correct and follows the established pattern of all peer parsers (JSONC, JSON5, YAML).
Extended reasoning...
Overview
This PR touches three files: src/bun.js/api/TOMLObject.zig (adds defer log.deinit() and modernizes argument access), test/internal/ban-limits.json (decrements .arguments_old( count by 1), and a new test file for Bun.TOML.parse.
Security Risks
No security-sensitive code is touched. This is a memory management fix in a parser utility function with no auth, crypto, or permission-related changes.
Level of Scrutiny
Low — this is a one-line semantic fix (defer log.deinit()) that directly mirrors the pattern already present in JSONCObject.zig:31, JSON5Object.zig:70, and YAMLObject.zig:951. The argument modernization from callframe.arguments_old(1).slice()[0] to callframe.argument(0) matches JSONCObject.zig:32 exactly. Both changes are mechanical and follow well-established codebase patterns.
Other Factors
The bug hunter flagged the second test as having no assertions and being unable to detect regressions of the Zig memory leak. This is a valid nit — the test passes unconditionally since Bun.gc(true) only triggers the JS GC and cannot observe a Zig heap leak. However, the first test does provide real regression coverage (asserts that a throw occurs), and the second test does no harm. The pre-existing printJSON failure path bug I noted in a prior comment is out of scope per the PR author. CI failures are in pre-existing vendor library warnings (libuv, tinycc) unrelated to this change.
|
The |
![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 `@test/js/bun/resolve/toml/toml-parse.test.ts`:
- Line 4: The test currently passes the SharedArrayBuffer constructor to
Bun.TOML.parse which only verifies a function object throws; update the test to
pass an actual SharedArrayBuffer instance (e.g., new SharedArrayBuffer(10)) to
Bun.TOML.parse so it validates that binary SharedArrayBuffer data is rejected;
modify the line using Bun.TOML.parse and SharedArrayBuffer to instantiate the
buffer before calling parse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69582eab-5a65-4116-942f-eb5b8d992304
📒 Files selected for processing (1)
test/js/bun/resolve/toml/toml-parse.test.ts
| import { expect, test } from "bun:test"; | ||
|
|
||
| test("Bun.TOML.parse with non-string input throws", () => { | ||
| expect(() => Bun.TOML.parse(SharedArrayBuffer as any)).toThrow(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider testing with a SharedArrayBuffer instance instead of the constructor.
Passing the SharedArrayBuffer constructor function tests that a function object throws, but a more meaningful test would use an actual buffer instance like new SharedArrayBuffer(10) to verify that binary data types are properly rejected.
📝 Suggested improvement
- expect(() => Bun.TOML.parse(SharedArrayBuffer as any)).toThrow();
+ expect(() => Bun.TOML.parse(new SharedArrayBuffer(10) as any)).toThrow();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(() => Bun.TOML.parse(SharedArrayBuffer as any)).toThrow(); | |
| import { expect, test } from "bun:test"; | |
| test("Bun.TOML.parse throws for non-string inputs", () => { | |
| expect(() => Bun.TOML.parse(new SharedArrayBuffer(10) as any)).toThrow(); | |
| expect(() => Bun.TOML.parse(undefined as any)).toThrow(); | |
| expect(() => Bun.TOML.parse(null as any)).toThrow(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/resolve/toml/toml-parse.test.ts` at line 4, The test currently
passes the SharedArrayBuffer constructor to Bun.TOML.parse which only verifies a
function object throws; update the test to pass an actual SharedArrayBuffer
instance (e.g., new SharedArrayBuffer(10)) to Bun.TOML.parse so it validates
that binary SharedArrayBuffer data is rejected; modify the line using
Bun.TOML.parse and SharedArrayBuffer to instantiate the buffer before calling
parse.
There was a problem hiding this comment.
LGTM — the defer log.deinit() fix is correct and matches all peer parsers; the test now has proper assertions after dropping the assertion-less GC test.
Extended reasoning...
Overview
This PR touches src/bun.js/api/TOMLObject.zig (adds defer log.deinit(), switches to callframe.argument(0)), test/internal/ban-limits.json (decrements arguments_old count from 263 to 262), and adds a new test file with three expect().toThrow() assertions.
Security risks
None. The change only adds a cleanup defer and modernizes a callframe accessor. No auth, crypto, or permission code is involved.
Level of scrutiny
Low. The fix is mechanical: every peer parser (JSONCObject.zig, JSON5Object.zig, YAMLObject.zig) pairs Log.init with defer log.deinit(). The omission in TOML.parse was the only outlier. The log.toJS + defer log.deinit() ordering is safe (same pattern in JSONCObject.zig). The argument accessor change to callframe.argument(0) exactly matches JSONCObject.zig:32.
Other factors
My earlier concern about the assertion-less GC test was addressed by commit 12daf4b, which dropped that test and added null/undefined coverage to the first test. The pre-existing printJSON error path issue I noted was acknowledged as out of scope. The ban-limits.json decrement is accurate. No bugs were found by the bug hunting system.
TOML.parse was missing `defer log.deinit()` after `Log.init`, causing the logger's internal message ArrayList to leak on every call that produces parse errors. All peer parsers (JSONC, JSON5, YAML) already have this defer. Also modernized argument access from `callframe.arguments_old(1).slice()` to `callframe.argument(0)` to match the JSONC parser pattern. Crash fingerprint: `49da9789c26e29ab` --- Verification: Confirmed `main` has `Log.init` on line 30 with no matching `defer log.deinit()`. All peer parsers (JSONCObject.zig:31, JSON5Object.zig:70, YAMLObject.zig:951) pair `Log.init` with `defer log.deinit()`. The `log.toJS` + `defer log.deinit()` combination is safe (same pattern in JSONCObject.zig:31+44). Argument modernization to `callframe.argument(0)` matches JSONCObject.zig:32 exactly. Regression test exercises the error path with non-string input and GC. No TODO/FIXME/HACK in diff. Lint JS and pipeline passed; buildkite build oven-sh#41538 compiling (trivial one-line semantic change, adding a missing defer). --- Reviewer verification (robobun): Independently confirmed TOMLObject.zig on main (line 30) has `Log.init` with no `defer log.deinit()`, while peer JSONCObject.zig (lines 30-31) pairs them correctly. The diff adds exactly that missing defer and modernizes argument access to match JSONCObject.zig:32. No TODO/FIXME/HACK in added lines. Bot reviews (claude x2, coderabbit) all LGTM with no blocking issues. Test exercises the modified error paths with non-string inputs. CI: Lint JS pass, pipeline pass, buildkite build oven-sh#41557 compiling on commit 12daf4b. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
TOML.parse was missing `defer log.deinit()` after `Log.init`, causing the logger's internal message ArrayList to leak on every call that produces parse errors. All peer parsers (JSONC, JSON5, YAML) already have this defer. Also modernized argument access from `callframe.arguments_old(1).slice()` to `callframe.argument(0)` to match the JSONC parser pattern. Crash fingerprint: `49da9789c26e29ab` --- Verification: Confirmed `main` has `Log.init` on line 30 with no matching `defer log.deinit()`. All peer parsers (JSONCObject.zig:31, JSON5Object.zig:70, YAMLObject.zig:951) pair `Log.init` with `defer log.deinit()`. The `log.toJS` + `defer log.deinit()` combination is safe (same pattern in JSONCObject.zig:31+44). Argument modernization to `callframe.argument(0)` matches JSONCObject.zig:32 exactly. Regression test exercises the error path with non-string input and GC. No TODO/FIXME/HACK in diff. Lint JS and pipeline passed; buildkite build oven-sh#41538 compiling (trivial one-line semantic change, adding a missing defer). --- Reviewer verification (robobun): Independently confirmed TOMLObject.zig on main (line 30) has `Log.init` with no `defer log.deinit()`, while peer JSONCObject.zig (lines 30-31) pairs them correctly. The diff adds exactly that missing defer and modernizes argument access to match JSONCObject.zig:32. No TODO/FIXME/HACK in added lines. Bot reviews (claude x2, coderabbit) all LGTM with no blocking issues. Test exercises the modified error paths with non-string inputs. CI: Lint JS pass, pipeline pass, buildkite build oven-sh#41557 compiling on commit 12daf4b. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
TOML.parse was missing
defer log.deinit()afterLog.init, causing the logger's internal message ArrayList to leak on every call that produces parse errors. All peer parsers (JSONC, JSON5, YAML) already have this defer.Also modernized argument access from
callframe.arguments_old(1).slice()tocallframe.argument(0)to match the JSONC parser pattern.Crash fingerprint:
49da9789c26e29abVerification: Confirmed
mainhasLog.initon line 30 with no matchingdefer log.deinit(). All peer parsers (JSONCObject.zig:31, JSON5Object.zig:70, YAMLObject.zig:951) pairLog.initwithdefer log.deinit(). Thelog.toJS+defer log.deinit()combination is safe (same pattern in JSONCObject.zig:31+44). Argument modernization tocallframe.argument(0)matches JSONCObject.zig:32 exactly. Regression test exercises the error path with non-string input and GC. No TODO/FIXME/HACK in diff. Lint JS and pipeline passed; buildkite build #41538 compiling (trivial one-line semantic change, adding a missing defer).Reviewer verification (robobun): Independently confirmed TOMLObject.zig on main (line 30) has
Log.initwith nodefer log.deinit(), while peer JSONCObject.zig (lines 30-31) pairs them correctly. The diff adds exactly that missing defer and modernizes argument access to match JSONCObject.zig:32. No TODO/FIXME/HACK in added lines. Bot reviews (claude x2, coderabbit) all LGTM with no blocking issues. Test exercises the modified error paths with non-string inputs. CI: Lint JS pass, pipeline pass, buildkite build #41557 compiling on commit 12daf4b.