Copilot commented on code in PR #3786:
URL: https://github.com/apache/thrift/pull/3786#discussion_r3960559102
##########
compiler/cpp/tests/thrift_test_parser_support.cc:
##########
@@ -46,11 +46,26 @@ void yyerror(const char* fmt, ...) {
std::fprintf(stderr, "[ERROR:%s:%d] ", g_curpath.c_str(), yylineno);
Review Comment:
The error prefix is printed to `stderr` before the message formatting is
known to succeed. If formatting fails, this leaves a partial `[ERROR:...]` line
in stderr, which can make test logs confusing and harder to triage. Consider
formatting the message first, then writing the prefix + message to `stderr`
only after successful formatting (or ensuring the failure path also terminates
the line).
##########
compiler/cpp/src/thrift/thriftl.ll:
##########
@@ -290,14 +287,25 @@ 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 '\\':
Review Comment:
`yyinput()` returns 0 at end-of-input (and may also return 0 for a literal
NUL byte), so `case EOF:` (EOF == -1) will never match here. This can cause
unterminated string literals (EOF not preceded by a backslash) to be mishandled
(e.g., treated as a NUL and potentially loop indefinitely). A concrete fix is
to add a `case 0:` branch that checks `yyinput_reached_eof` (set by `yywrap()`)
and reports EOF appropriately; otherwise treat literal NUL consistently (likely
as an invalid byte) rather than falling through as normal content.
##########
compiler/cpp/tests/thrift_test_parser_support.cc:
##########
@@ -46,11 +46,26 @@ 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");
Review Comment:
The error prefix is printed to `stderr` before the message formatting is
known to succeed. If formatting fails, this leaves a partial `[ERROR:...]` line
in stderr, which can make test logs confusing and harder to triage. Consider
formatting the message first, then writing the prefix + message to `stderr`
only after successful formatting (or ensuring the failure path also terminates
the line).
##########
compiler/cpp/tests/thrift_test_parser_support.cc:
##########
@@ -46,11 +46,26 @@ 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");
+ }
Review Comment:
Using `std::vsnprintf(nullptr, 0, ...)` for sizing is not portable across
all C/C++ runtime implementations (some return -1 for this pattern). In those
environments, tests would start failing with `Unable to format thrift parser
error` even though the format string is valid. A more robust approach is to
attempt formatting into a reasonably sized buffer and grow/retry based on the
return value, avoiding reliance on the nullptr sizing call.
--
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]
