-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang][dataflow] Cache accessors returning pointers in bugprone-unchecked-optional-access #113922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ecked-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
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-analysis Author: Jan Voung (jvoung) ChangesPreviously, we covered returning refs, or copies of optional, and bools. Full diff: https://github.com/llvm/llvm-project/pull/113922.diff 3 Files Affected:
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"
|
ymand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
B -> A (why start naming at B?) a -> p (for pointer)
…ecked-optional-access (llvm#113922) 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
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 #58510