[clang] Fix possible nullptr deref in BuildCXXNestedNameSpecifier

[ilovepi]

There is a possible nullptr deref in BuildCXXNestedNameSpecifier when calling ExtendNestedNameSpecifier or using isa<>. This initially showed up as a crash in clangd, that didn't manifest in when compiling w/ clang. The reduced test case added in this patch, however does expose the issue in clang. Testing locally shows that both this test case and the original clangd issue are fixed by checking the validity of the pointer before trying to dispatch. Since all code paths require the pointer to be valid (usually by virtue of a dyn_cast or isa<> check), there should be no functional difference.

Fixes #166843

[llvmbot]

@llvm/pr-subscribers-clang

Author: Paul Kirth (ilovepi)

Changes

There is a possible nullptr deref in BuildCXXNestedNameSpecifier when calling ExtendNestedNameSpecifier or using isa<>. This initially showed up as a crash in clangd, that didn't manifest in when compiling w/ clang. The reduced test case added in this patch, however does expose the issue in clang. Testing locally shows that both this test case and the original clangd issue are fixed by checking the validity of the pointer before trying to dispatch. Since all code paths require the pointer to be valid (usually by virtue of a dyn_cast or isa<> check), there should be no functional difference.

Fixes #166843

---

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

1 Files Affected:

• (modified) clang/lib/Sema/SemaCXXScopeSpec.cpp (+20-19)

diff --git a/clang/lib/Sema/SemaCXXScopeSpec.cpp b/clang/lib/Sema/SemaCXXScopeSpec.cpp
index c52fc5bf815af..29e697d9eb029 100644
--- a/clang/lib/Sema/SemaCXXScopeSpec.cpp
+++ b/clang/lib/Sema/SemaCXXScopeSpec.cpp
@@ -779,25 +779,26 @@ bool Sema::BuildCXXNestedNameSpecifier(Scope *S, NestedNameSpecInfo &IdInfo,
   }
 
   if (!Found.empty()) {
-    const auto *ND = Found.getAsSingle<NamedDecl>();
-    if (::ExtendNestedNameSpecifier(*this, SS, ND, IdInfo.IdentifierLoc,
-                                    IdInfo.CCLoc)) {
-      const Type *T = SS.getScopeRep().getAsType();
-      Diag(IdInfo.IdentifierLoc, diag::err_expected_class_or_namespace)
-          << QualType(T, 0) << getLangOpts().CPlusPlus;
-      // Recover with this type if it would be a valid nested name specifier.
-      return !T->getAsCanonical<TagType>();
-    }
-    if (isa<TemplateDecl>(ND)) {
-      ParsedType SuggestedType;
-      DiagnoseUnknownTypeName(IdInfo.Identifier, IdInfo.IdentifierLoc, S, &SS,
-                              SuggestedType);
-    } else {
-      Diag(IdInfo.IdentifierLoc, diag::err_expected_class_or_namespace)
-          << IdInfo.Identifier << getLangOpts().CPlusPlus;
-      if (NamedDecl *ND = Found.getAsSingle<NamedDecl>())
-        Diag(ND->getLocation(), diag::note_entity_declared_at)
-            << IdInfo.Identifier;
+    if (const auto *ND = Found.getAsSingle<NamedDecl>()) {
+      if (::ExtendNestedNameSpecifier(*this, SS, ND, IdInfo.IdentifierLoc,
+                                      IdInfo.CCLoc)) {
+        const Type *T = SS.getScopeRep().getAsType();
+        Diag(IdInfo.IdentifierLoc, diag::err_expected_class_or_namespace)
+            << QualType(T, 0) << getLangOpts().CPlusPlus;
+        // Recover with this type if it would be a valid nested name specifier.
+        return !T->getAsCanonical<TagType>();
+      }
+      if (isa<TemplateDecl>(ND)) {
+        ParsedType SuggestedType;
+        DiagnoseUnknownTypeName(IdInfo.Identifier, IdInfo.IdentifierLoc, S, &SS,
+                                SuggestedType);
+      } else {
+        Diag(IdInfo.IdentifierLoc, diag::err_expected_class_or_namespace)
+            << IdInfo.Identifier << getLangOpts().CPlusPlus;
+        if (NamedDecl *ND = Found.getAsSingle<NamedDecl>())
+          Diag(ND->getLocation(), diag::note_entity_declared_at)
+              << IdInfo.Identifier;
+      }
     }
   } else if (SS.isSet())
     Diag(IdInfo.IdentifierLoc, diag::err_no_member) << IdInfo.Identifier

[mizvekov]

Thanks for working on this.

Please include test case.

[mizvekov]

Please bail out early to reduce the diff.

[ilovepi]

done.

[ilovepi]

Thanks for working on this.

Please include test case.

Thanks for pointing that out. I forgot to add it to git 🤦

[mizvekov]

LGTM, Thanks.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[efriedma-quic]

This looks like we aren't actually diagnosing the error? I'd expect something like error: 'c' is not a class, namespace, or enumeration, which is what we generate in the non-template case.

What happens if you instead write the following?

  ((::c::x));

[ilovepi]

This looks like we aren't actually diagnosing the error? I'd expect something like error: 'c' is not a class, namespace, or enumeration, which is what we generate in the non-template case.

Yeah, that was something I wasn't sure about.

What happens if you instead write the following?

  ((::c::x));

Looks like we report nothing, the way I've structured it. That version still causes the crash w/o the nullptr check, so let me update the check to make sure it works in the expected way.

[ilovepi]

i've updated the early exit by duplicating the diagnostic. I didn't see an obvious way to avoid doing that. Happy to amend if this still isn't quite right.