[Parser][ObjC] Avoid crashing when skipping to EOF while parsing an ObjC interface/implementation

[arphaman]

Summary

This patch fixes a Clang crash that happens when an Objective-C source code contains an @interface/@implementation declaration that follows unterminated @implementation declaration that contains a method with a message send that doesn't have the ']'. The crash happens because the @interface/@implementation parsing methods encounter an unexpected EOF that is caused by parser's attempt to recover from the missing ']' by skipping to the next ']'/';' (it reaches EOF instead since these tokens are not present).

[ahatanak]

This patch fixes the crash and that is fine, but the users might find the last error ("expected '}'" at the end of the file) confusing. This seems to happen because Parser::ParseLexedObjCMethodDefs consumes all tokens in the file until it sees the EOF after consuming all the cached tokens of LexedMethod.

I wonder whether we can insert a fake EOF token to prevent ParseLexedObjCMethodDefs from going all the way to the end, just like ParseCXXNonStaticMemberInitialize does. Do you think that would work or would it be too hackish?

[arphaman]

In D34185#780494, @ahatanak wrote:

This patch fixes the crash and that is fine, but the users might find the last error ("expected '}'" at the end of the file) confusing. This seems to happen because Parser::ParseLexedObjCMethodDefs consumes all tokens in the file until it sees the EOF after consuming all the cached tokens of LexedMethod.

I wonder whether we can insert a fake EOF token to prevent ParseLexedObjCMethodDefs from going all the way to the end, just like ParseCXXNonStaticMemberInitialize does. Do you think that would work or would it be too hackish?

I think that it would probably work quite well. I've thought about fixing it while working on a patch as well, but didn't realize that we could just inject EOF into the token stream. I will update this patch with this kind of handling then.

[arphaman]

Use the 'Eof' token to make sure that the "expected '}'" error is presented not at the end of the file, but at the start of the @interface/@implementation.

[ahatanak]

lib/Parse/ParseObjc.cpp

220 ↗

(On Diff #102718)

Do you need this check here (and below)?

3674 ↗

(On Diff #102718)

I think you want to clean up the EOF token after the code below which skips the leftover tokens, regardless of whether Tok is EOF. You can do it unconditionally since Tok.getLocation() == OrigLoc and you know the token is the EOF inserted above.

[arphaman]

lib/Parse/ParseObjc.cpp

220 ↗

(On Diff #102718)

Not anymore. Thanks!

3674 ↗

(On Diff #102718)

Good point. Thanks for noticing!

[arphaman]

Remove the old checks and consume EOF unconditionally.

[ahatanak]

Thanks, LGTM.

lib/Parse/ParseObjc.cpp

3636 ↗

(On Diff #103013)

You can use OrigLoc instead of calling Tok.getLocation().

cfe/trunk/lib/Parse/ParseObjc.cpp

bool skip = MCDecl &&
               (!parseMethod && Actions.isObjCMethodDecl(MCDecl)));
   if (skip)
     return;
   
   // Save the current token position.
   SourceLocation OrigLoc = Tok.getLocation();
 
   assert(!LM.Toks.empty() && "ParseLexedObjCMethodDef - Empty body!");
+  // Store an artificial EOF token to ensure that we don't run off the end of
+  // the method's body when we come to parse it.
+  Token Eof;
+  Eof.startToken();
+  Eof.setKind(tok::eof);
+  Eof.setEofData(MCDecl);
+  Eof.setLocation(OrigLoc);
+  LM.Toks.push_back(Eof);
   // Append the current token at the end of the new token stream so that it
   // doesn't get lost.
   LM.Toks.push_back(Tok);
   PP.EnterTokenStream(LM.Toks, true);
 
   // Consume the previously pushed token.
   ConsumeAnyToken(/*ConsumeCodeCompletionTok=*/true);
     
if (Tok.is(tok::kw_try))
     ParseFunctionTryBlock(MCDecl, BodyScope);
   else {
     if (Tok.is(tok::colon))
       ParseConstructorInitializer(MCDecl);
     else
       Actions.ActOnDefaultCtorInitializers(MCDecl);
     ParseFunctionStatementBody(MCDecl, BodyScope);
   }
-  
   if (Tok.getLocation() != OrigLoc) {
     // Due to parsing error, we either went over the cached tokens or
     // there are still cached tokens left. If it's the latter case skip the
     // leftover tokens.
     // Since this is an uncommon situation that should be avoided, use the
     // expensive isBeforeInTranslationUnit call.
     if (PP.getSourceManager().isBeforeInTranslationUnit(Tok.getLocation(),
                                                      OrigLoc))
       while (Tok.getLocation() != OrigLoc && Tok.isNot(tok::eof))
         ConsumeAnyToken();
   }
+  // Clean up the remaining EOF token.
+  ConsumeAnyToken();
 }

cfe/trunk/test/Parser/objc-at-implementation-eof-crash.m

+// RUN: %clang_cc1 -verify -Wno-objc-root-class %s
+
+@interface ClassA
+
+- (void)fileExistsAtPath:(int)x;
+
+@end
+
+@interface ClassB
+
+@end
+
+@implementation ClassB // expected-note {{implementation started here}}
+
+- (void) method:(ClassA *)mgr { // expected-note {{to match this '{'}}
+  mgr fileExistsAtPath:0
+} // expected-error {{expected ']'}}
+
+@implementation ClassC // expected-error {{missing '@end'}} // expected-error {{expected '}'}} // expected-warning {{cannot find interface declaration for 'ClassC'}}
+
+@end

cfe/trunk/test/Parser/objc-at-interface-eof-crash.m

+// RUN: %clang_cc1 -verify -Wno-objc-root-class %s
+
+@interface ClassA
+
+- (void)fileExistsAtPath:(int)x;
+
+@end
+
+@interface ClassB
+
+@end
+
+@implementation ClassB // expected-note {{implementation started here}}
+
+- (void) method:(ClassA *)mgr { // expected-note {{to match this '{'}}
+  mgr fileExistsAtPath:0
+} // expected-error {{expected ']'}}
+
+@interface ClassC // expected-error {{missing '@end'}} // expected-error {{expected '}'}}
+
+@end