[clang] [clang][ASTImporter] Invalidate ImportedTypes cache on Decl import failure (PR #214008)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-static-analyzer-1

Author: guillem-bartrina-sonarsource

<details>
<summary>Changes</summary>

A TagDecl's type can get cached as successfully imported in ASTImporter::ImportedTypes before the Decl's own import fails, if a member referencing the type (e.g. an implicit copy constructor) is imported first. That stale entry was never invalidated, so later references to the same type (directly, or via structural-equivalence comparisons on lambda closures) could silently resolve to a half-built Decl and crash instead of failing cleanly.

Add a unit test and a CTU regression test reproducing the crash.

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


3 Files Affected:

- (modified) clang/lib/AST/ASTImporter.cpp (+7) 
- (added) clang/test/Analysis/ctu/regression/lambda-import-corruption.cpp (+93) 
- (modified) clang/unittests/AST/ASTImporterTest.cpp (+37) 


``````````diff
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 3ad71a223903c..ffaca93b5d2ac 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -10038,6 +10038,13 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) {
       auto *ToD = CreatedToD;
       ImportedDecls.erase(Pos);
 
+      // Also scrub the imported type mapping, if applicable. Import(Type*) can
+      // cache a type mapping to a declaration that ultimately fails.
+      if (const auto *FromTD = dyn_cast<TagDecl>(FromD))
+        if (const Type *FromTy =
+                getFromContext().getCanonicalTagType(FromTD).getTypePtr())
+          ImportedTypes.erase(FromTy);
+
       // ImportedDecls and ImportedFromDecls are not symmetric.  It may happen
       // (e.g. with namespaces) that several decls from the 'from' context are
       // mapped to the same decl in the 'to' context.  If we removed entries
diff --git a/clang/test/Analysis/ctu/regression/lambda-import-corruption.cpp b/clang/test/Analysis/ctu/regression/lambda-import-corruption.cpp
new file mode 100644
index 0000000000000..2d81b9697bdf2
--- /dev/null
+++ b/clang/test/Analysis/ctu/regression/lambda-import-corruption.cpp
@@ -0,0 +1,93 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// Pathological case: a lambda's closure is created as a Decl before its
+// members are imported. If a member unrelated to the eventual failure
+// (e.g. the implicit copy constructor) imports successfully first, that
+// success permanently caches the closure's type as "imported", before the
+// member that actually fails is even reached. That cache was never
+// invalidated when the closure's own import later fails, so anything that
+// subsequently needs the same type (e.g. the DeclRefExpr inside
+// `decltype(func(...))` on `rudolf`, below) silently got the half-built
+// closure back instead of a clean failure, producing an inconsistent node
+// that crashed.
+
+
+// RUN: %clang_cc1 -std=c++20 -emit-pch -o %t/api.cpp.ast %t/api.cpp
+// RUN: %clang_cc1 -std=c++20 -emit-pch -o %t/isolate.cpp.ast %t/isolate.cpp
+
+// RUN: %clang_extdef_map %t/api.cpp -- -std=c++20 > %t/externalDefMap.tmp.txt
+// RUN: %clang_extdef_map %t/isolate.cpp -- -std=c++20 >> %t/externalDefMap.tmp.txt
+// On windows, absolute paths generated by extdef_map are not recognized,
+// so CSA prepends the workdir path to them. Force relative paths to work
+// around this issue.
+// RUN: sed -e 's| .*api\.cpp| api.cpp.ast|' -e 's| .*isolate\.cpp| isolate.cpp.ast|' \
+// RUN:   %t/externalDefMap.tmp.txt > %t/externalDefMap.txt
+
+// RUN: %clang_analyze_cc1 -std=c++20 \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t \
+// RUN:   -verify %t/main.cpp
+
+//--- main.cpp
+
+namespace ns {
+
+inline constexpr auto func = []<class T>(const T p) {};
+
+}
+
+void import_api(int v);
+
+void entrypoint() {
+  import_api(0); // expected-warning at isolate.cpp:20 {{Division by zero}}
+}
+
+//--- api.cpp
+
+template <class> int declval();
+
+namespace ns {
+int import_ns;
+
+// This closure fails to import: its call operator's trailing
+// requires-clause has no importer support.
+inline constexpr auto func = []<class T>(const T p) requires requires { 0; } {};
+
+// The DeclRefExpr for `func` in this decltype independently re-resolves
+// the closure's type after `func` itself was merged away above.
+template <class K> decltype(func(declval<K>())) rudolf(int v);
+
+} // namespace ns
+
+void import_isolate(int v);
+
+void import_api(int v) {
+  (void)ns::import_ns;
+  import_isolate(v);
+}
+
+//--- isolate.cpp
+
+template <class> int declval();
+
+namespace ns {
+int import_ns;
+
+constexpr auto func = []<class T>(const T p) requires requires { 0; } {};
+
+// Structural equivalence of the return type accesses the closure's
+// definition through its type -- an access that assumes the closure is
+// intact.
+template <class K> decltype(func(declval<K>())) rudolf(int v) { // no-crash
+  (void)(42 / v);
+}
+
+} // namespace ns
+
+void import_isolate(int v) {
+  (void)ns::import_ns;
+  (void)(42 / v); // raises "Division by zero"
+}
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 503f5da8af90f..d5fd9ec1d17d2 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -6608,6 +6608,43 @@ TEST_P(ErrorHandlingTest, ErrorIsPropagatedFromMemberToClass) {
   EXPECT_FALSE(ImportedOK);
 }
 
+// A member whose signature refers back to the enclosing class (e.g. a
+// copy constructor's `const Self&` parameter) can succeed and cache the
+// class's *type* before a later, failing member causes the class's own
+// Decl import to fail as a whole. Check that this doesn't leave a stale,
+// "successfully imported" entry for the class's type behind: any later,
+// independent request to import that type must also fail, not silently
+// hand back the half-built class.
+TEST_P(ErrorHandlingTest, ImportedTypeCacheIsInvalidatedOnFailure) {
+  TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+      class X {
+        void ok(const X &) {}          // Succeeds; imports X's own type
+                                        // as a side effect, before X's
+                                        // own import is known to fail.
+        void bad() { )") + ErroneousStmt + R"( }  // Fails to import.
+      };
+      )",
+                                          Lang_CXX03);
+  auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("X")));
+
+  CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX03);
+  EXPECT_FALSE(ImportedX); // X itself fails to import.
+
+  // The bug: without the fix, a later, independent request to import X's
+  // type silently succeeds, returning the half-built X as if nothing had
+  // gone wrong, because ASTImporter::ImportedTypes was never scrubbed
+  // when X's own Decl import failed.
+  ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+  const Type *FromXTy =
+      FromTU->getASTContext().getCanonicalTagType(FromX)->getTypePtr();
+  ASSERT_TRUE(FromXTy);
+  Expected<const Type *> ToTyOrErr = Importer->Import(FromXTy);
+  EXPECT_FALSE(static_cast<bool>(ToTyOrErr));
+  if (!ToTyOrErr)
+    llvm::consumeError(ToTyOrErr.takeError());
+}
+
 // Check that an error propagates to the dependent AST nodes.
 // In the below code it means that an error in X should propagate to A.
 // And even to F since the containing A is erroneous.

``````````

</details>


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