[clang] [clang][dataflow] Cache accessors returning pointers in bugprone-unchecked-optional-access (PR #113922)
Jan Voung via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 28 09:01:47 PDT 2024
https://github.com/jvoung updated https://github.com/llvm/llvm-project/pull/113922
>From 21f15146b8a7941781b6d728cdbb0d0be50b02fc Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 28 Oct 2024 14:45:39 +0000
Subject: [PATCH 1/2] [clang][dataflow] Cache accessors returning pointers in
bugprone-unchecked-optional-access
Previously, we covered returning refs, or copies of optional, and bools.
Now cover returning pointers (to any type).
This is useful for cases like operator-> of smart pointers.
Addresses more of issue llvm#58510
---
.../Models/UncheckedOptionalAccessModel.h | 8 +++
.../Models/UncheckedOptionalAccessModel.cpp | 20 +++++-
.../UncheckedOptionalAccessModelTest.cpp | 72 ++++++++++++++++---
3 files changed, 87 insertions(+), 13 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 9d81cacb507351..713494178b97bd 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -37,6 +37,14 @@ struct UncheckedOptionalAccessModelOptions {
/// can't identify when their results are used safely (across calls),
/// resulting in false positives in all such cases. Note: this option does not
/// cover access through `operator[]`.
+ /// FIXME: we currently cache and equate the result of const accessors
+ /// returning pointers, so cover the case of operator-> followed by
+ /// operator->, which covers the common case of smart pointers. We also cover
+ /// some limited cases of returning references (if return type is an optional
+ /// type), so cover some cases of operator* followed by operator*. We don't
+ /// cover mixing operator-> and operator*. Once we are confident in this const
+ /// accessor caching, we shouldn't need the IgnoreSmartPointerDereference
+ /// option anymore.
bool IgnoreSmartPointerDereference = false;
};
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 31ae2b94f5b617..da5dda063344f9 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -338,6 +338,11 @@ auto isZeroParamConstMemberCall() {
callee(cxxMethodDecl(parameterCountIs(0), isConst())));
}
+auto isZeroParamConstMemberOperatorCall() {
+ return cxxOperatorCallExpr(
+ callee(cxxMethodDecl(parameterCountIs(0), isConst())));
+}
+
auto isNonConstMemberCall() {
return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
}
@@ -572,9 +577,10 @@ void handleConstMemberCall(const CallExpr *CE,
return;
}
- // Cache if the const method returns a boolean type.
+ // Cache if the const method returns a boolean or pointer type.
// We may decide to cache other return types in the future.
- if (RecordLoc != nullptr && CE->getType()->isBooleanType()) {
+ if (RecordLoc != nullptr &&
+ (CE->getType()->isBooleanType() || CE->getType()->isPointerType())) {
Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, CE,
State.Env);
if (Val == nullptr)
@@ -597,6 +603,14 @@ void transferValue_ConstMemberCall(const CXXMemberCallExpr *MCE,
MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
}
+void transferValue_ConstMemberOperatorCall(
+ const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
+ State.Env.getStorageLocation(*OCE->getArg(0)));
+ handleConstMemberCall(OCE, RecordLoc, Result, State);
+}
+
void handleNonConstMemberCall(const CallExpr *CE,
dataflow::RecordStorageLocation *RecordLoc,
const MatchFinder::MatchResult &Result,
@@ -1020,6 +1034,8 @@ auto buildTransferMatchSwitch() {
// const accessor calls
.CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
transferValue_ConstMemberCall)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(isZeroParamConstMemberOperatorCall(),
+ transferValue_ConstMemberOperatorCall)
// non-const member calls that may modify the state of an object.
.CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
transferValue_NonConstMemberCall)
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 5b64eaca0e10d3..5890466488c3c3 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1282,28 +1282,35 @@ static raw_ostream &operator<<(raw_ostream &OS,
class UncheckedOptionalAccessTest
: public ::testing::TestWithParam<OptionalTypeIdentifier> {
protected:
- void ExpectDiagnosticsFor(std::string SourceCode) {
- ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target"));
+ void ExpectDiagnosticsFor(std::string SourceCode,
+ bool IgnoreSmartPointerDereference = true) {
+ ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target"),
+ IgnoreSmartPointerDereference);
}
- void ExpectDiagnosticsForLambda(std::string SourceCode) {
+ void ExpectDiagnosticsForLambda(std::string SourceCode,
+ bool IgnoreSmartPointerDereference = true) {
ExpectDiagnosticsFor(
- SourceCode, ast_matchers::hasDeclContext(
- ast_matchers::cxxRecordDecl(ast_matchers::isLambda())));
+ SourceCode,
+ ast_matchers::hasDeclContext(
+ ast_matchers::cxxRecordDecl(ast_matchers::isLambda())),
+ IgnoreSmartPointerDereference);
}
template <typename FuncDeclMatcher>
- void ExpectDiagnosticsFor(std::string SourceCode,
- FuncDeclMatcher FuncMatcher) {
+ void ExpectDiagnosticsFor(std::string SourceCode, FuncDeclMatcher FuncMatcher,
+ bool IgnoreSmartPointerDereference = true) {
// Run in C++17 and C++20 mode to cover differences in the AST between modes
// (e.g. C++20 can contain `CXXRewrittenBinaryOperator`).
for (const char *CxxMode : {"-std=c++17", "-std=c++20"})
- ExpectDiagnosticsFor(SourceCode, FuncMatcher, CxxMode);
+ ExpectDiagnosticsFor(SourceCode, FuncMatcher, CxxMode,
+ IgnoreSmartPointerDereference);
}
template <typename FuncDeclMatcher>
void ExpectDiagnosticsFor(std::string SourceCode, FuncDeclMatcher FuncMatcher,
- const char *CxxMode) {
+ const char *CxxMode,
+ bool IgnoreSmartPointerDereference) {
ReplaceAllOccurrences(SourceCode, "$ns", GetParam().NamespaceName);
ReplaceAllOccurrences(SourceCode, "$optional", GetParam().TypeName);
@@ -1328,8 +1335,7 @@ class UncheckedOptionalAccessTest
template <typename T>
T Make();
)");
- UncheckedOptionalAccessModelOptions Options{
- /*IgnoreSmartPointerDereference=*/true};
+ UncheckedOptionalAccessModelOptions Options{IgnoreSmartPointerDereference};
std::vector<SourceLocation> Diagnostics;
llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
AnalysisInputs<UncheckedOptionalAccessModel>(
@@ -3721,6 +3727,50 @@ TEST_P(UncheckedOptionalAccessTest, ConstByValueAccessorWithModInBetween) {
)cc");
}
+TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessor) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct B {
+ $ns::$optional<int> x;
+ };
+
+ struct MyUniquePtr {
+ B* operator->() const;
+ };
+
+ void target(MyUniquePtr a) {
+ if (a->x) {
+ *a->x;
+ }
+ }
+ )cc",
+ /*IgnoreSmartPointerDereference=*/false);
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessorWithModInBetween) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct B {
+ $ns::$optional<int> x;
+ };
+
+ struct MyUniquePtr {
+ B* operator->() const;
+ void reset(B*);
+ };
+
+ void target(MyUniquePtr a) {
+ if (a->x) {
+ a.reset(nullptr);
+ *a->x; // [[unsafe]]
+ }
+ }
+ )cc",
+ /*IgnoreSmartPointerDereference=*/false);
+}
+
TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessor) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
>From e2de6a1e1935368ee52659f8c820220df8fbc4e7 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 28 Oct 2024 16:01:09 +0000
Subject: [PATCH 2/2] Small cleanup in tests
B -> A (why start naming at B?)
a -> p (for pointer)
---
.../UncheckedOptionalAccessModelTest.cpp | 24 +++++++++----------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 5890466488c3c3..de16f6be8eedbc 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3731,17 +3731,17 @@ TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessor) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
- struct B {
+ struct A {
$ns::$optional<int> x;
};
struct MyUniquePtr {
- B* operator->() const;
+ A* operator->() const;
};
- void target(MyUniquePtr a) {
- if (a->x) {
- *a->x;
+ void target(MyUniquePtr p) {
+ if (p->x) {
+ *p->x;
}
}
)cc",
@@ -3752,19 +3752,19 @@ TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessorWithModInBetween) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
- struct B {
+ struct A {
$ns::$optional<int> x;
};
struct MyUniquePtr {
- B* operator->() const;
- void reset(B*);
+ A* operator->() const;
+ void reset(A*);
};
- void target(MyUniquePtr a) {
- if (a->x) {
- a.reset(nullptr);
- *a->x; // [[unsafe]]
+ void target(MyUniquePtr p) {
+ if (p->x) {
+ p.reset(nullptr);
+ *p->x; // [[unsafe]]
}
}
)cc",
More information about the cfe-commits
mailing list