steakhal created this revision.
steakhal added reviewers: teemperor, shafik, balazske, a.sidorin, martong.
Herald added subscribers: whisperity, rnkovacs.
steakhal requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
In the case of `TypedefDecl`s we set the `DeclContext` after we imported it.
It turns out, it could lead to null pointer dereferences during the cleanup
part of a failed import.
This patch demonstrates this issue and fixes it by checking if the DeclContext
is available or not.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D102640
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5225,6 +5225,40 @@
EXPECT_TRUE(ImportedOK);
}
+TEST_P(ErrorHandlingTest, ODRViolationWithinTypedefDecls) {
+ // Importing `z` should fail - instead of crashing - due to an ODR violation.
+ // The `bar::e` typedef sets it's DeclContext after the import is done.
+ // However, if the importation fails, it will be left as a nullptr.
+ // During the cleanup of the failed import, we should check whether the
+ // DeclContext is null or not - instead of dereferencing that
unconditionally.
+ constexpr auto ToTUCode = R"(
+ namespace X {
+ struct bar {
+ int odr_violation;
+ };
+ })";
+ constexpr auto FromTUCode = R"(
+ namespace X {
+ enum b {};
+ struct bar {
+ typedef b e;
+ static e d;
+ };
+ }
+ int z = X::bar::d;
+ )";
+ Decl *ToTU = getToTuDecl(ToTUCode, Lang_CXX11);
+ static_cast<void>(ToTU);
+ Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11);
+ auto *FromZ =
+ FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("z")));
+ ASSERT_TRUE(FromZ);
+ ASSERT_TRUE(FromZ->hasInit());
+
+ auto *ImportedZ = Import(FromZ, Lang_CXX11);
+ EXPECT_FALSE(ImportedZ);
+}
+
// An error should be set for a class if it had a previous import with an error
// from another TU.
TEST_P(ErrorHandlingTest,
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8383,7 +8383,11 @@
// traverse of the 'to' context).
auto PosF = ImportedFromDecls.find(ToD);
if (PosF != ImportedFromDecls.end()) {
- SharedState->removeDeclFromLookup(ToD);
+ // In the case of TypedefNameDecl we create the Decl first and only
+ // then we import and set its DeclContext. So, the DC might not be set
+ // when we reach here.
+ if (ToD->getDeclContext())
+ SharedState->removeDeclFromLookup(ToD);
ImportedFromDecls.erase(PosF);
}
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5225,6 +5225,40 @@
EXPECT_TRUE(ImportedOK);
}
+TEST_P(ErrorHandlingTest, ODRViolationWithinTypedefDecls) {
+ // Importing `z` should fail - instead of crashing - due to an ODR violation.
+ // The `bar::e` typedef sets it's DeclContext after the import is done.
+ // However, if the importation fails, it will be left as a nullptr.
+ // During the cleanup of the failed import, we should check whether the
+ // DeclContext is null or not - instead of dereferencing that unconditionally.
+ constexpr auto ToTUCode = R"(
+ namespace X {
+ struct bar {
+ int odr_violation;
+ };
+ })";
+ constexpr auto FromTUCode = R"(
+ namespace X {
+ enum b {};
+ struct bar {
+ typedef b e;
+ static e d;
+ };
+ }
+ int z = X::bar::d;
+ )";
+ Decl *ToTU = getToTuDecl(ToTUCode, Lang_CXX11);
+ static_cast<void>(ToTU);
+ Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11);
+ auto *FromZ =
+ FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("z")));
+ ASSERT_TRUE(FromZ);
+ ASSERT_TRUE(FromZ->hasInit());
+
+ auto *ImportedZ = Import(FromZ, Lang_CXX11);
+ EXPECT_FALSE(ImportedZ);
+}
+
// An error should be set for a class if it had a previous import with an error
// from another TU.
TEST_P(ErrorHandlingTest,
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8383,7 +8383,11 @@
// traverse of the 'to' context).
auto PosF = ImportedFromDecls.find(ToD);
if (PosF != ImportedFromDecls.end()) {
- SharedState->removeDeclFromLookup(ToD);
+ // In the case of TypedefNameDecl we create the Decl first and only
+ // then we import and set its DeclContext. So, the DC might not be set
+ // when we reach here.
+ if (ToD->getDeclContext())
+ SharedState->removeDeclFromLookup(ToD);
ImportedFromDecls.erase(PosF);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
