[preprocessor] Fix crash/hang on truncated callable macro invocation

GenerateBypassWhiteSpaces dereferences the stream iterator (**iterator) with no
end guard, relying on every stream ending in a kept EOF token. The streams
re-lexed for macro expansion in ExpandText, ExpandMacro, HandleInclude and the
standalone tool strip the EOF (loop stops on !isEOF()), so a callable macro
invocation truncated at end-of-stream (e.g. `define A(x) hello `A followed by
`A(1)) makes the streamer return the view's end() iterator and the deref reads
past the end -> SIGSEGV. An included file ending in a callable macro crashes the
same way; a '(' with no ')' spins forever.

Restore the kept-EOF sentinel on each re-lexed stream so the whitespace-skip
loop stops at EOF and callers return a diagnostic instead of dereferencing past
the end: append the EOF sentinel in all four stream builders; break on it in the
two token-pulling loops so it is not forwarded; skip it when splicing an included
child stream into the parent; and break on EOF in the argument-scanning loop
(the one caller that did not handle a mid-scan EOF, which otherwise hangs on a
'(' without ')'). Also record the "callable macro without ()" error in
preprocess_data_.errors instead of silently swallowing it (requires making
ConsumeAndParseMacroCall non-static).

Adds TruncatedCallableMacroDoesNotCrash. Existing preprocessor and analyzer test
suites pass. Unbounded macro self-recursion is a separate pre-existing bug and
is not addressed here.

Signed-off-by: Eylon Krause <eylon1909@gmail.com>

Author: EylonKrause

verible/verilog/preprocessor/verilog-preprocess.cc

@@ -245,11 +245,15 @@ absl::Status VerilogPreprocess::ConsumeAndParseMacroCall(
   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 @@ absl::Status VerilogPreprocess::ExpandText(
        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 @@ absl::Status VerilogPreprocess::ExpandText(
   // 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 @@ absl::Status VerilogPreprocess::ExpandMacro(
        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 @@ absl::Status VerilogPreprocess::ExpandMacro(
   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 @@ absl::Status VerilogPreprocess::HandleInclude(
        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 @@ absl::Status VerilogPreprocess::HandleInclude(
     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);
   }
 

verible/verilog/preprocessor/verilog-preprocess.h

@@ -164,9 +164,11 @@ class VerilogPreprocess {
   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 &,

verible/verilog/preprocessor/verilog-preprocess_test.cc

@@ -1033,5 +1033,30 @@ TEST(VerilogPreprocessTest,
       << 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

verible/verilog/tools/preprocessor/verilog-preprocessor.cc

@@ -122,6 +122,7 @@ static absl::Status PreprocessSingleFile(
     // 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);