Merge pull request #2535 from EylonKrause/fix/preprocess-truncated-callable-macro
[preprocessor] Fix crash/hang on truncated callable macro invocation
diff --git a/.github/bin/smoke-test.sh b/.github/bin/smoke-test.sh
index ff9d1af..5532187 100755
--- a/.github/bin/smoke-test.sh
+++ b/.github/bin/smoke-test.sh
@@ -126,7 +126,7 @@
ExpectedFailCount[syntax:sv-tests]=74
ExpectedFailCount[lint:sv-tests]=73
ExpectedFailCount[project:sv-tests]=176
-ExpectedFailCount[preprocessor:sv-tests]=128
+ExpectedFailCount[preprocessor:sv-tests]=129
ExpectedFailCount[syntax:caliptra-rtl]=41
ExpectedFailCount[lint:caliptra-rtl]=40
diff --git a/verible/verilog/preprocessor/verilog-preprocess.cc b/verible/verilog/preprocessor/verilog-preprocess.cc
index 2f77111..1d847fb 100644
--- a/verible/verilog/preprocessor/verilog-preprocess.cc
+++ b/verible/verilog/preprocessor/verilog-preprocess.cc
@@ -245,11 +245,15 @@
if ((*token_iter)->text() == "(") {
token_iter = GenerateBypassWhiteSpaces(generator); // skip the "("
} else {
+ preprocess_data_.errors.emplace_back(
+ **token_iter,
+ "Error it is illegal to call a callable macro without ().");
return absl::InvalidArgumentError(
"Error it is illegal to call a callable macro without ().");
}
while (parameters_size > 0) {
+ if ((*token_iter)->isEOF()) break; // truncated call; stop scanning args
if ((*token_iter)->token_enum() == MacroArg) {
macro_call->positional_arguments.emplace_back(**token_iter);
token_iter = GenerateBypassWhiteSpaces(generator);
@@ -342,6 +346,10 @@
lexer.DoNextToken()) {
lexed_sequence.push_back(lexer.GetLastToken());
}
+ // Retain the EOF token as an end sentinel so a truncated callable-macro
+ // invocation stops at EOF in GenerateBypassWhiteSpaces instead of
+ // dereferencing past the end of the stream view.
+ lexed_sequence.push_back(lexer.GetLastToken());
verible::TokenStreamView lexed_streamview;
// Initializing the lexed token stream view.
InitTokenStreamView(lexed_sequence, &lexed_streamview);
@@ -352,6 +360,7 @@
// Token-pulling loop.
for (auto iter = iter_generator(); iter != end; iter = iter_generator()) {
auto &last_token = **iter;
+ if (last_token.isEOF()) break; // end sentinel; nothing to forward
// TODO: handle lexical error
if (lexer.GetLastToken().token_enum() == TK_SPACE) {
continue; // don't forward spaces
@@ -396,6 +405,8 @@
lexer.DoNextToken()) {
lexed_sequence.push_back(lexer.GetLastToken());
}
+ // Retain EOF end sentinel (see ExpandText).
+ lexed_sequence.push_back(lexer.GetLastToken());
verible::TokenStreamView lexed_streamview;
// Initializing the lexed token stream view.
InitTokenStreamView(lexed_sequence, &lexed_streamview);
@@ -407,6 +418,7 @@
for (auto iter = iter_generator(); iter != end; iter = iter_generator()) {
// TODO: handle lexical error
auto &last_token = **iter;
+ if (last_token.isEOF()) break; // end sentinel; nothing to forward
if (last_token.token_enum() == TK_SPACE) continue; // don't forward spaces
// If the expanded token is another macro identifier that needs to be
// expanded.
@@ -635,6 +647,9 @@
lexer.DoNextToken()) {
included_sequence.push_back(lexer.GetLastToken());
}
+ // Retain EOF end sentinel; the child ScanStream expects an EOF-terminated
+ // stream.
+ included_sequence.push_back(lexer.GetLastToken());
// Preprocessing the included file tokens.
verible::TokenStreamView lexed_streamview;
@@ -657,8 +672,11 @@
preprocess_data_.included_text_structure.push_back(std::move(u));
}
- // Forwarding the included preprocessed view.
+ // Forwarding the included preprocessed view. The EOF end sentinel appended
+ // above is consumed by the child ScanStream and must not be spliced into the
+ // middle of the parent's token stream.
for (const auto &u : child_preprocessed_data.preprocessed_token_stream) {
+ if (u->isEOF()) continue;
preprocess_data_.preprocessed_token_stream.push_back(u);
}
diff --git a/verible/verilog/preprocessor/verilog-preprocess.h b/verible/verilog/preprocessor/verilog-preprocess.h
index 2d134ae..0d932f0 100644
--- a/verible/verilog/preprocessor/verilog-preprocess.h
+++ b/verible/verilog/preprocessor/verilog-preprocess.h
@@ -164,9 +164,11 @@
absl::Status HandleElse(TokenStreamView::const_iterator else_pos);
absl::Status HandleEndif(TokenStreamView::const_iterator endif_pos);
- static absl::Status ConsumeAndParseMacroCall(
- TokenStreamView::const_iterator, const StreamIteratorGenerator &,
- verible::MacroCall *, const verible::MacroDefinition &);
+ // Non-static so it can record diagnostics into preprocess_data_.errors.
+ absl::Status ConsumeAndParseMacroCall(TokenStreamView::const_iterator,
+ const StreamIteratorGenerator &,
+ verible::MacroCall *,
+ const verible::MacroDefinition &);
// The following functions return nullptr when there is no error:
absl::Status ConsumeMacroDefinition(const StreamIteratorGenerator &,
diff --git a/verible/verilog/preprocessor/verilog-preprocess_test.cc b/verible/verilog/preprocessor/verilog-preprocess_test.cc
index ae33567..a8e41d9 100644
--- a/verible/verilog/preprocessor/verilog-preprocess_test.cc
+++ b/verible/verilog/preprocessor/verilog-preprocess_test.cc
@@ -1033,5 +1033,30 @@
<< error.error_message;
}
+// Regression: a callable-macro invocation truncated at end-of-stream (no '(',
+// or '(' with no matching ')') must not crash or hang the preprocessor. Before
+// the fix these inputs dereferenced past the end of the token stream view
+// (SIGSEGV) or spun forever scanning arguments. With error-surfacing enabled
+// the no-'(' cases also report a preprocessor diagnostic.
+TEST(VerilogPreprocessTest, TruncatedCallableMacroDoesNotCrash) {
+ constexpr std::string_view kNoParenInputs[] = {
+ "`define A(x) hello `A\n`A(1)\n", // truncated callable ref in macro body
+ "`define A(x) x\n`A\n", // truncated callable ref at top level
+ };
+ for (std::string_view input : kNoParenInputs) {
+ PreprocessorTester tester(
+ input, VerilogPreprocess::Config({.expand_macros = true}));
+ EXPECT_FALSE(tester.Status().ok()) << input;
+ EXPECT_GE(tester.PreprocessorData().errors.size(), 1) << input;
+ }
+
+ // '(' with no matching ')': must terminate (was an infinite loop). The
+ // residue is rejected downstream, so only assert non-OK here.
+ PreprocessorTester open_paren(
+ "`define C(z) z\n`define A(x) hello `C(\n`A(1)\n",
+ VerilogPreprocess::Config({.expand_macros = true}));
+ EXPECT_FALSE(open_paren.Status().ok());
+}
+
} // namespace
} // namespace verilog
diff --git a/verible/verilog/tools/preprocessor/verilog-preprocessor.cc b/verible/verilog/tools/preprocessor/verilog-preprocessor.cc
index c23fdf7..26907dc 100644
--- a/verible/verilog/tools/preprocessor/verilog-preprocessor.cc
+++ b/verible/verilog/tools/preprocessor/verilog-preprocessor.cc
@@ -122,13 +122,17 @@
// source code just like it was, but with conditionals filtered.
lexed_sequence.push_back(lexer.GetLastToken());
}
+ lexed_sequence.push_back(lexer.GetLastToken()); // EOF end sentinel
verible::TokenStreamView lexed_streamview;
// Initializing the lexed token stream view.
InitTokenStreamView(lexed_sequence, &lexed_streamview);
verilog::VerilogPreprocessData preprocessed_data =
preprocessor.ScanStream(lexed_streamview);
auto &preprocessed_stream = preprocessed_data.preprocessed_token_stream;
- for (auto u : preprocessed_stream) outs << u->text();
+ for (auto u : preprocessed_stream) {
+ if (u->isEOF()) continue; // end sentinel, not part of the source
+ outs << u->text();
+ }
for (auto &u : preprocessed_data.errors) outs << u.error_message << '\n';
if (!preprocessed_data.errors.empty()) {
return absl::InvalidArgumentError("Error: The preprocessing has failed.");
