[clang] 18c84e2 - [clang][dataflow] Fix nullptr dereferencing error.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 7 19:09:48 PST 2022
Author: Yitzhak Mandelbaum
Date: 2022-03-08T03:01:31Z
New Revision: 18c84e2d325fe64f303a13b17addce1c0e6aca23
URL: https://github.com/llvm/llvm-project/commit/18c84e2d325fe64f303a13b17addce1c0e6aca23
DIFF: https://github.com/llvm/llvm-project/commit/18c84e2d325fe64f303a13b17addce1c0e6aca23.diff
LOG: [clang][dataflow] Fix nullptr dereferencing error.
When pre-initializing fields in the environment, the code assumed that all
fields of a struct would be initialized. However, given limits on value
construction, that assumption is incorrect. This patch changes the code to drop
that assumption and thereby avoid dereferencing a nullptr.
Differential Revision: https://reviews.llvm.org/D121158
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/Value.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/Value.h b/clang/include/clang/Analysis/FlowSensitive/Value.h
index 7c02cc6c3505b..98df8f6539f7b 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Value.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Value.h
@@ -201,11 +201,13 @@ class StructValue final : public Value {
return Val->getKind() == Kind::Struct;
}
- /// Returns the child value that is assigned for `D`.
- Value &getChild(const ValueDecl &D) const {
+ /// Returns the child value that is assigned for `D` or null if the child is
+ /// not initialized.
+ Value *getChild(const ValueDecl &D) const {
auto It = Children.find(&D);
- assert(It != Children.end());
- return *It->second;
+ if (It == Children.end())
+ return nullptr;
+ return It->second;
}
/// Assigns `Val` as the child value for `D`.
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 691b1257ba4fb..ec945bac09863 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -366,7 +366,8 @@ void Environment::setValue(const StorageLocation &Loc, Value &Val) {
assert(Field != nullptr);
StorageLocation &FieldLoc = AggregateLoc.getChild(*Field);
MemberLocToStruct[&FieldLoc] = std::make_pair(StructVal, Field);
- setValue(FieldLoc, StructVal->getChild(*Field));
+ if (auto *FieldVal = StructVal->getChild(*Field))
+ setValue(FieldLoc, *FieldVal);
}
}
@@ -485,9 +486,9 @@ Value *Environment::createValueUnlessSelfReferential(
continue;
Visited.insert(FieldType.getCanonicalType());
- FieldValues.insert(
- {Field, createValueUnlessSelfReferential(
- FieldType, Visited, Depth + 1, CreatedValuesCount)});
+ if (auto *FieldValue = createValueUnlessSelfReferential(
+ FieldType, Visited, Depth + 1, CreatedValuesCount))
+ FieldValues.insert({Field, FieldValue});
Visited.erase(FieldType.getCanonicalType());
}
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index de11ccbc6fd5c..51b40f2319878 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -7,7 +7,10 @@
//===----------------------------------------------------------------------===//
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "NoopAnalysis.h"
+#include "TestingSupport.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
#include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -17,6 +20,8 @@ namespace {
using namespace clang;
using namespace dataflow;
+using ::testing::ElementsAre;
+using ::testing::Pair;
class EnvironmentTest : public ::testing::Test {
DataflowAnalysisContext Context;
@@ -54,4 +59,43 @@ TEST_F(EnvironmentTest, FlowCondition) {
EXPECT_FALSE(Env.flowConditionImplies(NotX));
}
+TEST_F(EnvironmentTest, CreateValueRecursiveType) {
+ using namespace ast_matchers;
+
+ std::string Code = R"cc(
+ struct Recursive {
+ bool X;
+ Recursive *R;
+ };
+ )cc";
+
+ auto Unit =
+ tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++11"});
+ auto &Context = Unit->getASTContext();
+
+ ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+ auto Results =
+ match(qualType(hasDeclaration(recordDecl(
+ hasName("Recursive"),
+ has(fieldDecl(hasName("R")).bind("field-r")))))
+ .bind("target"),
+ Context);
+ const QualType *Ty = selectFirst<QualType>("target", Results);
+ const FieldDecl *R = selectFirst<FieldDecl>("field-r", Results);
+ ASSERT_TRUE(Ty != nullptr && !Ty->isNull());
+ ASSERT_TRUE(R != nullptr);
+
+ // Verify that the struct and the field (`R`) with first appearance of the
+ // type is created successfully.
+ Value *Val = Env.createValue(*Ty);
+ ASSERT_NE(Val, nullptr);
+ StructValue *SVal = clang::dyn_cast<StructValue>(Val);
+ ASSERT_NE(SVal, nullptr);
+ Val = SVal->getChild(*R);
+ ASSERT_NE(Val, nullptr);
+ PointerValue *PV = clang::dyn_cast<PointerValue>(Val);
+ EXPECT_NE(PV, nullptr);
+}
+
} // namespace
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 563de6e675792..49aaca9a7588c 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -181,7 +181,7 @@ TEST_F(TransferTest, StructVarDecl) {
cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
- const auto *BarVal = cast<IntegerValue>(&FooVal->getChild(*BarDecl));
+ const auto *BarVal = cast<IntegerValue>(FooVal->getChild(*BarDecl));
EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
});
}
@@ -227,7 +227,7 @@ TEST_F(TransferTest, ClassVarDecl) {
cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
- const auto *BarVal = cast<IntegerValue>(&FooVal->getChild(*BarDecl));
+ const auto *BarVal = cast<IntegerValue>(FooVal->getChild(*BarDecl));
EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
});
}
@@ -351,27 +351,27 @@ TEST_F(TransferTest, SelfReferentialReferenceVarDecl) {
cast<StructValue>(Env.getValue(FooVal->getPointeeLoc()));
const auto *BarVal =
- cast<ReferenceValue>(&FooPointeeVal->getChild(*BarDecl));
+ cast<ReferenceValue>(FooPointeeVal->getChild(*BarDecl));
const auto *BarPointeeVal =
cast<StructValue>(Env.getValue(BarVal->getPointeeLoc()));
const auto *FooRefVal =
- cast<ReferenceValue>(&BarPointeeVal->getChild(*FooRefDecl));
+ cast<ReferenceValue>(BarPointeeVal->getChild(*FooRefDecl));
const StorageLocation &FooRefPointeeLoc = FooRefVal->getPointeeLoc();
EXPECT_THAT(Env.getValue(FooRefPointeeLoc), IsNull());
const auto *FooPtrVal =
- cast<PointerValue>(&BarPointeeVal->getChild(*FooPtrDecl));
+ cast<PointerValue>(BarPointeeVal->getChild(*FooPtrDecl));
const StorageLocation &FooPtrPointeeLoc = FooPtrVal->getPointeeLoc();
EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull());
const auto *BazRefVal =
- cast<ReferenceValue>(&BarPointeeVal->getChild(*BazRefDecl));
+ cast<ReferenceValue>(BarPointeeVal->getChild(*BazRefDecl));
const StorageLocation &BazRefPointeeLoc = BazRefVal->getPointeeLoc();
EXPECT_THAT(Env.getValue(BazRefPointeeLoc), NotNull());
const auto *BazPtrVal =
- cast<PointerValue>(&BarPointeeVal->getChild(*BazPtrDecl));
+ cast<PointerValue>(BarPointeeVal->getChild(*BazPtrDecl));
const StorageLocation &BazPtrPointeeLoc = BazPtrVal->getPointeeLoc();
EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull());
});
@@ -508,27 +508,27 @@ TEST_F(TransferTest, SelfReferentialPointerVarDecl) {
cast<StructValue>(Env.getValue(FooVal->getPointeeLoc()));
const auto *BarVal =
- cast<PointerValue>(&FooPointeeVal->getChild(*BarDecl));
+ cast<PointerValue>(FooPointeeVal->getChild(*BarDecl));
const auto *BarPointeeVal =
cast<StructValue>(Env.getValue(BarVal->getPointeeLoc()));
const auto *FooRefVal =
- cast<ReferenceValue>(&BarPointeeVal->getChild(*FooRefDecl));
+ cast<ReferenceValue>(BarPointeeVal->getChild(*FooRefDecl));
const StorageLocation &FooRefPointeeLoc = FooRefVal->getPointeeLoc();
EXPECT_THAT(Env.getValue(FooRefPointeeLoc), IsNull());
const auto *FooPtrVal =
- cast<PointerValue>(&BarPointeeVal->getChild(*FooPtrDecl));
+ cast<PointerValue>(BarPointeeVal->getChild(*FooPtrDecl));
const StorageLocation &FooPtrPointeeLoc = FooPtrVal->getPointeeLoc();
EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull());
const auto *BazRefVal =
- cast<ReferenceValue>(&BarPointeeVal->getChild(*BazRefDecl));
+ cast<ReferenceValue>(BarPointeeVal->getChild(*BazRefDecl));
const StorageLocation &BazRefPointeeLoc = BazRefVal->getPointeeLoc();
EXPECT_THAT(Env.getValue(BazRefPointeeLoc), NotNull());
const auto *BazPtrVal =
- cast<PointerValue>(&BarPointeeVal->getChild(*BazPtrDecl));
+ cast<PointerValue>(BarPointeeVal->getChild(*BazPtrDecl));
const StorageLocation &BazPtrPointeeLoc = BazPtrVal->getPointeeLoc();
EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull());
});
@@ -888,7 +888,7 @@ TEST_F(TransferTest, StructParamDecl) {
cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
- const auto *BarVal = cast<IntegerValue>(&FooVal->getChild(*BarDecl));
+ const auto *BarVal = cast<IntegerValue>(FooVal->getChild(*BarDecl));
EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
});
}
@@ -1000,7 +1000,7 @@ TEST_F(TransferTest, StructMember) {
const auto *FooLoc = cast<AggregateStorageLocation>(
Env.getStorageLocation(*FooDecl, SkipPast::None));
const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
- const auto *BarVal = cast<IntegerValue>(&FooVal->getChild(*BarDecl));
+ const auto *BarVal = cast<IntegerValue>(FooVal->getChild(*BarDecl));
const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
ASSERT_THAT(BazDecl, NotNull());
@@ -1048,7 +1048,7 @@ TEST_F(TransferTest, ClassMember) {
const auto *FooLoc = cast<AggregateStorageLocation>(
Env.getStorageLocation(*FooDecl, SkipPast::None));
const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
- const auto *BarVal = cast<IntegerValue>(&FooVal->getChild(*BarDecl));
+ const auto *BarVal = cast<IntegerValue>(FooVal->getChild(*BarDecl));
const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
ASSERT_THAT(BazDecl, NotNull());
@@ -1095,7 +1095,7 @@ TEST_F(TransferTest, ReferenceMember) {
const auto *FooLoc = cast<AggregateStorageLocation>(
Env.getStorageLocation(*FooDecl, SkipPast::None));
const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
- const auto *BarVal = cast<ReferenceValue>(&FooVal->getChild(*BarDecl));
+ const auto *BarVal = cast<ReferenceValue>(FooVal->getChild(*BarDecl));
const auto *BarPointeeVal =
cast<IntegerValue>(Env.getValue(BarVal->getPointeeLoc()));
@@ -1173,7 +1173,7 @@ TEST_F(TransferTest, StructThisMember) {
const auto *BazLoc =
cast<ScalarStorageLocation>(&QuxLoc->getChild(*BazDecl));
- const auto *BazVal = cast<IntegerValue>(&QuxVal->getChild(*BazDecl));
+ const auto *BazVal = cast<IntegerValue>(QuxVal->getChild(*BazDecl));
EXPECT_EQ(Env.getValue(*BazLoc), BazVal);
const ValueDecl *QuuxDecl = findValueDecl(ASTCtx, "Quux");
@@ -1249,7 +1249,7 @@ TEST_F(TransferTest, ClassThisMember) {
const auto *BazLoc =
cast<ScalarStorageLocation>(&QuxLoc->getChild(*BazDecl));
- const auto *BazVal = cast<IntegerValue>(&QuxVal->getChild(*BazDecl));
+ const auto *BazVal = cast<IntegerValue>(QuxVal->getChild(*BazDecl));
EXPECT_EQ(Env.getValue(*BazLoc), BazVal);
const ValueDecl *QuuxDecl = findValueDecl(ASTCtx, "Quux");
@@ -1399,7 +1399,7 @@ TEST_F(TransferTest, TemporaryObject) {
cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
- const auto *BarVal = cast<IntegerValue>(&FooVal->getChild(*BarDecl));
+ const auto *BarVal = cast<IntegerValue>(FooVal->getChild(*BarDecl));
EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
});
}
@@ -1438,7 +1438,7 @@ TEST_F(TransferTest, ElidableConstructor) {
cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
- const auto *BarVal = cast<IntegerValue>(&FooVal->getChild(*BarDecl));
+ const auto *BarVal = cast<IntegerValue>(FooVal->getChild(*BarDecl));
EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
},
LangStandard::lang_cxx14);
@@ -1706,7 +1706,7 @@ TEST_F(TransferTest, BindTemporary) {
*cast<StructValue>(Env.getValue(*FooDecl, SkipPast::None));
const auto *BarVal =
cast<IntegerValue>(Env.getValue(*BarDecl, SkipPast::None));
- EXPECT_EQ(BarVal, &FooVal.getChild(*BazDecl));
+ EXPECT_EQ(BarVal, FooVal.getChild(*BazDecl));
});
}
@@ -1982,12 +1982,12 @@ TEST_F(TransferTest, AggregateInitialization) {
cast<StructValue>(Env.getValue(*QuuxDecl, SkipPast::None));
ASSERT_THAT(QuuxVal, NotNull());
- const auto *BazVal = cast<StructValue>(&QuuxVal->getChild(*BazDecl));
+ const auto *BazVal = cast<StructValue>(QuuxVal->getChild(*BazDecl));
ASSERT_THAT(BazVal, NotNull());
- EXPECT_EQ(&QuuxVal->getChild(*BarDecl), BarArgVal);
- EXPECT_EQ(&BazVal->getChild(*FooDecl), FooArgVal);
- EXPECT_EQ(&QuuxVal->getChild(*QuxDecl), QuxArgVal);
+ EXPECT_EQ(QuuxVal->getChild(*BarDecl), BarArgVal);
+ EXPECT_EQ(BazVal->getChild(*FooDecl), FooArgVal);
+ EXPECT_EQ(QuuxVal->getChild(*QuxDecl), QuxArgVal);
});
}
}
@@ -2383,7 +2383,7 @@ TEST_F(TransferTest, AssignMemberBeforeCopy) {
const auto *A2Val =
cast<StructValue>(Env.getValue(*A2Decl, SkipPast::None));
- EXPECT_EQ(&A2Val->getChild(*FooDecl), BarVal);
+ EXPECT_EQ(A2Val->getChild(*FooDecl), BarVal);
});
}
More information about the cfe-commits
mailing list