Page MenuHomePhabricator

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

Authored by arphaman on Jun 13 2017, 4:55 PM.

Details

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).

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jun 13 2017, 4:55 PM
ahatanak edited edge metadata.Jun 14 2017, 10:50 AM

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?

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 updated this revision to Diff 102718.Jun 15 2017, 1:36 PM

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 added inline comments.Jun 15 2017, 3:26 PM
lib/Parse/ParseObjc.cpp
220

Do you need this check here (and below)?

3670

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 marked an inline comment as done.Jun 19 2017, 4:30 AM
arphaman added inline comments.
lib/Parse/ParseObjc.cpp
220

Not anymore. Thanks!

3670

Good point. Thanks for noticing!

arphaman updated this revision to Diff 103013.Jun 19 2017, 4:31 AM
arphaman marked an inline comment as done.

Remove the old checks and consume EOF unconditionally.

Thanks, LGTM.

lib/Parse/ParseObjc.cpp
3636

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

This revision was automatically updated to reflect the committed changes.
arphaman marked an inline comment as done.