[cfe-commits] [PATCH] Implementation quantity limits to prevent crashes

Aaron Ballman aaron at aaronballman.com
Fri Sep 23 13:22:09 PDT 2011


On Fri, Sep 23, 2011 at 1:35 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Sep 21, 2011, at 8:45 PM, Aaron Ballman wrote:
>
>> On Wed, Sep 21, 2011 at 8:11 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>>> Also, the diagnostic wording was taken from MSVC, but could likely be
>>>> improved.  Suggestions welcome.
>>>
>>> I would say something more along the lines of "parser recusion limit
>>> reached"; using the term "stack overflow" makes it sound like a bug in
>>> the compiler.
>>
>> Here is a revised patch with the new diagnostic wording.
>
> Minor stuff: there's a typo "recusion" in the diagnostic wording, and the new test case hasn't been updated for the new wording.

Good catches!

> I like this general direction, but I do wonder about adding another RAII object for this. You've added the RAII object in two places, but aren't there many other similar places? Can we be more methodical, so we can be sure that we don't lose the benefits of this checking with later maintenance?

The intention was to see if folks liked the direction, and then add it
to more places.  Why add it everywhere under the sun if people think
it is the wrong way to solve the problem?

> I have one idea: what if we expanded the mandate of this RAII object a bit, so that it was responsible for both balancing the delimiters (parentheses, square brackets, braces, etc.)? In other words, we introduce the RAII object to consume the opening paren/bracket/brace and also to find the matching ')':

When I started out, I had intended this to have a bit more to do with
[implimits].  But what I ended up with is really is about depth of
recursion in the parser.  Without further accessors, it's not useful
for iterative limit checking (such as the number of constants in an
enumeration).  But do we care?

>  BalancedDelimiterTracket Parens(*this, tok::l_paren); // tries to consume the opening '('
>  if (!Parens) // checks whether an error occurred in consuming the opening ')'
>    return error;
>
>  // parse whatever is in the parens
>
>  if (Parens.close()) { // tries to consume the closing ')'
>    // couldn't find closing ')'
>  }
>
> The idea here would be to encapsulate all of the behavior that now uses MatchRHSPunctuation into an RAII object, and have that also check for implementation limits. There are three benefits here:
>
>  (1) It becomes clearer where we're matching delimited sets, so the structure of the parser improves in general.
>  (2) Implementation limits checking comes "free" when we use the right abstraction, so it won't be forgotten or lost.
>  (3) These RAII objects, in aggregate, outline the source as we're parsing it. We should be able to use that information to improve parser recovery.
>
> What do you think?

I think I'll play around with the idea and get back to you.  :-)  On
its face, it sounds like a clean way to go about it, so I'll see if it
works in practice as well.  Thank you for the idea!

~Aaron




More information about the cfe-commits mailing list