[clang] [clang][NFC] Prevent scope pollution from repeat type specifiers (PR #193144)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Oliver Hunt (ojhunt)

<details>
<summary>Changes</summary>

Fixes #<!-- -->187664

When parsing `type-specifier {class,union,struct,enum,etc} nested-name` ParseClassSpecifier and ParseEnumSpecifier both operated on the current declaration scope on the assumption that they were the only type specifier. Of course in incorrect code that assumption is false, and as a result when parsing the name specifier they would pollute the the real scope.

This is not relevant to the semantic correctness: the error is detected and reported. The problem is that the subsequent state is not correct, though not in a way that impacts functional behavior of release builds.

In assertion builds however this is detected (via a somewhat obtuse path) when we attempt to plant namespace location information from the invalid declaration on the initial declaration.

This PR fixes the state corruption by moving the name parsing in enum and record type specifiers into a throwaway scope if the declaration already has a type specifier.

---
Full diff: https://github.com/llvm/llvm-project/pull/193144.diff


3 Files Affected:

- (modified) clang/lib/Parse/ParseDecl.cpp (+5-1) 
- (modified) clang/lib/Parse/ParseDeclCXX.cpp (+5-1) 
- (added) clang/test/SemaCXX/gh187664.cpp (+76) 


``````````diff
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index e2ac86bc5e064..4a7e95e3cfc6a 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5076,7 +5076,11 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, DeclSpec &DS,
                          (AllowEnumSpecifier == AllowDefiningTypeSpec::Yes ||
                           CanBeOpaqueEnumDeclaration);
 
-  CXXScopeSpec &SS = DS.getTypeSpecScope();
+  // We use a temporary scope when parsing the name specifier for a
+  // declaration with additional invalid type specifiers
+  CXXScopeSpec InvalidDeclScope;
+  CXXScopeSpec &SS =
+      DS.hasTypeSpecifier() ? InvalidDeclScope : DS.getTypeSpecScope();
   if (getLangOpts().CPlusPlus) {
     // "enum foo : bar;" is not a potential typo for "enum foo::bar;".
     ColonProtectionRAIIObject X(*this);
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 683a1b4695625..1507923cc8fa6 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -1694,8 +1694,12 @@ void Parser::ParseClassSpecifier(tok::TokenKind TagTokKind,
   PreserveAtomicIdentifierInfoRAII AtomicTokenGuard(
       Tok, ShouldChangeAtomicToIdentifier);
 
+  // We use a temporary scope when parsing the name specifier for a
+  // declaration with additional invalid type specifiers
+  CXXScopeSpec InvalidDeclScope;
+  CXXScopeSpec &SS =
+      DS.hasTypeSpecifier() ? InvalidDeclScope : DS.getTypeSpecScope();
   // Parse the (optional) nested-name-specifier.
-  CXXScopeSpec &SS = DS.getTypeSpecScope();
   if (getLangOpts().CPlusPlus) {
     // "FOO : BAR" is not a potential typo for "FOO::BAR".  In this context it
     // is a base-specifier-list.
diff --git a/clang/test/SemaCXX/gh187664.cpp b/clang/test/SemaCXX/gh187664.cpp
new file mode 100644
index 0000000000000..812881302d239
--- /dev/null
+++ b/clang/test/SemaCXX/gh187664.cpp
@@ -0,0 +1,76 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct A { // #A
+};
+
+struct B {
+  struct C{}; // #B_C
+  enum E{};
+};
+
+namespace NS {
+  struct D{};
+}
+
+enum E {}; // #E
+
+typedef int T;
+using U = int;
+
+// Basic sanity check on the original fuzzing generated syntax
+template <class T = U struct B::C> void test_template();
+// expected-error at -1{{cannot combine with previous 'type-name' declaration specifier}}
+
+// The bug we're looking at is the result of parsing repeated named type
+// specifiers. All the record types actually go through a single path, so
+// we don't repeat all of them. The first few are a basic sanity check of
+// each one anyway
+A struct  B::C test01;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+A union   B::C test02;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+// expected-error at -2 {{use of 'C' with tag type that does not match previous declaration}}
+// expected-note@#B_C {{previous use is here}}
+A class   B::C test03;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+A struct NS::D test04;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+A struct   ::A test05;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+T struct  B::C test06;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+U struct  B::C test07;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+
+// enums _do_ need separate testing as they have a different syntax, and so
+// take a different path through the parser
+E struct  B::C test08;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+A enum     ::A test09;
+// expected-error at -1 {{use of 'A' with tag type that does not match previous declaration}}
+// expected-note@#A {{previous use is here}}
+
+A enum     ::E test10;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+E enum    B::E test11;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+E enum     ::E test12;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+T enum     ::E test13;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+E struct   ::E test14;
+// expected-error at -1 {{use of 'E' with tag type that does not match previous declaration}}
+// expected-note@#E {{previous use is here}}
+
+// These cases also _technically_ go wrong, but when the initial type specifier
+// is namespaced, we get the required info from the parser directly, which avoids
+// the reading from the corrupted scope
+B::C struct ::B   test15;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+B::C struct B::C  test16;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+B::C struct NS::D test17;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}
+
+using Test18 = A struct ::A;
+// expected-error at -1 {{cannot combine with previous 'type-name' declaration specifier}}

``````````

</details>


https://github.com/llvm/llvm-project/pull/193144