https://github.com/fmayer created https://github.com/llvm/llvm-project/pull/170935
This is not necessarily correct, but prevents us from flagging lots of false positives because code usually abides by this. >From 651e3bf72e1680a69ee138fcd07e50f1395d8307 Mon Sep 17 00:00:00 2001 From: Florian Mayer <[email protected]> Date: Fri, 5 Dec 2025 14:17:56 -0800 Subject: [PATCH] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20initia?= =?UTF-8?q?l=20version?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created using spr 1.3.7 --- .../Models/UncheckedStatusOrAccessModel.h | 4 +- .../Models/UncheckedStatusOrAccessModel.cpp | 161 ++++++++++++++++ ...ncheckedStatusOrAccessModelTestFixture.cpp | 173 ++++++++++++++++++ 3 files changed, 337 insertions(+), 1 deletion(-) 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<UncheckedStatusOrAccessModel, NoopLattice> { + : public DataflowAnalysis<UncheckedStatusOrAccessModel, + CachedConstAccessorsLattice<NoopLattice>> { 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..038f2b0338c8d 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 CFGMatchSwitchBuilder<const Environment, @@ -697,6 +740,107 @@ static void transferPointerToBoolean(const ImplicitCastExpr *Expr, dyn_cast_or_null<BoolValue>(State.Env.getValue(*Expr->getSubExpr()))) State.Env.setValue(*Expr, *SubExprVal); } +static void handleConstStatusOrAccessorMemberCall( + const CallExpr *Expr, RecordStorageLocation *RecordLoc, + const MatchFinder::MatchResult &Result, LatticeTransferState &State) { + assert(isStatusOrType(Expr->getType())); + if (RecordLoc == nullptr) + return; + const FunctionDecl *DirectCallee = Expr->getDirectCallee(); + if (DirectCallee == nullptr) + return; + StorageLocation &Loc = + State.Lattice.getOrCreateConstMethodReturnStorageLocation( + *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) { + initializeStatusOr(cast<RecordStorageLocation>(Loc), State.Env); + }); + if (Expr->isPRValue()) { + auto &ResultLoc = State.Env.getResultObjectLocation(*Expr); + copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env); + } else { + State.Env.setStorageLocation(*Expr, Loc); + } +} + +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<RecordStorageLocation>( + 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<RecordStorageLocation>( + State.Env.getStorageLocation(*Expr->getArg(0))); + handleConstStatusOrPointerAccessorMemberCall(Expr, RecordLoc, Result, State); +} + +static void transferStatusOrReturningCall(const CallExpr *Expr, + LatticeTransferState &State) { + RecordStorageLocation *StatusOrLoc = + Expr->isPRValue() ? &State.Env.getResultObjectLocation(*Expr) + : State.Env.get<RecordStorageLocation>(*Expr); + if (StatusOrLoc != nullptr && + State.Env.getValue(locForOk(locForStatus(*StatusOrLoc))) == nullptr) + initializeStatusOr(*StatusOrLoc, State.Env); +} + +static void handleNonConstMemberCall(const CallExpr *Expr, + RecordStorageLocation *RecordLoc, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + if (RecordLoc == nullptr) + return; + 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<RecordStorageLocation>( + State.Env.getStorageLocation(*Expr->getArg(0))); + handleNonConstMemberCall(Expr, RecordLoc, Result, State); +} CFGMatchSwitch<LatticeTransferState> buildTransferMatchSwitch(ASTContext &Ctx, @@ -755,6 +899,23 @@ buildTransferMatchSwitch(ASTContext &Ctx, transferLoggingGetReferenceableValueCall) .CaseOfCFGStmt<CallExpr>(isLoggingCheckEqImpl(), transferLoggingCheckEqImpl) + // const accessor calls + .CaseOfCFGStmt<CXXMemberCallExpr>(isConstStatusOrAccessorMemberCall(), + transferConstStatusOrAccessorMemberCall) + .CaseOfCFGStmt<CXXOperatorCallExpr>( + isConstStatusOrAccessorMemberOperatorCall(), + transferConstStatusOrAccessorMemberOperatorCall) + .CaseOfCFGStmt<CXXMemberCallExpr>( + isConstStatusOrPointerAccessorMemberCall(), + transferConstStatusOrPointerAccessorMemberCall) + .CaseOfCFGStmt<CXXOperatorCallExpr>( + isConstStatusOrPointerAccessorMemberOperatorCall(), + transferConstStatusOrPointerAccessorMemberOperatorCall) + // non-const member calls that may modify the state of an object. + .CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(), + transferNonConstMemberCall) + .CaseOfCFGStmt<CXXOperatorCallExpr>(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/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp index 13cde05df0a3f..e075818f8a2c1 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 inbetween. + 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 inbetween. + 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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
