diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp index 75b0959491c22..6e7695fc7ce39 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp @@ -24,6 +24,7 @@ #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h" #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" #include "clang/Analysis/FlowSensitive/MatchSwitch.h" +#include "clang/Analysis/FlowSensitive/RecordOps.h" #include "clang/Analysis/FlowSensitive/StorageLocation.h" #include "clang/Analysis/FlowSensitive/Value.h" #include "clang/Basic/LLVM.h" @@ -95,6 +96,18 @@ static QualType getStatusOrValueType(ClassTemplateSpecializationDecl *TRD) { return TRD->getTemplateArgs().get(0).getAsType(); } +static auto ofClassStatus() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return ofClass(hasName("::absl::Status")); +} + +static auto isStatusMemberCallWithName(llvm::StringRef member_name) { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxMemberCallExpr( + on(expr(unless(cxxThisExpr()))), + callee(cxxMethodDecl(hasName(member_name), ofClassStatus()))); +} + static auto isStatusOrMemberCallWithName(llvm::StringRef member_name) { using namespace ::clang::ast_matchers; // NOLINT: Too many names return cxxMemberCallExpr( @@ -249,6 +262,61 @@ static void transferStatusOrOkCall(const CXXMemberCallExpr *Expr, State.Env.setValue(*Expr, OkVal); } +static void transferStatusCall(const CXXMemberCallExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + RecordStorageLocation *StatusOrLoc = + getImplicitObjectLocation(*Expr, State.Env); + if (StatusOrLoc == nullptr) + return; + + RecordStorageLocation &StatusLoc = locForStatus(*StatusOrLoc); + + if (State.Env.getValue(locForOk(StatusLoc)) == nullptr) + initializeStatusOr(*StatusOrLoc, State.Env); + + if (Expr->isPRValue()) + copyRecord(StatusLoc, State.Env.getResultObjectLocation(*Expr), State.Env); + else + State.Env.setStorageLocation(*Expr, StatusLoc); +} + +static void transferStatusOkCall(const CXXMemberCallExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + RecordStorageLocation *StatusLoc = + getImplicitObjectLocation(*Expr, State.Env); + if (StatusLoc == nullptr) + return; + + if (Value *Val = State.Env.getValue(locForOk(*StatusLoc))) + State.Env.setValue(*Expr, *Val); +} + +static void transferStatusUpdateCall(const CXXMemberCallExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + // S.Update(OtherS) sets S to the error code of OtherS if it is OK, + // otherwise does nothing. + assert(Expr->getNumArgs() == 1); + auto *Arg = Expr->getArg(0); + RecordStorageLocation *ArgRecord = + Arg->isPRValue() ? &State.Env.getResultObjectLocation(*Arg) + : State.Env.get(*Arg); + RecordStorageLocation *ThisLoc = getImplicitObjectLocation(*Expr, State.Env); + if (ThisLoc == nullptr || ArgRecord == nullptr) + return; + + auto &ThisOkVal = valForOk(*ThisLoc, State.Env); + auto &ArgOkVal = valForOk(*ArgRecord, State.Env); + auto &A = State.Env.arena(); + auto &NewVal = State.Env.makeAtomicBoolValue(); + State.Env.assume(A.makeImplies(A.makeNot(ThisOkVal.formula()), + A.makeNot(NewVal.formula()))); + State.Env.assume(A.makeImplies(NewVal.formula(), ArgOkVal.formula())); + State.Env.setValue(locForOk(*ThisLoc), NewVal); +} + CFGMatchSwitch buildTransferMatchSwitch(ASTContext &Ctx, CFGMatchSwitchBuilder Builder) { @@ -256,6 +324,12 @@ buildTransferMatchSwitch(ASTContext &Ctx, return std::move(Builder) .CaseOfCFGStmt(isStatusOrMemberCallWithName("ok"), transferStatusOrOkCall) + .CaseOfCFGStmt(isStatusOrMemberCallWithName("status"), + transferStatusCall) + .CaseOfCFGStmt(isStatusMemberCallWithName("ok"), + transferStatusOkCall) + .CaseOfCFGStmt(isStatusMemberCallWithName("Update"), + transferStatusUpdateCall) .Build(); } diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp index 4827cc1d0a7e9..cae926570ffe4 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp @@ -2453,6 +2453,167 @@ TEST_P(UncheckedStatusOrAccessModelTest, SubclassOperator) { )cc"); } +TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusCheck) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + if (sor.status().ok()) + sor.value(); + else + sor.value(); // [[unsafe]] + } + )cc"); +} + +TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusRefCheck) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + const STATUS& s = sor.status(); + if (s.ok()) + sor.value(); + else + sor.value(); // [[unsafe]] + } + )cc"); +} + +TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusPtrCheck) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + const STATUS* s = &sor.status(); + if (s->ok()) + sor.value(); + else + sor.value(); // [[unsafe]] + } + )cc"); +} + +TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithMovedStatus) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + if (std::move(sor.status()).ok()) + sor.value(); + else + sor.value(); // [[unsafe]] + } + )cc"); +} + +TEST_P(UncheckedStatusOrAccessModelTest, MembersUsedInsideStatus) { + ExpectDiagnosticsFor(R"cc( + namespace absl { + + class Status { + public: + bool ok() const; + + void target() const { ok(); } + }; + + } // namespace absl + )cc"); +} + +TEST_P(UncheckedStatusOrAccessModelTest, StatusUpdate) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + STATUS s; + s.Update(sor.status()); + if (s.ok()) + sor.value(); + else + sor.value(); // [[unsafe]] + } + )cc"); + + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor1, STATUSOR_INT sor2) { + STATUS s; + s.Update(sor1.status()); + s.Update(sor2.status()); + if (s.ok()) { + sor1.value(); + sor2.value(); + } + } + )cc"); + + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor1, STATUSOR_INT sor2) { + STATUS s; + s.Update(sor1.status()); + CHECK(s.ok()); + s.Update(sor2.status()); + sor1.value(); + sor2.value(); // [[unsafe]] + } + )cc"); + + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor1, STATUSOR_INT sor2) { + STATUS s; + s.Update(sor1.status()); + CHECK(s.ok()); + sor1.value(); + sor2.value(); // [[unsafe]] + } + )cc"); + + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor1, STATUSOR_INT sor2) { + STATUS s; + STATUS sor1_status = sor1.status(); + s.Update(std::move(sor1_status)); + CHECK(s.ok()); + sor1.value(); + sor2.value(); // [[unsafe]] + } + )cc"); + + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor1, STATUSOR_INT sor2) { + STATUS s; + STATUS sor1_status = sor1.status(); + sor1_status.Update(sor2.status()); + s.Update(std::move(sor1_status)); + CHECK(s.ok()); + sor1.value(); + sor2.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + const STATUS& OptStatus(); + + void target(STATUSOR_INT sor) { + auto s = sor.status(); + s.Update(OptStatus()); + if (s.ok()) sor.value(); + } + )cc"); +} + } // namespace std::string