[clang] af98b0a - [clang][dataflow] Add analysis that detects unsafe accesses to optionals
Stanislav Gatev via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 10 03:07:28 PST 2022
Author: Stanislav Gatev
Date: 2022-03-10T11:05:31Z
New Revision: af98b0af6705df3859ee3c98525b57025d40d9e8
URL: https://github.com/llvm/llvm-project/commit/af98b0af6705df3859ee3c98525b57025d40d9e8
DIFF: https://github.com/llvm/llvm-project/commit/af98b0af6705df3859ee3c98525b57025d40d9e8.diff
LOG: [clang][dataflow] Add analysis that detects unsafe accesses to optionals
This commit reverts e0cc28dfdc67105974924cce42bb8c85bd44925a and moves
UncheckedOptionalAccessModelTest.cpp into clang/unittests/Analysis/FlowSensitive,
to avoid build failures. The test will be moved back into a Models subdir
in a follow up patch that will address the build configuration issues.
Original description:
Adds a dataflow analysis that detects unsafe accesses to values of type
`std::optional`, `absl::optional`, or `base::Optional`.
Reviewed-by: ymandel, xazax.hun
Differential Revision: https://reviews.llvm.org/D121197
Added:
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Modified:
clang/lib/Analysis/FlowSensitive/CMakeLists.txt
clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
new file mode 100644
index 0000000000000..e5ad0cadd7bd9
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -0,0 +1,40 @@
+#ifndef CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H
+#define CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/SourceLocationsLattice.h"
+
+namespace clang {
+namespace dataflow {
+
+/// Dataflow analysis that discovers unsafe accesses of optional values and
+/// adds the respective source locations to the lattice.
+///
+/// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
+///
+/// FIXME: Consider separating the models from the unchecked access analysis.
+class UncheckedOptionalAccessModel
+ : public DataflowAnalysis<UncheckedOptionalAccessModel,
+ SourceLocationsLattice> {
+public:
+ explicit UncheckedOptionalAccessModel(ASTContext &AstContext);
+
+ static SourceLocationsLattice initialElement() {
+ return SourceLocationsLattice();
+ }
+
+ void transfer(const Stmt *Stmt, SourceLocationsLattice &State,
+ Environment &Env);
+
+private:
+ MatchSwitch<TransferState<SourceLocationsLattice>> TransferMatchSwitch;
+};
+
+} // namespace dataflow
+} // namespace clang
+
+#endif // CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H
diff --git a/clang/lib/Analysis/FlowSensitive/CMakeLists.txt b/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
index cfe3c8e77b4fd..4a8c63b3db21e 100644
--- a/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
@@ -12,3 +12,5 @@ add_clang_library(clangAnalysisFlowSensitive
clangAST
clangBasic
)
+
+add_subdirectory(Models)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt
new file mode 100644
index 0000000000000..5bed00c4bfdc6
--- /dev/null
+++ b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt
@@ -0,0 +1,10 @@
+add_clang_library(clangAnalysisFlowSensitiveModels
+ UncheckedOptionalAccessModel.cpp
+
+ LINK_LIBS
+ clangAnalysis
+ clangAnalysisFlowSensitive
+ clangAST
+ clangASTMatchers
+ clangBasic
+ )
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
new file mode 100644
index 0000000000000..bb6aa5ff74e2c
--- /dev/null
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -0,0 +1,136 @@
+#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/SourceLocationsLattice.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include <cassert>
+
+namespace clang {
+namespace dataflow {
+namespace {
+
+using namespace ::clang::ast_matchers;
+
+using LatticeTransferState = TransferState<SourceLocationsLattice>;
+
+static auto optionalClass() {
+ return classTemplateSpecializationDecl(
+ anyOf(hasName("std::optional"), hasName("std::__optional_storage_base"),
+ hasName("__optional_destruct_base"), hasName("absl::optional"),
+ hasName("base::Optional")),
+ hasTemplateArgument(0, refersToType(type().bind("T"))));
+}
+
+static auto hasOptionalType() { return hasType(optionalClass()); }
+
+static auto isOptionalMemberCallWithName(llvm::StringRef MemberName) {
+ return cxxMemberCallExpr(
+ on(expr(unless(cxxThisExpr()))),
+ callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass()))));
+}
+
+static auto isOptionalOperatorCallWithName(llvm::StringRef OperatorName) {
+ return cxxOperatorCallExpr(hasOverloadedOperatorName(OperatorName),
+ callee(cxxMethodDecl(ofClass(optionalClass()))));
+}
+
+/// Returns the symbolic value that represents the "has_value" property of the
+/// optional value `Val`. Returns null if `Val` is null.
+static BoolValue *getHasValue(Value *Val) {
+ if (auto *OptionalVal = cast_or_null<StructValue>(Val)) {
+ return cast<BoolValue>(OptionalVal->getProperty("has_value"));
+ }
+ return nullptr;
+}
+
+static void initializeOptionalReference(const Expr *OptionalExpr,
+ LatticeTransferState &State) {
+ if (auto *OptionalVal = cast_or_null<StructValue>(
+ State.Env.getValue(*OptionalExpr, SkipPast::Reference))) {
+ if (OptionalVal->getProperty("has_value") == nullptr) {
+ OptionalVal->setProperty("has_value", State.Env.makeAtomicBoolValue());
+ }
+ }
+}
+
+static void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
+ LatticeTransferState &State) {
+ if (auto *OptionalVal = cast_or_null<StructValue>(
+ State.Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer))) {
+ auto *HasValueVal = getHasValue(OptionalVal);
+ assert(HasValueVal != nullptr);
+
+ if (State.Env.flowConditionImplies(*HasValueVal))
+ return;
+ }
+
+ // Record that this unwrap is *not* provably safe.
+ State.Lattice.getSourceLocations().insert(ObjectExpr->getBeginLoc());
+}
+
+static void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
+ LatticeTransferState &State) {
+ if (auto *OptionalVal = cast_or_null<StructValue>(
+ State.Env.getValue(*CallExpr->getImplicitObjectArgument(),
+ SkipPast::ReferenceThenPointer))) {
+ auto *HasValueVal = getHasValue(OptionalVal);
+ assert(HasValueVal != nullptr);
+
+ auto &CallExprLoc = State.Env.createStorageLocation(*CallExpr);
+ State.Env.setValue(CallExprLoc, *HasValueVal);
+ State.Env.setStorageLocation(*CallExpr, CallExprLoc);
+ }
+}
+
+static auto buildTransferMatchSwitch() {
+ return MatchSwitchBuilder<LatticeTransferState>()
+ // Attach a symbolic "has_value" state to optional values that we see for
+ // the first time.
+ .CaseOf(expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()),
+ initializeOptionalReference)
+
+ // optional::value
+ .CaseOf(
+ isOptionalMemberCallWithName("value"),
+ +[](const CXXMemberCallExpr *E, LatticeTransferState &State) {
+ transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
+ })
+
+ // optional::operator*, optional::operator->
+ .CaseOf(
+ expr(anyOf(isOptionalOperatorCallWithName("*"),
+ isOptionalOperatorCallWithName("->"))),
+ +[](const CallExpr *E, LatticeTransferState &State) {
+ transferUnwrapCall(E, E->getArg(0), State);
+ })
+
+ // optional::has_value
+ .CaseOf(isOptionalMemberCallWithName("has_value"),
+ transferOptionalHasValueCall)
+
+ .Build();
+}
+
+} // namespace
+
+UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx)
+ : DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice>(
+ Ctx),
+ TransferMatchSwitch(buildTransferMatchSwitch()) {}
+
+void UncheckedOptionalAccessModel::transfer(const Stmt *S,
+ SourceLocationsLattice &L,
+ Environment &Env) {
+ LatticeTransferState State(L, Env);
+ TransferMatchSwitch(*S, getASTContext(), State);
+}
+
+} // namespace dataflow
+} // namespace clang
diff --git a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
index d2608503a5396..c2bae4d0e4882 100644
--- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
@@ -16,12 +16,14 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests
TransferTest.cpp
TypeErasedDataflowAnalysisTest.cpp
SolverTest.cpp
+ UncheckedOptionalAccessModelTest.cpp
)
clang_target_link_libraries(ClangAnalysisFlowSensitiveTests
PRIVATE
clangAnalysis
clangAnalysisFlowSensitive
+ clangAnalysisFlowSensitiveModels
clangAST
clangASTMatchers
clangBasic
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
new file mode 100644
index 0000000000000..e3840e1171f39
--- /dev/null
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -0,0 +1,341 @@
+//===- UncheckedOptionalAccessModelTest.cpp -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+// FIXME: Move this to clang/unittests/Analysis/FlowSensitive/Models.
+
+#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
+#include "TestingSupport.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/SourceLocationsLattice.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <string>
+#include <utility>
+#include <vector>
+
+using namespace clang;
+using namespace dataflow;
+using namespace test;
+
+using ::testing::Pair;
+using ::testing::UnorderedElementsAre;
+
+static constexpr char StdTypeTraitsHeader[] = R"(
+namespace std {
+
+template< class T > struct remove_reference {typedef T type;};
+template< class T > struct remove_reference<T&> {typedef T type;};
+template< class T > struct remove_reference<T&&> {typedef T type;};
+
+template <class T>
+ using remove_reference_t = typename remove_reference<T>::type;
+
+} // namespace std
+)";
+
+static constexpr char StdUtilityHeader[] = R"(
+#include "std_type_traits.h"
+
+namespace std {
+
+template <typename T>
+constexpr std::remove_reference_t<T>&& move(T&& x);
+
+} // namespace std
+)";
+
+static constexpr char StdOptionalHeader[] = R"(
+namespace std {
+
+template <typename T>
+class optional {
+ public:
+ constexpr optional() noexcept;
+
+ const T& operator*() const&;
+ T& operator*() &;
+ const T&& operator*() const&&;
+ T&& operator*() &&;
+
+ const T* operator->() const;
+ T* operator->();
+
+ const T& value() const&;
+ T& value() &;
+ const T&& value() const&&;
+ T&& value() &&;
+
+ constexpr bool has_value() const noexcept;
+};
+
+} // namespace std
+)";
+
+static constexpr char AbslOptionalHeader[] = R"(
+namespace absl {
+
+template <typename T>
+class optional {
+ public:
+ constexpr optional() noexcept;
+
+ const T& operator*() const&;
+ T& operator*() &;
+ const T&& operator*() const&&;
+ T&& operator*() &&;
+
+ const T* operator->() const;
+ T* operator->();
+
+ const T& value() const&;
+ T& value() &;
+ const T&& value() const&&;
+ T&& value() &&;
+
+ constexpr bool has_value() const noexcept;
+};
+
+} // namespace absl
+)";
+
+static constexpr char BaseOptionalHeader[] = R"(
+namespace base {
+
+template <typename T>
+class Optional {
+ public:
+ constexpr Optional() noexcept;
+
+ const T& operator*() const&;
+ T& operator*() &;
+ const T&& operator*() const&&;
+ T&& operator*() &&;
+
+ const T* operator->() const;
+ T* operator->();
+
+ const T& value() const&;
+ T& value() &;
+ const T&& value() const&&;
+ T&& value() &&;
+
+ constexpr bool has_value() const noexcept;
+};
+
+} // namespace base
+)";
+
+/// Converts `L` to string.
+static std::string ConvertToString(const SourceLocationsLattice &L,
+ const ASTContext &Ctx) {
+ return L.getSourceLocations().empty() ? "safe"
+ : "unsafe: " + DebugString(L, Ctx);
+}
+
+/// Replaces all occurrences of `Pattern` in `S` with `Replacement`.
+static void ReplaceAllOccurrences(std::string &S, const std::string &Pattern,
+ const std::string &Replacement) {
+ size_t Pos = 0;
+ while (true) {
+ Pos = S.find(Pattern, Pos);
+ if (Pos == std::string::npos)
+ break;
+ S.replace(Pos, Pattern.size(), Replacement);
+ }
+}
+
+struct OptionalTypeIdentifier {
+ std::string NamespaceName;
+ std::string TypeName;
+};
+
+class UncheckedOptionalAccessTest
+ : public ::testing::TestWithParam<OptionalTypeIdentifier> {
+protected:
+ template <typename LatticeChecksMatcher>
+ void ExpectLatticeChecksFor(std::string SourceCode,
+ LatticeChecksMatcher MatchesLatticeChecks) {
+ ExpectLatticeChecksFor(SourceCode, ast_matchers::hasName("target"),
+ MatchesLatticeChecks);
+ }
+
+private:
+ template <typename FuncDeclMatcher, typename LatticeChecksMatcher>
+ void ExpectLatticeChecksFor(std::string SourceCode,
+ FuncDeclMatcher FuncMatcher,
+ LatticeChecksMatcher MatchesLatticeChecks) {
+ ReplaceAllOccurrences(SourceCode, "$ns", GetParam().NamespaceName);
+ ReplaceAllOccurrences(SourceCode, "$optional", GetParam().TypeName);
+
+ std::vector<std::pair<std::string, std::string>> Headers;
+ Headers.emplace_back("std_type_traits.h", StdTypeTraitsHeader);
+ Headers.emplace_back("std_utility.h", StdUtilityHeader);
+ Headers.emplace_back("std_optional.h", StdOptionalHeader);
+ Headers.emplace_back("absl_optional.h", AbslOptionalHeader);
+ Headers.emplace_back("base_optional.h", BaseOptionalHeader);
+ Headers.emplace_back("unchecked_optional_access_test.h", R"(
+ #include "absl_optional.h"
+ #include "base_optional.h"
+ #include "std_optional.h"
+ #include "std_utility.h"
+ )");
+ const tooling::FileContentMappings FileContents(Headers.begin(),
+ Headers.end());
+ llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
+ SourceCode, FuncMatcher,
+ [](ASTContext &Ctx, Environment &) {
+ return UncheckedOptionalAccessModel(Ctx);
+ },
+ [&MatchesLatticeChecks](
+ llvm::ArrayRef<std::pair<
+ std::string, DataflowAnalysisState<SourceLocationsLattice>>>
+ CheckToLatticeMap,
+ ASTContext &Ctx) {
+ // FIXME: Consider using a matcher instead of translating
+ // `CheckToLatticeMap` to `CheckToStringifiedLatticeMap`.
+ std::vector<std::pair<std::string, std::string>>
+ CheckToStringifiedLatticeMap;
+ for (const auto &E : CheckToLatticeMap) {
+ CheckToStringifiedLatticeMap.emplace_back(
+ E.first, ConvertToString(E.second.Lattice, Ctx));
+ }
+ EXPECT_THAT(CheckToStringifiedLatticeMap, MatchesLatticeChecks);
+ },
+ {"-fsyntax-only", "-std=c++17", "-Wno-undefined-inline"}, FileContents);
+ if (Error)
+ FAIL() << llvm::toString(std::move(Error));
+ }
+};
+
+INSTANTIATE_TEST_SUITE_P(
+ UncheckedOptionalUseTestInst, UncheckedOptionalAccessTest,
+ ::testing::Values(OptionalTypeIdentifier{"std", "optional"},
+ OptionalTypeIdentifier{"absl", "optional"},
+ OptionalTypeIdentifier{"base", "Optional"}),
+ [](const ::testing::TestParamInfo<OptionalTypeIdentifier> &Info) {
+ return Info.param.NamespaceName;
+ });
+
+TEST_P(UncheckedOptionalAccessTest, EmptyFunctionBody) {
+ ExpectLatticeChecksFor(R"(
+ void target() {
+ (void)0;
+ /*[[check]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("check", "safe")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, UnwrapUsingValueNoCheck) {
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ void target($ns::$optional<int> opt) {
+ opt.value();
+ /*[[check]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("check", "unsafe: input.cc:5:7")));
+
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ void target($ns::$optional<int> opt) {
+ std::move(opt).value();
+ /*[[check]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("check", "unsafe: input.cc:5:7")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, UnwrapUsingOperatorStarNoCheck) {
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ void target($ns::$optional<int> opt) {
+ *opt;
+ /*[[check]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("check", "unsafe: input.cc:5:8")));
+
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ void target($ns::$optional<int> opt) {
+ *std::move(opt);
+ /*[[check]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("check", "unsafe: input.cc:5:8")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, UnwrapUsingOperatorArrowNoCheck) {
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Foo {
+ void foo();
+ };
+
+ void target($ns::$optional<Foo> opt) {
+ opt->foo();
+ /*[[check]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("check", "unsafe: input.cc:9:7")));
+
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Foo {
+ void foo();
+ };
+
+ void target($ns::$optional<Foo> opt) {
+ std::move(opt)->foo();
+ /*[[check]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("check", "unsafe: input.cc:9:7")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, UnwrapWithCheck) {
+ ExpectLatticeChecksFor(R"(
+ #include "unchecked_optional_access_test.h"
+
+ void target($ns::$optional<int> opt) {
+ if (opt.has_value()) {
+ opt.value();
+ /*[[check]]*/
+ }
+ }
+ )",
+ UnorderedElementsAre(Pair("check", "safe")));
+}
+
+// FIXME: Add support for:
+// - constructors (default, copy, move, non-standard)
+// - assignment operators (default, copy, move, non-standard)
+// - operator bool
+// - emplace
+// - reset
+// - value_or
+// - swap
+// - make_optional
+// - invalidation (passing optional by non-const reference/pointer)
More information about the cfe-commits
mailing list