Fix infinite loop when ::new or ::delete are found in member initializer list

[• d.zobnin.bugzilla]

Summary

Fix for a case found by fuzzing PR23057 (comment #33 https://llvm.org/bugs/show_bug.cgi?id=23057#c33). Diagnose and consume unexpected ::new and ::delete tokens to prevent infinite loop in parsing a member initializer list.

[rsmith]

This isn't quite the right way to fix this issue.

lib/Parse/ParseDeclCXX.cpp

3206–3209 ↗

(On Diff #44971)

This is the right place to fix the bug. We should not enter this codepath if the previous initializer was not valid, and should instead fall into the else below to skip the rest of the initializers. (Delete the else { so that MemInit is still in scope here, and don't enter this block if MemInit.isInvalid(). Please also suppress the err_expected_either diagnostic below in that case.)

[• d.zobnin.bugzilla]

Thank you for the review! Updated the patch: parser now skips the rest of the initializers if the previous one was invalid, added several checks to the test.

[rsmith]

LGTM, thanks! Do you need someone to commit for you?

[• d.zobnin.bugzilla]

I am going to ask @ABataev to commit this. Hope to get commit access soon (this is my 5th patch) :)

cfe/trunk/lib/Parse/ParseDeclCXX.cpp

void Parser::ParseConstructorInitializer(Decl *ConstructorDecl) {
   SmallVector<CXXCtorInitializer*, 4> MemInitializers;
   bool AnyErrors = false;
 
   do {
     if (Tok.is(tok::code_completion)) {
       Actions.CodeCompleteConstructorInitializer(ConstructorDecl,
                                                  MemInitializers);
       return cutOffParsing();
-    } else {
-      MemInitResult MemInit = ParseMemInitializer(ConstructorDecl);
-      if (!MemInit.isInvalid())
-        MemInitializers.push_back(MemInit.get());
-      else
-        AnyErrors = true;
-    }
-    
+    }
+
     if (Tok.is(tok::comma))
       ConsumeToken();
     else if (Tok.is(tok::l_brace))
       break;
-    // If the next token looks like a base or member initializer, assume that
-    // we're just missing a comma.
-    else if (Tok.isOneOf(tok::identifier, tok::coloncolon)) {
+    // If the previous initializer was valid and the next token looks like a
+    // base or member initializer, assume that we're just missing a comma.
+    else if (!MemInit.isInvalid() &&
+             Tok.isOneOf(tok::identifier, tok::coloncolon)) {
       SourceLocation Loc = PP.getLocForEndOfToken(PrevTokLocation);
       Diag(Loc, diag::err_ctor_init_missing_comma)
         << FixItHint::CreateInsertion(Loc, ", ");
     } else {
       // Skip over garbage, until we get to '{'.  Don't eat the '{'.
+      if (!MemInit.isInvalid())
         Diag(Tok.getLocation(), diag::err_expected_either) << tok::l_brace
                                                            << tok::comma;
       SkipUntil(tok::l_brace, StopAtSemi | StopBeforeMatch);
       break;
     }
   } while (true);
 
   Actions.ActOnMemInitializers(ConstructorDecl, ColonLoc, MemInitializers,
                                AnyErrors);
 }

cfe/trunk/test/Parser/cxx-invalid-function-decl.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check that "::new" and "::delete" in member initializer list are diagnosed
+// correctly and don't lead to infinite loop on parsing.
+
+// Error: X() (initializer on non-constructor), "::new" is skipped.
+void f1() : X() ::new{}; // expected-error{{only constructors take base initializers}}
+
+// Errors: first "::delete" and initializer on non-constructor, others skipped.
+void f2() : ::delete, ::new, X() ::new ::delete{} // expected-error{{expected class member or base class name}}
+                                                  // expected-error@-1{{only constructors take base initializers}}
+
+// Errors: the '::' token, "::delete" and initializer on non-constructor, others skipped.
+void f3() : ::, ::delete X(), ::new {}; // expected-error2{{expected class member or base class name}}
+                                        // expected-error@-1{{only constructors take base initializers}}
+
+template <class T>
+struct Base1 {
+  T x1;
+  Base1(T a1) : x1(a1) {}
+};
+
+template <class T>
+struct Base2 {
+  T x2;
+  Base2(T a2) : x2(a2) {}
+};
+
+struct S : public Base1<int>, public Base2<float> {
+  int x;
+
+  // 1-st initializer is correct (just missing ','), 2-nd incorrect, skip other.
+  S() : ::Base1<int>(0) ::new, ::Base2<float>(1.0) ::delete x(2) {} // expected-error{{expected class member or base class name}}
+                                                                    // expected-error@-1{{missing ',' between base or member initializers}}
+
+  // 1-st and 2-nd are correct, errors: '::' and "::new", others skipped.
+  S(int a) : Base1<int>(a), ::Base2<float>(1.0), ::, // expected-error{{expected class member or base class name}}
+             ::new, ! ::delete, ::Base2<() x(3) {}   // expected-error{{expected class member or base class name}}
+
+  // All initializers are correct, nothing to skip, diagnose 2 missing commas.
+  S(const S &) : Base1<int>(0) ::Base2<float>(1.0) x(2) {} // expected-error2{{missing ',' between base or member initializers}}
+};