[ObjC] Prevent infinite loops when iterating over redeclaration of a method that was declared in an invalid interface

[arphaman]

Summary

This patch fixes an infinite loop that occurs when clang tries to iterate over redeclaration of a method that was declared in an invalid @interface. The existing validity checks don't catch this as that @interface is a duplicate of a previously declared valid @interface declaration, so we have to verify that the found redeclaration is in a valid declaration context.

[mehdi_amini]

LGTM, thks.
(See one inline comment to fix though)

lib/AST/DeclObjC.cpp

841

Either you should use dyn_cast, or you should just cast but without a if

[arphaman]

lib/AST/DeclObjC.cpp

841

Thanks! Yeah, this is supposed to be dyn_cast.

[erik.pilkington]

lib/AST/DeclObjC.cpp

841

Why dyn_cast? Can't we bank on a DeclContext being a Decl?

888

Do you think it would be cleaner to just do the check once here, if Redecl is non-null?

[arphaman]

lib/AST/DeclObjC.cpp

841

I think you're right, I just need a cast without an if since DeclContext subclasses are all Decls.

888

Yes, that would be neater. Thanks for the suggestion!

[arphaman]

I managed to get rid of the extra function and simplify the check thanks to suggestions from Erik.

lib/AST/DeclObjC.cpp

void ObjCMethodDecl::setMethodParams(ASTContext &C,
   SelLocsKind = hasStandardSelectorLocs(getSelector(), SelLocs, Params,
                                         DeclEndLoc);
   if (SelLocsKind != SelLoc_NonStandard)
     return setParamsAndSelLocs(C, Params, llvm::None);
 
   setParamsAndSelLocs(C, Params, SelLocs);
 }
 
+/// Ensures that the discovered method redeclaration has a valid declaration
+/// context. Used by ObjCMethodDecl::getNextRedeclarationImpl to prevent
+/// infinite loops when iterating redeclarations in a partially invalid AST.
+static ObjCMethodDecl *ensureRedeclHasValidContext(ObjCMethodDecl *Redecl) {
+  if (!Redecl)
+    return nullptr;
+  if (const auto *Ctx = cast<Decl>(Redecl->getDeclContext()))
+    if (Ctx->isInvalidDecl())
+      return nullptr;
+  return Redecl;
+}
+
 /// \brief A definition will return its interface declaration.
 /// An interface declaration will return its definition.
 /// Otherwise it will return itself.
 ObjCMethodDecl *ObjCMethodDecl::getNextRedeclarationImpl() {
   ASTContext &Ctx = getASTContext();
   ObjCMethodDecl *Redecl = nullptr;
   if (HasRedeclaration)
     Redecl = const_cast<ObjCMethodDecl*>(Ctx.getObjCMethodRedeclaration(this));
   if (Redecl)
     return Redecl;
 
   Decl *CtxD = cast<Decl>(getDeclContext());
 
   if (!CtxD->isInvalidDecl()) {
     if (ObjCInterfaceDecl *IFD = dyn_cast<ObjCInterfaceDecl>(CtxD)) {
       if (ObjCImplementationDecl *ImplD = Ctx.getObjCImplementation(IFD))
         if (!ImplD->isInvalidDecl())
-          Redecl = ImplD->getMethod(getSelector(), isInstanceMethod());
+          Redecl = ensureRedeclHasValidContext(
+              ImplD->getMethod(getSelector(), isInstanceMethod()));
 
     } else if (ObjCCategoryDecl *CD = dyn_cast<ObjCCategoryDecl>(CtxD)) {
       if (ObjCCategoryImplDecl *ImplD = Ctx.getObjCImplementation(CD))
         if (!ImplD->isInvalidDecl())
-          Redecl = ImplD->getMethod(getSelector(), isInstanceMethod());
+          Redecl = ensureRedeclHasValidContext(
+              ImplD->getMethod(getSelector(), isInstanceMethod()));
 
     } else if (ObjCImplementationDecl *ImplD =
                  dyn_cast<ObjCImplementationDecl>(CtxD)) {
       if (ObjCInterfaceDecl *IFD = ImplD->getClassInterface())
         if (!IFD->isInvalidDecl())
-          Redecl = IFD->getMethod(getSelector(), isInstanceMethod());
+          Redecl = ensureRedeclHasValidContext(
+              IFD->getMethod(getSelector(), isInstanceMethod()));
 
     } else if (ObjCCategoryImplDecl *CImplD =
                  dyn_cast<ObjCCategoryImplDecl>(CtxD)) {
       if (ObjCCategoryDecl *CatD = CImplD->getCategoryDecl())
         if (!CatD->isInvalidDecl())
-          Redecl = CatD->getMethod(getSelector(), isInstanceMethod());
+          Redecl = ensureRedeclHasValidContext(
+              CatD->getMethod(getSelector(), isInstanceMethod()));
     }
   }
 
   if (!Redecl && isRedeclaration()) {
     // This is the last redeclaration, go back to the first method.
     return cast<ObjCContainerDecl>(CtxD)->getMethod(getSelector(),
                                                     isInstanceMethod());
   }
 
   return Redecl ? Redecl : this;
 }

test/SemaObjC/method-redecls-invalid-interface.m

+// RUN: %clang_cc1 -fsyntax-only -verify -Wdocumentation -Wno-objc-root-class %s
+// rdar://29220965
+
+@interface InvalidInterface { // expected-note {{previous definition is here}}
+  int *_property;
+}
+
+@end
+
+/*!
+ */
+
+@interface InvalidInterface // expected-error {{duplicate interface definition for class 'InvalidInterface'}}
+@property int *property;
+
+-(void) method;
+@end
+
+@implementation InvalidInterface
+-(void) method { }
+@end