From 7d39af30da8351b3520b9cae0b70c18f3051f3d6 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Fri, 5 Jun 2026 22:16:32 -1000 Subject: [PATCH 1/2] fix(#716): parseerror signature now const char *; drop Pascal-string 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 --- Common/headers/langinternal.h | 2 +- Common/source/langerror.c | 29 ++++++++++++++---- Common/source/langparser.c | 4 +-- Common/source/langparser.y | 4 +-- tests/parser_tests.c | 56 +++++++++++++++++++++++++++++++++++ 5 files changed, 85 insertions(+), 10 deletions(-) diff --git a/Common/headers/langinternal.h b/Common/headers/langinternal.h index 2d28fd175..a882b8e06 100644 --- a/Common/headers/langinternal.h +++ b/Common/headers/langinternal.h @@ -424,7 +424,7 @@ extern boolean langrundialog (hdltreenode, tyvaluerecord *); extern boolean langdialogstart (void); /*langdialog.c*/ -extern void parseerror (bigstring); /*langerror.c*/ +extern void parseerror (const char *); /*langerror.c*/ extern hdlerrorstack langerrorgetstack (void); /*langerrorwindow.c*/ diff --git a/Common/source/langerror.c b/Common/source/langerror.c index 2bc1614dd..91197a051 100644 --- a/Common/source/langerror.c +++ b/Common/source/langerror.c @@ -32,6 +32,7 @@ #include "ops.h" #include "resources.h" #include "strings.h" +#include "logging.h" #include "langinternal.h" @@ -160,11 +161,29 @@ void langostypeparamerror (short stringnum, OSType x) { } /*langostypeparamerror*/ -void parseerror (bigstring bs) { - /* 2025-12-09 Codex: bs is a C string from yacc/lex; copy to Pascal safely. */ - bigstring bscopy; /* must work on a copy */ - - copyctopstring ((const char *) bs, bscopy); +void parseerror (const char *cs) { + /* + * Issue #716 item 1: parseerror takes a NUL-terminated C string from + * yacc/lex (see yyerror in langparser.y / langparser.c). Convert to a + * Pascal bigstring for lang3paramerror's parsedialogstring formatter. + * + * Pre-fix the prototype was `bigstring bs`, so yyerror cast its + * `const char *s` to `(ptrstring) s` and this function cast it back + * to `(const char *)`. The double cast laundered the real type + * through a misleading Pascal-typed parameter for no reason. + * + * copyctopstring (post-#707) clamps payload to 255 bytes and returns + * false on truncation. Long syntax-error messages from bison (e.g. + * the multi-fragment "syntax error, unexpected ... expecting ..." + * variants) can exceed that; surface as a warning so the truncation + * is observable rather than silent. + */ + bigstring bscopy; + + if (!copyctopstring (cs, bscopy)) { + log_warn (LOG_COMP_PARSE, + "parseerror: yacc message exceeded 255 bytes and was truncated"); + } langparamerror (parsererror, bscopy); } /*parseerror*/ diff --git a/Common/source/langparser.c b/Common/source/langparser.c index 3ac84b4d8..8df598885 100644 --- a/Common/source/langparser.c +++ b/Common/source/langparser.c @@ -3711,7 +3711,7 @@ int yyerror (const char *s) { clearbytes (&parseresult, (long) sizeof (parseresult)); */ - parseerror ((ptrstring) s); - return 0; + parseerror (s); + return 0; } /*yyerror*/ diff --git a/Common/source/langparser.y b/Common/source/langparser.y index e33edb14d..f65ef92e4 100644 --- a/Common/source/langparser.y +++ b/Common/source/langparser.y @@ -1585,6 +1585,6 @@ int yyerror (const char *s) { clearbytes (&parseresult, (long) sizeof (parseresult)); */ - parseerror ((ptrstring) s); - return 0; + parseerror (s); + return 0; } /*yyerror*/ diff --git a/tests/parser_tests.c b/tests/parser_tests.c index 8eff300a6..59e480c12 100644 --- a/tests/parser_tests.c +++ b/tests/parser_tests.c @@ -6,6 +6,7 @@ #include "memory.h" #include "strings.h" #include "lang.h" +#include "langinternal.h" #include "tablestructure.h" #include "logging.h" #include "../portable/wptext_portable.h" @@ -227,6 +228,58 @@ static void test_quirk_compile_smoke(void) { * to work — earlier parser fixes (PR #609) broke this path and produced * 43 integration regressions. */ +/* + * Issue #716 item 1: parseerror used to take `bigstring` and the only caller + * (yyerror in langparser.c) cast its `const char *s` to `(ptrstring) s` to + * match. parseerror then cast it back to `(const char *)` to feed + * copyctopstring. The cast pair laundered the real type for no reason. Post + * fix: parseerror takes const char * directly. These tests drive the + * parse-error path with malformed input so that the yyerror -> parseerror + * -> copyctopstring chain is exercised end-to-end. They guard against + * regressions where someone "tidies" the signature back to bigstring. + */ +static void test_parseerror_short_message_does_not_crash(void) { + /* Trailing operator triggers a short bison "syntax error" message. */ + assert(!compile_only("1 + ")); +} + +static void test_parseerror_long_token_does_not_crash(void) { + /* + * A single 300-char identifier followed by garbage produces a yyerror + * call whose message embeds the (truncated) token. The C-string-typed + * argument flows through parseerror -> copyctopstring which post-#707 + * clamps payload to 255 bytes. We only require that compilation fails + * cleanly without crashing or overflowing the bigstring. + */ + char prog[512]; + memset(prog, 'a', 300); + prog[300] = '\0'; + strcat(prog, " +"); /* trailing operator forces parse error */ + assert(!compile_only(prog)); +} + +static void test_parseerror_direct_long_message_well_defined(void) { + /* + * Call parseerror directly with a >255-byte C string. Pre-fix this + * input would have been cast to ptrstring at the only callsite and + * then back to const char *; the round-trip happened to be safe + * because the original was always a real C string, but the function + * signature lied about the type. Post-fix the signature matches + * reality and the truncation path is taken in a well-defined manner. + */ + char msg[400]; + memset(msg, 'x', 350); + msg[350] = '\0'; + /* + * langerrordisable suppresses the error-reporting callback so the + * test doesn't print a wall of noise; the body that copies the + * C string into a bigstring runs unconditionally. + */ + disablelangerror(); + parseerror(msg); + enablelangerror(); +} + static void test_cr_path_with_tab_indented_comments(void) { /* Top-of-file comments, tab-indented like startupScript. */ eval_handle_expect("// header\r\t// indented comment\r\t\t// more indent\rreturn (42)\r", @@ -263,6 +316,9 @@ int main(void) { TR_RUN(test_quirk3_consecutive_local_with_field_access); TR_RUN(test_quirk_compile_smoke); TR_RUN(test_cr_path_with_tab_indented_comments); + TR_RUN(test_parseerror_short_message_does_not_crash); + TR_RUN(test_parseerror_long_token_does_not_crash); + TR_RUN(test_parseerror_direct_long_message_well_defined); TR_SUMMARY(); return TR_EXIT_CODE(); From fdcf103e44fe9e671bdbcb9967e0b68fad137f1a Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Fri, 5 Jun 2026 22:24:07 -1000 Subject: [PATCH 2/2] =?UTF-8?q?fix(#716):=20address=20/gate=20P2=20?= =?UTF-8?q?=E2=80=94=20include=20bounded=20prefix=20in=20truncation=20log;?= =?UTF-8?q?=20comment=20style?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Common/source/langerror.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/Common/source/langerror.c b/Common/source/langerror.c index 91197a051..2fa492073 100644 --- a/Common/source/langerror.c +++ b/Common/source/langerror.c @@ -163,26 +163,24 @@ void langostypeparamerror (short stringnum, OSType x) { void parseerror (const char *cs) { /* - * Issue #716 item 1: parseerror takes a NUL-terminated C string from - * yacc/lex (see yyerror in langparser.y / langparser.c). Convert to a - * Pascal bigstring for lang3paramerror's parsedialogstring formatter. - * - * Pre-fix the prototype was `bigstring bs`, so yyerror cast its - * `const char *s` to `(ptrstring) s` and this function cast it back - * to `(const char *)`. The double cast laundered the real type - * through a misleading Pascal-typed parameter for no reason. - * - * copyctopstring (post-#707) clamps payload to 255 bytes and returns - * false on truncation. Long syntax-error messages from bison (e.g. - * the multi-fragment "syntax error, unexpected ... expecting ..." - * variants) can exceed that; surface as a warning so the truncation - * is observable rather than silent. - */ + 2026-06-05 #716 item 1: cs is a NUL-terminated C string from yacc/lex + (see yyerror in langparser.y / langparser.c); convert to Pascal bigstring + for lang3paramerror's parsedialogstring formatter. Pre-fix the prototype + was `bigstring bs`, so yyerror cast its `const char *s` to `(ptrstring) s` + and this function cast it back to `(const char *)` -- a double cast that + laundered the real type through a misleading Pascal-typed parameter for + no reason. copyctopstring (post-#707) clamps payload to 255 bytes and + returns false on truncation; long bison syntax-error messages (e.g. the + multi-fragment "syntax error, unexpected ... expecting ..." variants) + can exceed that, so surface a warning so the truncation is observable + rather than silent. + */ bigstring bscopy; if (!copyctopstring (cs, bscopy)) { log_warn (LOG_COMP_PARSE, - "parseerror: yacc message exceeded 255 bytes and was truncated"); + "parseerror: yacc message exceeded 255 bytes and was truncated: %.80s...", + cs); } langparamerror (parsererror, bscopy); } /*parseerror*/