Jens-G commented on PR #3786: URL: https://github.com/apache/thrift/pull/3786#issuecomment-5576108226
### Code review Re-reviewed at `7dcc58ca0`. Built the compiler and the standalone test suite from the PR head and from `da18006b9`, and compared behaviour on crafted inputs. The rewrite closes everything actionable from the last round: the fixture is a committed `.thrift` loaded through `source_dir()`/`join_path()`, the `php` namespace is gone, all six supported escapes are pinned by `ALL_DOUBLE`/`ALL_SINGLE`, and since accepted input is unchanged the `doc/specs/idl.md` question is moot. The suite goes from 17 cases / 216 assertions green on the base to 19 / 234 on the head, order-independent (`--order rand`, six seeds) and clean under ASan/UBSan. The ticket's escape now reports what it should: `Invalid escape sequence '\T'. Use \\ for a literal backslash.` Found 2 issues: 1. The new `case EOF:` in the escape switch is unreachable, so the end-of-file case it was added for still lands in `default:`. flex's `yyinput()` returns `0` at end of input — the generated scanner does `if (yywrap()) return 0;`, and `%option noyywrap` (line 105) makes `yywrap()` the constant `1` — and every other return path is `c = *(unsigned char *)(yy_c_buf_p)`, so the value is always 0..255 and never `EOF` (-1). A file ending `const string A = "abc\` prints, at the PR head, `Invalid escape sequence '\^@'. Use \\ for a literal backslash.` — `^@` being a NUL byte written straight to stderr by the `%c`. `case '\n':` two lines down does work: `\` before a newline correctly reports `End of line while reading string`. `case 0:` would make both the new branch and the identical, equally dead outer one at line 292 live. (For context, the outer one being dead is why `const string A = "abc` with no closing quote spins forever in the `for(;;)` pushing NUL bytes — I timed it out at 8s on both `da18006b9` and the head, so that part is pre-existing and not this PR's to fix.) https://github.com/apache/thrift/blob/7dcc58ca09d52b8aec91cd9d990e6971920eae6e/compiler/cpp/src/thrift/thriftl.ll#L299-L307 2. The rewritten test-harness `yyerror()` truncates its own message at the first embedded NUL, which drops exactly the hint this PR adds. `message` is a `std::vector<char>` correctly sized by the two-pass `vsnprintf`, but line 60 hands `message.data()` to `%s` and line 63 to `std::runtime_error(const char*)`, and both stop at the first NUL. Together with issue 1, the backslash-at-EOF case formats `%c` with `0`, so `what()` comes back as the 26 bytes `Invalid escape sequence '\` and the `Use \\ for a literal backslash.` clause is gone — verified by calling `yylex()` on `yy_scan_string("\"abc\\")` against a build of the head. `main.cc`'s real `yyerror()` is unaffected, since it `vfprintf`s straight off the va_list; this is specific to the harness. The new test cannot see it: the loop only probes `{"T","x41","0","u00e9"}`, all of which put a printable byte after the backslash, and it asserts on the hint substring only, never on the `'\%c'` part. https://github.com/apache/thrift/blob/7dcc58ca09d52b8aec91cd9d990e6971920eae6e/compiler/cpp/tests/thrift_test_parser_support.cc#L57-L64 Three suggestions, below the bar for the list above but verified: - `%c` writes one raw byte, so the other two non-printables land in the message as-is. On a CRLF file, `\` before the line break gives `Invalid escape sequence '` + CR, which returns the cursor to column 0 and overwrites the rest of the line on a terminal; a multi-byte character gives a lone continuation byte (`"ab\éc"` → `Invalid escape sequence '\M-C'`, not valid UTF-8). Escaping non-printables would cover those and the NUL in one go. - Still 4 commits, up from 2 last time, and `34ac5e314`/`97efe4d3d` still carry `Client: cpp,php` while the two newer ones carry the correct `Client: compiler`. `build/generate-changes.py` reads that trailer per commit and a default squash-merge concatenates all four messages, so as it stands this would also be filed under C++ and PHP. `34ac5e314`'s subject also states the opposite of what now ships. - `extern int yylex();` at line 27 of the new test duplicates the `extern "C"` declaration already visible from `main.h` through `t_cpp_generator_test_utils.h`. 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- 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]
