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); // [email protected]: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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
