Skip to content

fix(#716): parseerror signature now const char *; drop Pascal-string laundering#718

Merged
jsavin merged 2 commits into
developfrom
fix/716-parseerror-cstring-signature
Jun 6, 2026
Merged

fix(#716): parseerror signature now const char *; drop Pascal-string laundering#718
jsavin merged 2 commits into
developfrom
fix/716-parseerror-cstring-signature

Conversation

@jsavin

@jsavin jsavin commented Jun 6, 2026

Copy link
Copy Markdown
Owner

Summary

Closes item 1 of #716 — the langerror.c:167 cast pattern.

The pre-fix call chain laundered a real C string through a Pascal-typed parameter for no reason:

yyerror(const char *s)            <- yacc gives us a NUL-terminated C string
    parseerror((ptrstring) s)     <- cast to Pascal type to match the (wrong) decl
parseerror(bigstring bs)
    copyctopstring((const char *) bs, bscopy)  <- cast back so strlen "works"

The 2025-12-09 Codex comment on line 164 even said it out loud: "bs is a C string from yacc/lex". The type label was lying.

This PR drops the cast pair and makes the signature honest:

  • parseerror takes const char * (matches runtime_stubs_parser.c and yyerror's own argument)
  • the (ptrstring) s cast is dropped at the single callsite (in both langparser.y and the checked-in bison-generated langparser.c)
  • copyctopstring (still needed inside parseerror because the downstream lang3paramerror formatter wants a bigstring) now has its boolean return checked, with a log_warn(LOG_COMP_PARSE, ...) on truncation so a >255-byte bison message surfaces rather than silently clipping

Scope

This PR addresses only item 1 of #716. Items 2-6 are independent defects and remain open:

Adjacent audit

langerror.c contains no other bigstring/cstring cast patterns. Across the project:

$ rg 'copyctopstring\(\(const char \*\)' Common/source Common/headers portable frontier-cli tests
(no matches)

Test plan

  • make build — clean (6 pre-existing warnings, none new)
  • make unit (./tools/run_headless_tests.sh) — 589/589 pass (was 586; +3 new in parser_tests)
  • Direct ./tests/parser_tests — 12/12; the [parse-WARN] parseerror: yacc message exceeded 255 bytes and was truncated line is observed on the >255-byte direct-call test, confirming the new truncation surface works
  • cd tests && make test-integration — 2186 pass / 21 baseline failures (matches the baseline stated in PR test(#715): behavioral coverage for copyctopstring deep-stack defenses #717's commit log; the 21 failures are pre-existing html/tcp/startup flakiness, no parse/langerror failures)
  • Audit search: rg 'copyctopstring\(\(const char \*\).*bigstring|copyctopstring\(\(const char \*\)' ... returns zero hits

Regression coverage shape

Three new cases added to tests/parser_tests.c:

  1. test_parseerror_short_message_does_not_crash — compile "1 + " → yacc emits a short syntax-error message → exercised end-to-end
  2. test_parseerror_long_token_does_not_crash — 300-byte identifier + trailing operator → forces the bison error path with a long token name embedded
  3. test_parseerror_direct_long_message_well_defined — calls parseerror directly with a 350-byte C string; verifies the truncation-log warning fires in a well-defined manner

These would have compiled and run pre-fix (the cast pair was a round-trip), so they're regression guards against someone "tidying" the signature back to bigstring — which would silently re-introduce the laundering pattern.

🤖 Generated with Claude Code

jsavin and others added 2 commits June 5, 2026 22:16
…laundering

Issue #716 item 1: langerror.c:167 cast a bigstring parameter to
const char * before passing it to copyctopstring. The only caller
(yyerror in langparser.c) was casting its actual const char * to
(ptrstring) to match the dishonest signature. Two no-op casts laundering
the real type through a Pascal-typed parameter for no reason.

Fix shape:
- parseerror takes const char * (its real type), matching the portable
  stub (runtime_stubs_parser.c) and yyerror's own argument
- drop the (ptrstring) cast at yyerror's only invocation in both
  langparser.y and the checked-in bison-generated langparser.c
- check copyctopstring's boolean return (post-#707) and log a warning
  on truncation so long bison syntax-error messages are observable
  rather than silently clipped

Adjacent audit: langerror.c contains no other bigstring/cstring cast
patterns. rg "copyctopstring\(\(const char \*\)" across Common/source,
Common/headers, portable, frontier-cli, and tests now returns zero
hits.

Scope: this addresses only item 1 of issue #716. Items 2-6
(strings.c push-after-copy, shellsysverbs.c getenv, langsqlite errmsg,
tcpverbs INDIRECT sites, db.compactDatabase YAML coverage) are
separate defects and remain open.

Tests:
- new tests/parser_tests.c cases drive the yyerror -> parseerror chain:
  short message, long-token parse error, and direct parseerror call
  with a >255-byte C string. parser_tests now 12/12 (was 9). Direct
  run confirms the truncation warning fires on the >255-byte input.
- make build: clean (6 pre-existing warnings, none new)
- make unit: 589/589 pass (was 586; +3 new)
- make test-integration: 2186 pass / 21 baseline failures (matches
  pre-change baseline per PR #717 commit log; no parse/langerror
  failures in the diff)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…g; comment style

Bar-raiser P2 #1: truncation log_warn omitted the prefix of the message
that was truncated, limiting diagnostic value. Add bounded `%.80s...` so
operators can see what the bison error looked like without unbounded
log output.

Bar-raiser P2 #3: switch the introductory block comment from leading-stars
style to the date-prefixed plain block style used by the rest of
langerror.c.

P2 #2 (test names "does_not_crash" undersell what is asserted) declined:
consistent with existing style in tests/parser_tests.c.

Verified: make build clean, ./tests/parser_tests 12/12, full unit
589/589. Manual run confirms the new log line:

  [parse-WARN] langerror.c:183: parseerror: yacc message exceeded 255
  bytes and was truncated: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
  xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx...

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jsavin jsavin merged commit e67985d into develop Jun 6, 2026
jsavin added a commit that referenced this pull request Jun 6, 2026
…ard (#719)

* fix(#716): item 3 -- document headless getenv path; refactor legacy guard

#716 item 3 audit pointed at Common/source/shellsysverbs.c:594 as a
potential truncation surface for >255-byte env vars piped through
copyctopstring. Investigation showed that file is the legacy (full Mac
app) impl, NOT linked into the headless frontier-cli build. The real
headless path is tests/headless_sys_verbs.c::sysv_getenvironmentvariable,
which uses newfilledhandle + setheapvalue to return a heap-allocated
string handle -- no bigstring, no 255-byte limit, no truncation surface
by construction.

Changes:

- tests/headless_sys_verbs.c: expand the @implemented comment to document
  the heap-handle design intent and explicitly call out that this path
  has NO bigstring truncation surface, with a back-reference to #716
  item 3 so a future audit doesn't waste cycles re-flagging it. Marks
  the path as "do not add a bigstring length guard".

- Common/source/shellsysverbs.c (legacy/full-app path): collapse the
  pre-#707 pattern (strlen + manual >255 check + unguarded copyctopstring)
  into the canonical post-#707 pattern (copyctopstring's boolean return
  with langerrormessage on truncation). Functionally equivalent -- same
  user-visible "exceeds 255 characters" error message, same return
  contract. Matches frontier-cli/window_registry.c:116 and PR #718's
  langerror.c:163 fix. Reduces the code from 12 to 8 lines and removes
  the redundant strlen.

- tests/integration/sys_getenv_long_value_test.sh + tests/Makefile:
  new shell integration test that locks in the headless behavior --
  200-byte env var reads back at length 200 (control), 300-byte env var
  reads back at length 300 with content preserved verbatim (regression
  guard against someone mistakenly applying a bigstring guard to the
  heap-handle path). 3/3 GREEN.

This PR addresses ONLY #716 item 3. Items 2, 4, 5 remain open and will
be addressed in separate PRs.

Verified:
- make build: clean
- ./tools/run_headless_tests.sh: 589/589 pass
- tests/integration/sys_getenv_long_value_test.sh: 3/3 pass
- cd tests && make test-integration: TBD (in progress as of commit)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(#716): address /gate P2 -- tighten doc wording, add mid-buffer markers

Security reviewer P2: original doc said "Do not add a bigstring length
guard to this path" which could be misread as forbidding any
future overflow-safety check. Tighten to "Do not add a 255-byte
bigstring length guard" and explicitly note that an overflow-safe
larger-limit check remains future-compatible.

Security reviewer P2: shell test used a uniform 'y' payload; a
length-preserving mid-buffer corruption would have passed the
length-only + final-byte assertions. Upgrade payload to 150 'A's +
150 'B's and probe at byte 150 (A), 151 (B), and 300 (B). Catches
truncation, mid-buffer corruption, and tail corruption in a single
CLI invocation (the three probes are concatenated in one --batch -e
script).

Bar-raiser P2 items (3 invocation count, strip_warnings regex
robustness, pre-existing 108-col line) declined: current
diagnosability and behavior are correct; not blocking.

Test still 3/3 GREEN.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <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.

1 participant