Jens-G commented on PR #3786:
URL: https://github.com/apache/thrift/pull/3786#issuecomment-5608348885
### Code review
Re-reviewed at `09c89d9cf`. Built the compiler and the standalone test suite
from the PR head and from the merge base (`59a949c80`), compared behaviour on
crafted inputs, and probed flex 2.6.4 directly with the scanner options this
file uses.
The rewrite does what the last round asked for, and it fixes one thing more
than the ticket needed:
- Unknown escapes stay invalid, and the diagnostic now names the offender
and the way out: `"a\Tb"` → `Invalid escape sequence '\T'. Use \\ for a literal
backslash.`
- The dead `case EOF:` in the *escape* switch is genuinely fixed. `const
string B = "abc\` at end of file reports `Bad escape character` on the base and
`End of file while reading string at 1` at the head.
- Dropping `%option noyywrap` is behaviour-neutral: flex expands that option
to `#define yywrap() 1`, so the new `yywrap()` returns exactly what the macro
returned. It is defined in the same file, so no `-lfl` dependency and no
checked-in generated scanner to go stale.
- The fixture and the new test file need no build-file changes — `file(GLOB
${name}/*.c* ${name}/*.thrift)` in `tests/CMakeLists.txt` picks them up, and
`EXTRA_DIST = … tests` covers `make dist`.
- Accepted input is unchanged: `tutorial.thrift` through all 25 generators
is byte-identical between the base compiler and the PR head.
Found 1 issue.
**The *outer* `case EOF:` (line 289) is still unreachable, and the loop it
guards never terminates.**
`yyinput()` returns `0` at end of input, never `-1` — I measured that with a
standalone flex 2.6.4 scanner using this file's options, with and without
`noyywrap`, and it keeps returning `0` on every subsequent call. So an
unterminated string literal at end of file falls through to `default:`, pushes
a NUL into `result`, and comes back for more. Two manifestations of the one
defect:
| input | base `59a949c80` | head `09c89d9cf` |
|---|---|---|
| `const string A = "abc` at EOF, from a file | hangs (10s timeout) | hangs
(10s timeout) |
| the same source via `yy_scan_bytes` | — | SIGSEGV |
The crash is the memory-buffer path: with no `yyin`, the next `yyinput()`
reaches `yy_get_next_buffer()`, which calls `fread` on a null `FILE*`.
```
#0 __GI__IO_fread (buf=..., size=1, count=3, fp=0x0) at ./libio/iofread.c:37
#1 yy_get_next_buffer () at thriftl.cc:1708
#2 yyinput () at thriftl.cc:1841
#3 yylex () at thriftl.ll:287
```
That second row matters for this PR specifically: the new tests drive the
lexer through `yy_scan_bytes`, so the first person who adds a case for an
unterminated string takes the whole suite down with it.
This is pre-existing — the base hangs identically, so it is not a regression
from this PR. But the PR edits that exact line (`while read` → `while reading`,
on a message that can never print), and `yyinput_reached_eof` is already there
to tell the two cases apart. Four lines finish it:
```diff
for(;;)
{
+ yyinput_reached_eof = false;
int ch = yyinput();
switch (ch) {
- case EOF:
- yyerror("End of file while reading string at %d\n", yylineno);
- exit(1);
+ case 0:
+ if (yyinput_reached_eof) {
+ yyerror("End of file while reading string at %d\n", yylineno);
+ exit(1);
+ }
+ result.push_back('\0');
+ continue;
case '\n':
```
`result.push_back('\0')` keeps today's behaviour for a literal NUL inside a
*closed* string — it lands in `result` and `strdup` truncates there, exactly as
now.
I have pushed that onto this branch as f8d01479b, together with two tests
(an unterminated string at end of input, and a literal NUL inside a closed
string). With it applied, `const string A = "abc` reports `End of file while
reading string at 1` and exits 1, `const string N = "a<NUL>b"` still compiles
unchanged, `tutorial.thrift` through all 25 generators stays byte-identical,
and the suite is 22 cases / 326 assertions green, also under `--order rand`.
Both tests fail (SIGSEGV) without the lexer change. Please squash it into your
commit before this merges — and do shout if you would rather have it out again.
Note the same dead `case EOF:` sits in the doc-comment block (line 139) and
the multiline-comment block (line 182): `/* x` and `/** x` at end of file hang
the compiler as well. Those are untouched by this PR and belong in their own
ticket.
### Copilot's open comments
Of the eight threads still marked current, one was worth acting on, and it
is the finding above — reported twice, at `thriftl.ll:295` and `:305`, and now
addressed by f8d01479b. The rest:
- `parser_support.cc:68` (unchecked second `vsnprintf`, double newline) and
`:54` (prefix printed before formatting is known to succeed) — both already
fixed in this head; Copilot is re-reporting its own earlier round.
- `parser_support.cc:55` and `:57` (`vsnprintf(nullptr, 0, …)` portability)
— not a real constraint. That behaviour is required by C99 §7.19.6.5 and by
C++11, and MSVC has conformed since VS2015. Test-only code besides.
- `thriftl.ll:333` (`ch` signedness) — theoretical. `yyinput()` provably
returns 0…255 here and `0` is handled separately, so `ch` in `default:` is
1…255. Harmless to add the `unsigned char` cast if you want the belt as well as
the braces.
- `parser_support.cc:68` (newline normalised for stderr but not for the
thrown message) — true, and the call sites in `thrifty.yy` and `main.cc` do
lack the trailing `\n`, but nothing asserts the difference and it is test-only.
Worth knowing when you read those: Copilot's last four runs all report
*"couldn't run its full agentic review … timeout"*, so they ran degraded. That
is why the same half-resolved finding came back three times.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
