diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h index 24f8b0b99870a..2d54bd3c6f7ad 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h @@ -13,6 +13,7 @@ #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Analysis/CFG.h" #include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h" +#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h" #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h" #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" #include "clang/Analysis/FlowSensitive/MatchSwitch.h" @@ -69,7 +70,8 @@ struct UncheckedStatusOrAccessModelOptions {}; // Dataflow analysis that discovers unsafe uses of StatusOr values. class UncheckedStatusOrAccessModel - : public DataflowAnalysis { + : public DataflowAnalysis> { public: explicit UncheckedStatusOrAccessModel(ASTContext &Ctx, Environment &Env); diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp index 5869aa9a3e227..0bb8da57edece 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp @@ -237,6 +237,49 @@ static auto isAsStatusCallWithStatusOr() { hasArgument(0, hasType(statusOrType()))); } +static auto possiblyReferencedStatusOrType() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return anyOf(statusOrType(), referenceType(pointee(statusOrType()))); +} + +static auto isConstStatusOrAccessorMemberCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxMemberCallExpr(callee( + cxxMethodDecl(parameterCountIs(0), isConst(), + returns(qualType(possiblyReferencedStatusOrType()))))); +} + +static auto isConstStatusOrAccessorMemberOperatorCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxOperatorCallExpr( + callee(cxxMethodDecl(parameterCountIs(0), isConst(), + returns(possiblyReferencedStatusOrType())))); +} + +static auto isConstStatusOrPointerAccessorMemberCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxMemberCallExpr(callee(cxxMethodDecl( + parameterCountIs(0), isConst(), + returns(pointerType(pointee(possiblyReferencedStatusOrType())))))); +} + +static auto isConstStatusOrPointerAccessorMemberOperatorCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxOperatorCallExpr(callee(cxxMethodDecl( + parameterCountIs(0), isConst(), + returns(pointerType(pointee(possiblyReferencedStatusOrType())))))); +} + +static auto isNonConstMemberCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst())))); +} + +static auto isNonConstMemberOperatorCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxOperatorCallExpr(callee(cxxMethodDecl(unless(isConst())))); +} + static auto buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) { return CFGMatchSwitchBuilderisPRValue() ? &State.Env.getResultObjectLocation(*Expr) + : State.Env.get(*Expr); + if (StatusOrLoc != nullptr && + State.Env.getValue(locForOk(locForStatus(*StatusOrLoc))) == nullptr) + initializeStatusOr(*StatusOrLoc, State.Env); +} + +static bool doHandleConstStatusOrAccessorMemberCall( + const CallExpr *Expr, RecordStorageLocation *RecordLoc, + const MatchFinder::MatchResult &Result, LatticeTransferState &State) { + assert(isStatusOrType(Expr->getType())); + if (RecordLoc == nullptr) + return false; + const FunctionDecl *DirectCallee = Expr->getDirectCallee(); + if (DirectCallee == nullptr) + return false; + StorageLocation &Loc = + State.Lattice.getOrCreateConstMethodReturnStorageLocation( + *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) { + initializeStatusOr(cast(Loc), State.Env); + }); + if (Expr->isPRValue()) { + auto &ResultLoc = State.Env.getResultObjectLocation(*Expr); + copyRecord(cast(Loc), ResultLoc, State.Env); + } else { + State.Env.setStorageLocation(*Expr, Loc); + } + return true; +} + +static void handleConstStatusOrAccessorMemberCall( + const CallExpr *Expr, RecordStorageLocation *RecordLoc, + const MatchFinder::MatchResult &Result, LatticeTransferState &State) { + if (!doHandleConstStatusOrAccessorMemberCall(Expr, RecordLoc, Result, State)) + transferStatusOrReturningCall(Expr, State); +} +static void handleConstStatusOrPointerAccessorMemberCall( + const CallExpr *Expr, RecordStorageLocation *RecordLoc, + const MatchFinder::MatchResult &Result, LatticeTransferState &State) { + if (RecordLoc == nullptr) + return; + auto *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, Expr, + State.Env); + State.Env.setValue(*Expr, *Val); +} + +static void +transferConstStatusOrAccessorMemberCall(const CXXMemberCallExpr *Expr, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + handleConstStatusOrAccessorMemberCall( + Expr, getImplicitObjectLocation(*Expr, State.Env), Result, State); +} + +static void transferConstStatusOrAccessorMemberOperatorCall( + const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + auto *RecordLoc = cast_or_null( + State.Env.getStorageLocation(*Expr->getArg(0))); + handleConstStatusOrAccessorMemberCall(Expr, RecordLoc, Result, State); +} + +static void transferConstStatusOrPointerAccessorMemberCall( + const CXXMemberCallExpr *Expr, const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + handleConstStatusOrPointerAccessorMemberCall( + Expr, getImplicitObjectLocation(*Expr, State.Env), Result, State); +} + +static void transferConstStatusOrPointerAccessorMemberOperatorCall( + const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + auto *RecordLoc = cast_or_null( + State.Env.getStorageLocation(*Expr->getArg(0))); + handleConstStatusOrPointerAccessorMemberCall(Expr, RecordLoc, Result, State); +} + +static void handleNonConstMemberCall(const CallExpr *Expr, + RecordStorageLocation *RecordLoc, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + if (RecordLoc) { + State.Lattice.clearConstMethodReturnValues(*RecordLoc); + State.Lattice.clearConstMethodReturnStorageLocations(*RecordLoc); + } + if (isStatusOrType(Expr->getType())) + transferStatusOrReturningCall(Expr, State); +} + +static void transferNonConstMemberCall(const CXXMemberCallExpr *Expr, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + handleNonConstMemberCall(Expr, getImplicitObjectLocation(*Expr, State.Env), + Result, State); +} + +static void +transferNonConstMemberOperatorCall(const CXXOperatorCallExpr *Expr, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + auto *RecordLoc = cast_or_null( + State.Env.getStorageLocation(*Expr->getArg(0))); + handleNonConstMemberCall(Expr, RecordLoc, Result, State); +} + CFGMatchSwitch buildTransferMatchSwitch(ASTContext &Ctx, CFGMatchSwitchBuilder Builder) { @@ -755,6 +906,23 @@ buildTransferMatchSwitch(ASTContext &Ctx, transferLoggingGetReferenceableValueCall) .CaseOfCFGStmt(isLoggingCheckEqImpl(), transferLoggingCheckEqImpl) + // const accessor calls + .CaseOfCFGStmt(isConstStatusOrAccessorMemberCall(), + transferConstStatusOrAccessorMemberCall) + .CaseOfCFGStmt( + isConstStatusOrAccessorMemberOperatorCall(), + transferConstStatusOrAccessorMemberOperatorCall) + .CaseOfCFGStmt( + isConstStatusOrPointerAccessorMemberCall(), + transferConstStatusOrPointerAccessorMemberCall) + .CaseOfCFGStmt( + isConstStatusOrPointerAccessorMemberOperatorCall(), + transferConstStatusOrPointerAccessorMemberOperatorCall) + // non-const member calls that may modify the state of an object. + .CaseOfCFGStmt(isNonConstMemberCall(), + transferNonConstMemberCall) + .CaseOfCFGStmt(isNonConstMemberOperatorCall(), + transferNonConstMemberOperatorCall) // N.B. These need to come after all other CXXConstructExpr. // These are there to make sure that every Status and StatusOr object // have their ok boolean initialized when constructed. If we were to diff --git a/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp b/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp index 2e528edd7c1f9..76c7310e16a4f 100644 --- a/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp +++ b/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp @@ -2232,6 +2232,95 @@ using testing::AssertionResult; #endif // TESTING_DEFS_H )cc"; +constexpr const char StdUniquePtrHeader[] = R"cc( +namespace std { + + template + struct default_delete {}; + + template > + class unique_ptr { + public: + using element_type = T; + using deleter_type = D; + + constexpr unique_ptr(); + constexpr unique_ptr(nullptr_t) noexcept; + unique_ptr(unique_ptr&&); + explicit unique_ptr(T*); + template + unique_ptr(unique_ptr&&); + + ~unique_ptr(); + + unique_ptr& operator=(unique_ptr&&); + template + unique_ptr& operator=(unique_ptr&&); + unique_ptr& operator=(nullptr_t); + + void reset(T* = nullptr) noexcept; + T* release(); + T* get() const; + + T& operator*() const; + T* operator->() const; + explicit operator bool() const noexcept; + }; + + template + class unique_ptr { + public: + T* get() const; + T& operator[](size_t i); + const T& operator[](size_t i) const; + }; + + template + unique_ptr make_unique(Args&&...); + + template + void swap(unique_ptr& x, unique_ptr& y) noexcept; + + template + bool operator==(const unique_ptr& x, const unique_ptr& y); + template + bool operator!=(const unique_ptr& x, const unique_ptr& y); + template + bool operator<(const unique_ptr& x, const unique_ptr& y); + template + bool operator<=(const unique_ptr& x, const unique_ptr& y); + template + bool operator>(const unique_ptr& x, const unique_ptr& y); + template + bool operator>=(const unique_ptr& x, const unique_ptr& y); + + template + bool operator==(const unique_ptr& x, nullptr_t) noexcept; + template + bool operator==(nullptr_t, const unique_ptr& y) noexcept; + template + bool operator!=(const unique_ptr& x, nullptr_t) noexcept; + template + bool operator!=(nullptr_t, const unique_ptr& y) noexcept; + template + bool operator<(const unique_ptr& x, nullptr_t); + template + bool operator<(nullptr_t, const unique_ptr& y); + template + bool operator<=(const unique_ptr& x, nullptr_t); + template + bool operator<=(nullptr_t, const unique_ptr& y); + template + bool operator>(const unique_ptr& x, nullptr_t); + template + bool operator>(nullptr_t, const unique_ptr& y); + template + bool operator>=(const unique_ptr& x, nullptr_t); + template + bool operator>=(nullptr_t, const unique_ptr& y); +} +)cc"; + std::vector> getMockHeaders() { std::vector> Headers; Headers.emplace_back("cstddef.h", CStdDefHeader); @@ -2249,6 +2338,7 @@ std::vector> getMockHeaders() { Headers.emplace_back("statusor_defs.h", StatusOrDefsHeader); Headers.emplace_back("absl_log.h", AbslLogHeader); Headers.emplace_back("testing_defs.h", TestingDefsHeader); + Headers.emplace_back("std_unique_ptr.h", StdUniquePtrHeader); return Headers; } diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp index 13cde05df0a3f..e13a341f4d006 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp @@ -3270,6 +3270,179 @@ TEST_P(UncheckedStatusOrAccessModelTest, ConstructStatusFromReference) { )cc"); } +TEST_P(UncheckedStatusOrAccessModelTest, AccessorCall) { + // Accessor returns reference. + ExpectDiagnosticsFor( + R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + const STATUSOR_INT& sor() const { return sor_; } + }; + + void target(Foo foo) { + if (foo.sor().ok()) foo.sor().value(); + } + )cc"); + + // Uses an operator + ExpectDiagnosticsFor( + R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + const STATUSOR_INT& operator()() const { return sor_; } + }; + + void target(Foo foo) { + if (foo().ok()) foo().value(); + } + )cc"); + + // Calls nonconst method in between. + ExpectDiagnosticsFor( + R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + void invalidate() {} + + const STATUSOR_INT& sor() const { return sor_; } + }; + + void target(Foo foo) { + if (foo.sor().ok()) { + foo.invalidate(); + foo.sor().value(); // [[unsafe]] + } + } + )cc"); + + // Calls nonconst operator in between. + ExpectDiagnosticsFor( + R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + void operator()() {} + + const STATUSOR_INT& sor() const { return sor_; } + }; + + void target(Foo foo) { + if (foo.sor().ok()) { + foo(); + foo.sor().value(); // [[unsafe]] + } + } + )cc"); + + // Accessor returns copy. + ExpectDiagnosticsFor( + R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + STATUSOR_INT sor() const { return sor_; } + }; + + void target(Foo foo) { + if (foo.sor().ok()) foo.sor().value(); + } + )cc"); + + // Non-const accessor. + ExpectDiagnosticsFor( + R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + const STATUSOR_INT& sor() { return sor_; } + }; + + void target(Foo foo) { + if (foo.sor().ok()) foo.sor().value(); // [[unsafe]] + } + )cc"); + + // Non-const rvalue accessor. + ExpectDiagnosticsFor( + R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + STATUSOR_INT&& sor() { return std::move(sor_); } + }; + + void target(Foo foo) { + if (foo.sor().ok()) foo.sor().value(); // [[unsafe]] + } + )cc"); + + // const pointer accessor. + ExpectDiagnosticsFor( + R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + const STATUSOR_INT* sor() const { return &sor_; } + }; + + void target(Foo foo) { + if (foo.sor()->ok()) foo.sor()->value(); + } + )cc"); + + // const pointer operator. + ExpectDiagnosticsFor( + R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + const STATUSOR_INT* operator->() const { return &sor_; } + }; + + void target(Foo foo) { + if (foo->ok()) foo->value(); + } + )cc"); + + // We copy the result of the accessor. + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + struct Foo { + STATUSOR_INT sor_; + + const STATUSOR_INT& sor() const { return sor_; } + }; + void target() { + Foo foo; + if (!foo.sor().ok()) return; + const auto sor = foo.sor(); + sor.value(); + } + )cc"); +} + } // namespace std::string