Copilot commented on code in PR #3786:
URL: https://github.com/apache/thrift/pull/3786#discussion_r3959842653
##########
compiler/cpp/src/thrift/thriftl.ll:
##########
@@ -290,14 +290,21 @@ literal_begin (['\"])
int ch = yyinput();
switch (ch) {
case EOF:
- yyerror("End of file while read string at %d\n", yylineno);
+ yyerror("End of file while reading string at %d\n", yylineno);
exit(1);
case '\n':
- yyerror("End of line while read string at %d\n", yylineno - 1);
+ yyerror("End of line while reading string at %d\n", yylineno - 1);
exit(1);
case '\\':
ch = yyinput();
switch (ch) {
+ // Flex yyinput() returns zero at end of input.
+ case 0:
+ yyerror("End of file while reading string at %d\n", yylineno);
+ exit(1);
Review Comment:
Treating `yyinput()` returning `0` as end-of-file can misdiagnose (or
prematurely terminate on) a literal NUL byte after a backslash. If NUL is
possible in scanned input (e.g., via `yy_scan_bytes`), this branches to an EOF
message instead of reporting an invalid escape byte (0x00). Consider
distinguishing true end-of-buffer from a NUL byte (e.g., using Flex
mechanisms/guards) or treating `0` here as `Invalid escape byte 0x00` for
correctness.
##########
compiler/cpp/src/thrift/thriftl.ll:
##########
@@ -317,7 +324,12 @@ literal_begin (['\"])
result.push_back('\\');
continue;
default:
- yyerror("Bad escape character\n");
+ if (ch >= 0x20 && ch <= 0x7e) {
+ yyerror("Invalid escape sequence '\\%c'. Use \\\\ for a literal
backslash.\n", ch);
+ } else {
+ yyerror("Invalid escape byte 0x%02X. Use \\\\ for a literal
backslash.\n",
+ static_cast<unsigned int>(ch));
Review Comment:
`ch` is an `int` from `yyinput()` and can be negative depending on
platform/char signedness or scanner internals. In that scenario, the
printable-range check is wrong and `static_cast<unsigned int>(ch)` can yield
values like `0xFFFFFFFF` instead of the intended single-byte hex. Normalize to
an unsigned byte before comparisons/formatting (e.g., cast to `unsigned char`
and format that value) to ensure diagnostics are correct for bytes >= 0x80 and
any negative values.
##########
compiler/cpp/tests/thrift_test_parser_support.cc:
##########
@@ -46,11 +46,21 @@ void yyerror(const char* fmt, ...) {
std::fprintf(stderr, "[ERROR:%s:%d] ", g_curpath.c_str(), yylineno);
va_list args;
va_start(args, fmt);
- std::vfprintf(stderr, fmt, args);
+ va_list size_args;
+ va_copy(size_args, args);
+ const int size = std::vsnprintf(nullptr, 0, fmt, size_args);
+ va_end(size_args);
+ if (size < 0) {
+ va_end(args);
+ throw std::runtime_error("Unable to format thrift parser error");
+ }
+ std::vector<char> message(size + 1);
+ std::vsnprintf(message.data(), message.size(), fmt, args);
va_end(args);
+ std::fwrite(message.data(), 1, static_cast<size_t>(size), stderr);
std::fprintf(stderr, "\n");
- throw std::runtime_error("thrift parser error");
+ throw std::runtime_error(std::string(message.data(),
static_cast<size_t>(size)));
Review Comment:
Two issues here: (1) the return value of the second `vsnprintf` call is
ignored, so formatting errors (or unexpected size changes) won’t be detected
and could lead to throwing/printing a partially formatted message; consider
checking that return value and failing deterministically if it’s negative. (2)
`yyerror` unconditionally prints an extra newline via `fprintf(\"\\n\")`, but
many call sites already include `\\n` in the format string (including the new
lexer messages), which will produce double blank lines on stderr; consider
enforcing a single newline policy (either remove embedded `\\n` from call sites
or only print the extra newline when the formatted message doesn’t already end
with one).
--
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]
