[SemaObjC] Fix crash while parsing type arguments and protocols

[bruno]

Summary

Fix a crash-on-invalid.

When parsing type arguments and protocols, ParseTypeName() tries to find
matching tokens for '[', '(', etc whenever they appear among potential
type names. If unmatched, ParseTypeName() yields a tok::eof token
stream. This leads to crashes since the parsing at this point is not
expected to go beyond the param list closing '>'.

rdar://problem/25063557

[doug.gregor]

This will work, but it's *really* unfortunate to put tentative parsing into this code path because tentative parsing is far from free, and specialized Objective-C types are getting pretty common in Objective-C headers. Why can't the callers be taught to handle EOF and bail out?

[bruno]

Updated patch and changed approach after Doug's comment.

[bruno]

Ping!

[aaron.ballman]

In D23852#525291, @doug.gregor wrote:

This will work, but it's *really* unfortunate to put tentative parsing into this code path because tentative parsing is far from free, and specialized Objective-C types are getting pretty common in Objective-C headers. Why can't the callers be taught to handle EOF and bail out?

The unfortunate part of that is it makes all of the callers fragile -- we have to sprinkle "are we at EOF now" checks all over, or risk running into the same problem later. We don't typically do that (there are only 7 eof checks in the parser currently, this adds 6 more). I don't know which option is worse.

[bruno]

Ping!

In D23852#529308, @aaron.ballman wrote:

In D23852#525291, @doug.gregor wrote:

This will work, but it's *really* unfortunate to put tentative parsing into this code path because tentative parsing is far from free, and specialized Objective-C types are getting pretty common in Objective-C headers. Why can't the callers be taught to handle EOF and bail out?

The unfortunate part of that is it makes all of the callers fragile -- we have to sprinkle "are we at EOF now" checks all over, or risk running into the same problem later. We don't typically do that (there are only 7 eof checks in the parser currently, this adds 6 more). I don't know which option is worse.

Likewise, I'm not sure what's the best tradeoff. And there's probably more to come: some part of PR23057 is likely the need to properly handle EOFs.

[bruno]

Ping!

[doug.gregor]

I don't love the fact that it makes callers fragile, but having to do tentative parsing for these otherwise-unambiguous cases is expensive (in compile time). We should only use tentative parsing when we have an actual ambiguity to resolve.

[bruno]

Thanks Doug!

Committed r281383

lib/Parse/ParseDecl.cpp

return !getLangOpts().CPlusPlus
          // list.  If we are presented with something like: "void foo(intptr x,
          // float y)", we don't want to start parsing the function declarator as
          // though it is a K&R style declarator just because intptr is an
          // invalid type.
          //
          // To handle this, we check to see if the token after the first
          // identifier is a "," or ")".  Only then do we parse it as an
          // identifier list.
-         && (NextToken().is(tok::comma) || NextToken().is(tok::r_paren));
+         && (!Tok.is(tok::eof) &&
+             (NextToken().is(tok::comma) || NextToken().is(tok::r_paren)));
 }
 
 /// ParseFunctionDeclaratorIdentifierList - While parsing a function declarator
 /// we found a K&R-style identifier list instead of a typed parameter list.
 ///
 /// After returning, ParamInfo will hold the parsed parameters.
 ///
 ///       identifier-list: [C99 6.7.5]

lib/Parse/ParseObjc.cpp

if (Tok.is(tok::colon)) { // a super class is specified.
 
     // Type arguments for the superclass or protocol conformances.
     if (Tok.is(tok::less)) {
       parseObjCTypeArgsOrProtocolQualifiers(
           nullptr, typeArgsLAngleLoc, typeArgs, typeArgsRAngleLoc, LAngleLoc,
           protocols, protocolLocs, EndProtoLoc,
           /*consumeLastToken=*/true,
           /*warnOnIncompleteProtocols=*/true);
+      if (Tok.is(tok::eof))
+        return nullptr;
     }
   }
-  
   // Next, we need to check for any protocol references.
   if (LAngleLoc.isValid()) {
     if (!ProtocolIdents.empty()) {
       // We already parsed the protocols named when we thought we had a
       // type parameter list. Translate them into actual protocol references.
       for (const auto &pair : ProtocolIdents) {
         protocolLocs.push_back(pair.second);
       }
parseObjCTypeArgsOrProtocolQualifiers(baseType,
                                         typeArgs,
                                         typeArgsRAngleLoc,
                                         protocolLAngleLoc,
                                         protocols,
                                         protocolLocs,
                                         protocolRAngleLoc,
                                         consumeLastToken,
                                         /*warnOnIncompleteProtocols=*/false);
+  if (Tok.is(tok::eof)) // Nothing else to do here...
+    return;
 
   // An Objective-C object pointer followed by type arguments
   // can then be followed again by a set of protocol references, e.g.,
   // \c NSArray<NSView><NSTextDelegate>
   if ((consumeLastToken && Tok.is(tok::less)) ||
       (!consumeLastToken && NextToken().is(tok::less))) {
     // If we aren't consuming the last token, the prior '>' is still hanging
     // there. Consume it before we parse the protocol qualifiers.
TypeResult Parser::parseObjCTypeArgsAndProtocolQualifiers(
   SourceLocation protocolRAngleLoc;
 
   // Parse type arguments and protocol qualifiers.
   parseObjCTypeArgsAndProtocolQualifiers(type, typeArgsLAngleLoc, typeArgs,
                                          typeArgsRAngleLoc, protocolLAngleLoc,
                                          protocols, protocolLocs,
                                          protocolRAngleLoc, consumeLastToken);
 
+  if (Tok.is(tok::eof))
+    return true; // Invalid type result.
+
   // Compute the location of the last token.
   if (consumeLastToken)
     endLoc = PrevTokLocation;
   else
     endLoc = Tok.getLocation();
 
   return Actions.actOnObjCTypeArgsAndProtocolQualifiers(
            getCurScope(),

lib/Parse/Parser.cpp

if (getLangOpts().ObjC1 && NextToken().is(tok::less) &&
       SourceLocation IdentifierLoc = ConsumeToken();
       SourceLocation NewEndLoc;
       TypeResult NewType
           = parseObjCTypeArgsAndProtocolQualifiers(IdentifierLoc, Ty,
                                                    /*consumeLastToken=*/false,
                                                    NewEndLoc);
       if (NewType.isUsable())
         Ty = NewType.get();
+      else if (Tok.is(tok::eof)) // Nothing to do here, bail out...
+        return ANK_Error;
     }
 
     Tok.setKind(tok::annot_typename);
     setTypeAnnotation(Tok, Ty);
     Tok.setAnnotationEndLoc(Tok.getLocation());
     Tok.setLocation(BeginLoc);
     PP.AnnotateCachedTokens(Tok);
     return ANK_Success;
if (ParsedType Ty = Actions.getTypeName(
         SourceLocation IdentifierLoc = ConsumeToken();
         SourceLocation NewEndLoc;
         TypeResult NewType
           = parseObjCTypeArgsAndProtocolQualifiers(IdentifierLoc, Ty,
                                                    /*consumeLastToken=*/false,
                                                    NewEndLoc);
         if (NewType.isUsable())
           Ty = NewType.get();
+        else if (Tok.is(tok::eof)) // Nothing to do here, bail out...
+          return false;
       }
 
       // This is a typename. Replace the current token in-place with an
       // annotation type token.
       Tok.setKind(tok::annot_typename);
       setTypeAnnotation(Tok, Ty);
       Tok.setAnnotationEndLoc(Tok.getLocation());
       Tok.setLocation(BeginLoc);

test/SemaObjC/crash-on-type-args-protocols.m

+// RUN: %clang_cc1 -DFIRST -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DSECOND -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DTHIRD -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DFOURTH -fsyntax-only -verify %s
+
+@protocol P;
+@interface NSObject
+@end
+@protocol X
+@end
+@interface X : NSObject <X>
+@end
+
+@class A;
+
+#ifdef FIRST
+id<X> F1(id<[P> v) { // expected-error {{expected a type}} // expected-error {{use of undeclared identifier 'P'}} // expected-error {{use of undeclared identifier 'v'}} // expected-note {{to match this '('}}
+  return 0;
+}
+#endif
+
+#ifdef SECOND
+id<X> F2(id<P[X> v) { // expected-error {{unknown type name 'P'}} // expected-error {{unexpected interface name 'X': expected expression}} // expected-error {{use of undeclared identifier 'v'}} // expected-note {{to match this '('}}
+  return 0;
+}
+#endif
+
+#ifdef THIRD
+id<X> F3(id<P, P *[> v) { // expected-error {{unknown type name 'P'}} // expected-error {{expected expression}} // expected-error {{use of undeclared identifier 'v'}} // expected-note {{to match this '('}}
+  return 0;
+}
+#endif
+
+#ifdef FOURTH
+id<X> F4(id<P, P *(> v { // expected-error {{unknown type name 'P'}} // expected-error {{expected ')'}} // expected-note {{to match this '('}} // expected-note {{to match this '('}}
+  return 0;
+}
+#endif
+
+// expected-error {{expected '>'}} // expected-error {{expected parameter declarator}} // expected-error {{expected ')'}} // expected-error {{expected function body after function declarator}}