[preprocessor] Fix crash/hang on truncated callable macro invocation - #2535
Open
EylonKrause wants to merge 1 commit into
Open
[preprocessor] Fix crash/hang on truncated callable macro invocation#2535EylonKrause wants to merge 1 commit into
EylonKrause wants to merge 1 commit into
Conversation
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>
Collaborator
|
The preprocessing test is failing the CI. Can you rebase (as there was some unrelated mac failure issue), and have a look at the failing test ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The preprocessor crashes (SIGSEGV) or hangs on parseable SystemVerilog when a callable macro invocation is truncated at end-of-stream. Minimal reproducers (all with
--expand_macros, e.g. viaverible-verilog-preprocessor preprocess):An included file ending in a callable macro reference (e.g.
foo \A) crashes the same way, and a(with no matching)(``define A(x) hello `C( ``) spins in an infinite loop.Root cause
GenerateBypassWhiteSpaces(verilog-preprocess.cc:63) pulls tokens from aStreamIteratorGeneratorand dereferences the result —**iterator— with noendguard:MakeConstIteratorStreamerreturns the view'send()iterator once exhausted (token-stream-adapter.h), so**iteratoronend()is a double past-the-end dereference. The function relies on the invariant that every stream ends in a kept EOF token (KeepSyntaxTreeTokens(EOF)is true), which stops the loop and lets callers seeisEOF(). The top-level analyzer stream satisfies this (it retains EOF), but the streams re-lexed for macro expansion do not:ExpandText,ExpandMacro,HandleInclude, and the standalone tool all build their token sequence with a loop that stops before the EOF (!lexer.GetLastToken().isEOF()). A truncated callable-macro invocation at the end of such a stream reachesConsumeAndParseMacroCall, which callsGenerateBypassWhiteSpaces, whosegenerator()returnsend()→ crash. The sibling streamers (MakeTokenStreamer) already guard this by returning anEOFTokensentinel;GenerateBypassWhiteSpacesdoes not, because it only has thestd::function, not theenditerator.Fix
Restore the kept-EOF invariant on every re-lexed stream, so
GenerateBypassWhiteSpacesstops at EOF and callers handle it gracefully (returningInvalidArgumentError) instead of dereferencing past the end:ExpandText/ExpandMacro/HandleInclude/ the standalone tool: append the EOF token as an end sentinel before building the stream view.ExpandText/ExpandMacrotoken-pulling loops:breakon the EOF sentinel so it is not forwarded into the expanded output.HandleInclude: skip the sentinel EOF when splicing the child stream into the parent (the childScanStreamforwards it as a pass-through token; without this it would land in the middle of the parent stream).ConsumeAndParseMacroCallargument loop:breakon EOF. With the sentinel in place a(without a matching)would otherwise spin forever; this is the one caller that did not already handle a mid-scan EOF.ConsumeAndParseMacroCall: record the "illegal to call a callable macro without ()" error inpreprocess_data_.errors(it was returned as aStatusbut never surfaced, so truncated input was silently accepted). This required making the method non-static (its only caller is non-static).Testing
Built
verible-verilog-preprocessorand confirmed all four crash variants and the open-paren hang now exit cleanly. The full//verible/verilog/preprocessor/...suite (including the existing "Nested callable macros" test) andverilog-analyzer_testpass. AddedTruncatedCallableMacroDoesNotCrashcovering the no-((now a diagnostic) and open-((now terminates) cases.Out of scope (separate, pre-existing bug, not addressed here): unbounded macro self-recursion such as
`define A(x) `A(x)overflows the stack with no depth guard — worth a follow-up.Disclosure: this contribution was authored with an AI coding assistant (Claude) and reviewed before submission.