Skip to content

json: a single parse root that owns the error and recoverable flags - #798

Merged
InauguralPhysicist merged 1 commit into
InauguralSystems:mainfrom
Nitjsefnie-OSC:fix/json-parse-err-reset-777
Aug 2, 2026
Merged

json: a single parse root that owns the error and recoverable flags#798
InauguralPhysicist merged 1 commit into
InauguralSystems:mainfrom
Nitjsefnie-OSC:fix/json-parse-err-reset-777

Conversation

@Nitjsefnie

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #777. Adds eigs_json_parse_root, which owns the lifetime of both g_json_parse_err and the g_json_parse_recoverable channel #778 introduced, and migrates all six entry points you named — json_decode, json_path, and the four ext_http.c sites — onto it. Callers no longer touch the globals, so the wrapper becomes the single place that defines "fresh parse", which is the shape you prescribed on the issue.

The parser both sets and mid-parse checks g_json_parse_err, but only json_decode cleared it. A stale flag hit the array guard and the object guard, so the next same-thread parse truncated arrays after the first element and emptied objects on the first key.

Strict json_decode behaviour is unchanged — same flags cleared at the same point, same raise check. Verified rather than assumed: a 67-input differential corpus, including stateful poison/raise/recoverable sequences, is byte-identical before and after, as are all four other test_json*.eigs files.

Changes

  • src/builtins.c — new eigs_json_parse_root; json_decode and json_path migrated.
  • src/ext_http.c — the four JSON entry points migrated.
  • tests/test_json_hard.eigs — JH118–JH127, placed mid-file after the JH81–93 lenient section.

Testing

  • Build clean; full suite 3348/3348 passed, 0 failed; make asan exit 0 with the asan+ubsan binary clean under detect_leaks=1 across the test file, both demos and the corpus.
  • The poisoning, before and after. Before: unterminated / (empty) / (empty) / 0. After: unterminated / bob / 30 / 3. The strict-raise variant is poisoned before and correct after in the same way.
  • JH118–JH126 bite. With the production files reverted to upstream and the tests kept, all nine fail standalone with their own assert labels.
  • JH127 is different, and worth stating precisely. It pins the g_json_parse_recoverable reset: delete only that line from the wrapper and it fails, keep it and it passes. It does not fail against upstream, and cannot — upstream's builtin_json_decode cleared both flags on its own entry, so the recoverable-stale case only becomes reachable once the resets move into the wrapper. It is a regression guard for this PR's own structure, not a reproduction of the original bug.
  • The JH118–JH127 block ends with an unnumbered cleanup decode so the pre-existing JH94–97 don't become collateral failures when the block runs against unfixed code. That guard is verified both ways: with it, JH94–97 pass on unfixed code; without it, JH96 fails.

Two disclosures. make asan compiles ext_http.c out, so the four migrated HTTP sites had no local sanitizer coverage — CI's asan-http leg is what covers them. And I've left CHANGELOG.md alone, since the convention across recent merges here is mixed; happy to add an entry if you'd like one.

Related Issues and Pull Requests

Fixes #777

Bugs Discovered

Your prescription named six entry points. Enumerating them across the tree turned up a seventh — eigs_json_lint_allow_for in src/lint.c — which is a live lenient root that also never clears the flag. I've left it unmigrated because it's outside what you specified and it isn't reachable from any shipped binary (--lint is one file per process, and the LSP goes through lint_collect); it is reachable for embedders using the public C API, and #797 has the reproducer. Say the word if you'd rather it came into this PR.

Checklist

  • make test passes locally — full suite 3348/3348 passed, 0 failed; tests/test_json_hard.eigs run directly, rc=0
  • New builtins have signature comments and docs in docs/BUILTINS.md — n/a, no new builtins; eigs_json_parse_root is internal
  • New library functions follow conventions in docs/STDLIB.md — n/a
  • New examples have a comment header explaining what they demonstrate — n/a
  • CHANGELOG.md updated (if user-facing change) — not done, see the note above; happy to add one

Generated by Claude Opus 5 (brief, review), Kimi K3 (implementation, verification)

…lSystems#777)

g_json_parse_err was only reset by builtin_json_decode, but the parser
checks it mid-parse (object first-key and array element guards), so one
malformed parse poisoned the NEXT parse in the same thread: lenient
callers (json_path, ext_http) saw well-formed objects decode to empty
dicts and arrays truncate to one element.

Per the maintainer's prescription on InauguralSystems#777, a single non-recursive
eigs_json_parse_root(s, pos) now owns the flags' lifetime: it clears
both g_json_parse_err and g_json_parse_recoverable and delegates to
eigs_json_parse_value. All six entry points migrate to it (json_decode,
json_path, the four ext_http.c sites) and stop touching the globals
directly. Strict json_decode behaviour is unchanged: the wrapper clears
exactly the flags json_decode cleared, immediately before the same
parse call, and the post-parse raise check is untouched.

Regression tests JH118-JH127 in tests/test_json_hard.eigs cover both
poison paths (lenient-after-lenient, lenient-after-strict-raise) and
both truncation shapes (object-empty, array-truncate); each fails on
unfixed code. JH127 pins the recoverable-channel half of the reset: a
clean strict decode after a recoverable lenient parse must not raise,
which fails if the g_json_parse_recoverable reset is dropped from the
wrapper.

Co-Authored-By: Kimi K3 <noreply@kimi.com>

@InauguralPhysicist InauguralPhysicist left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified locally on top of your branch:

  • test_json_hard.eigs (JH118–127) green under ASan (detect_leaks=1), rc=0.
  • Planted fault reproduced: with src/builtins.c/src/ext_http.c/src/eigenscript.h reverted to main and your tests kept, JH118 fails exactly as claimed ("lenient json_path after malformed json_path") — the poison shape is real and the tests bite.
  • The single-owner wrapper is the right structure: the parser checks g_json_parse_err mid-parse, so per-caller resets were always going to rot; eigs_json_parse_root makes "fresh parse" a property of the entry point instead of caller discipline. JH127's precise framing (guards the wrapper's own recoverable reset, not the original bug) is exactly the honesty this repo wants in test comments.
  • Good catch on the seventh root (#797) — agreed it's correctly out of scope here with the issue filed. One nit for whoever picks that up: the new eigenscript.h comment says parse_root is "the ONLY entry point", which becomes true only once #797 lands; fine to leave as aspiration since the issue tracks it.

CI 16/16 green including asan-http (which covers your four migrated HTTP sites — right call flagging that make asan alone doesn't). Merging. Two for two today — thanks.

@InauguralPhysicist
InauguralPhysicist merged commit c681517 into InauguralSystems:main Aug 2, 2026
16 checks passed
InauguralPhysicist added a commit that referenced this pull request Aug 2, 2026
)

Both merged without CHANGELOG entries (disclosed in the PR bodies);
folded here rather than as docs-only pushes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
InauguralPhysicist added a commit that referenced this pull request Aug 2, 2026
…le-root hook (#728) (#800)

* jit: gate thunk invocation under MT; emitted decref runs the #307 hook (#728)

Two fixes for #728, both red-then-green:

1. Thunk invocation now carries the same !g_vm_multithreaded gate as the
   OSR site. #296 only gated COMPILING under MT — a chunk compiled on
   main before the first spawn stayed runnable from workers, and every
   thunk epilogue writes the shared chunk->jit_advance while in-thunk
   helpers fill the shared env_ic/const_hashes caches ungated (the
   interpreter's twins are #297-gated). Repro: warm-then-spawn-parallel,
   7 TSan warnings on the IC fill (the epilogue store is uninstrumented
   emitted code, which is why the issue had no repro). Also: OP_DISPATCH's
   exec_count++ picks up the #297 gate its OP_CALL twin had, and
   jit_helper_call refuses nested thunk entry after a mid-thunk spawn
   flips the flag. Suite [129] + the test_tsan.sh race-free slice.

2. The emitted inline decref now runs the gc_note_possible_root hook on
   a surviving LIST/DICT, like slot_decref. The gap was masked because
   C-path decrefs register every cycle at creation — but
   gc_collect_cycles UNBUFFERS live candidates, so a cycle that survived
   a collection and then lost its last external ref inside an
   OSR-compiled loop was invisible to the exit collector (136 bytes
   leaked, clean under EIGS_JIT_OFF=1). The hook arm is emitted only at
   sites that drop arbitrary values (OP_POP, SET_LOCAL swap, inline
   SET_NAME); binop commit decrefs and INDEX_SET stay hookless — their
   type guards bail non-num/non-buffer operands before any commit decref
   runs, and the arm there cost a measured ~12% on bench_dmg_shape
   (flat with the split). Strict leak gate: suite [130].

Validation: jit-smoke green (new gc_note_possible_root stub); release
suite 3350/3350; ASan+UBSan detect_leaks=1 suite 3354/3354 leak tally 0;
test_tsan.sh 12/12 with the seeded race still caught; both new tests
fail against pre-fix HEAD (planted fault); bench_dmg_shape / bench_idxset
medians identical to base.

Closes #728

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* changelog: record #776/#777 (first external contributions, PRs #799/#798)

Both merged without CHANGELOG entries (disclosed in the PR bodies);
folded here rather than as docs-only pushes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

g_json_parse_err is never reset by lenient JSON callers, so one malformed parse poisons the next json_path in the same thread

2 participants