Skip to content

Commit eabfddd

Browse files
fmayerHoney Goyal
fmayer
authored and
Honey Goyal
committed
[FlowSensitive] [StatusOr] [11/N] Assume const accessor calls are stable (llvm#170935)
This is not necessarily correct, but prevents us from flagging lots of false positives because code usually abides by this.
1 parent efc55b9 commit eabfddd

File tree

3 files changed

+344
-1
lines changed

3 files changed

+344
-1
lines changed

‎clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "clang/ASTMatchers/ASTMatchers.h"
1414
#include "clang/Analysis/CFG.h"
1515
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
16+
#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
1617
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
1718
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
1819
#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
@@ -69,7 +70,8 @@ struct UncheckedStatusOrAccessModelOptions {};
6970

7071
// Dataflow analysis that discovers unsafe uses of StatusOr values.
7172
class UncheckedStatusOrAccessModel
72-
: public DataflowAnalysis<UncheckedStatusOrAccessModel, NoopLattice> {
73+
: public DataflowAnalysis<UncheckedStatusOrAccessModel,
74+
CachedConstAccessorsLattice<NoopLattice>> {
7375
public:
7476
explicit UncheckedStatusOrAccessModel(ASTContext &Ctx, Environment &Env);
7577

‎clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp‎

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,49 @@ static auto isAsStatusCallWithStatusOr() {
237237
hasArgument(0, hasType(statusOrType())));
238238
}
239239

240+
static auto possiblyReferencedStatusOrType() {
241+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
242+
return anyOf(statusOrType(), referenceType(pointee(statusOrType())));
243+
}
244+
245+
static auto isConstStatusOrAccessorMemberCall() {
246+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
247+
return cxxMemberCallExpr(callee(
248+
cxxMethodDecl(parameterCountIs(0), isConst(),
249+
returns(qualType(possiblyReferencedStatusOrType())))));
250+
}
251+
252+
static auto isConstStatusOrAccessorMemberOperatorCall() {
253+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
254+
return cxxOperatorCallExpr(
255+
callee(cxxMethodDecl(parameterCountIs(0), isConst(),
256+
returns(possiblyReferencedStatusOrType()))));
257+
}
258+
259+
static auto isConstStatusOrPointerAccessorMemberCall() {
260+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
261+
return cxxMemberCallExpr(callee(cxxMethodDecl(
262+
parameterCountIs(0), isConst(),
263+
returns(pointerType(pointee(possiblyReferencedStatusOrType()))))));
264+
}
265+
266+
static auto isConstStatusOrPointerAccessorMemberOperatorCall() {
267+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
268+
return cxxOperatorCallExpr(callee(cxxMethodDecl(
269+
parameterCountIs(0), isConst(),
270+
returns(pointerType(pointee(possiblyReferencedStatusOrType()))))));
271+
}
272+
273+
static auto isNonConstMemberCall() {
274+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
275+
return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
276+
}
277+
278+
static auto isNonConstMemberOperatorCall() {
279+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
280+
return cxxOperatorCallExpr(callee(cxxMethodDecl(unless(isConst()))));
281+
}
282+
240283
static auto
241284
buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) {
242285
return CFGMatchSwitchBuilder<const Environment,
@@ -698,6 +741,114 @@ static void transferPointerToBoolean(const ImplicitCastExpr *Expr,
698741
State.Env.setValue(*Expr, *SubExprVal);
699742
}
700743

744+
static void transferStatusOrReturningCall(const CallExpr *Expr,
745+
LatticeTransferState &State) {
746+
RecordStorageLocation *StatusOrLoc =
747+
Expr->isPRValue() ? &State.Env.getResultObjectLocation(*Expr)
748+
: State.Env.get<RecordStorageLocation>(*Expr);
749+
if (StatusOrLoc != nullptr &&
750+
State.Env.getValue(locForOk(locForStatus(*StatusOrLoc))) == nullptr)
751+
initializeStatusOr(*StatusOrLoc, State.Env);
752+
}
753+
754+
static bool doHandleConstStatusOrAccessorMemberCall(
755+
const CallExpr *Expr, RecordStorageLocation *RecordLoc,
756+
const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
757+
assert(isStatusOrType(Expr->getType()));
758+
if (RecordLoc == nullptr)
759+
return false;
760+
const FunctionDecl *DirectCallee = Expr->getDirectCallee();
761+
if (DirectCallee == nullptr)
762+
return false;
763+
StorageLocation &Loc =
764+
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
765+
*RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
766+
initializeStatusOr(cast<RecordStorageLocation>(Loc), State.Env);
767+
});
768+
if (Expr->isPRValue()) {
769+
auto &ResultLoc = State.Env.getResultObjectLocation(*Expr);
770+
copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
771+
} else {
772+
State.Env.setStorageLocation(*Expr, Loc);
773+
}
774+
return true;
775+
}
776+
777+
static void handleConstStatusOrAccessorMemberCall(
778+
const CallExpr *Expr, RecordStorageLocation *RecordLoc,
779+
const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
780+
if (!doHandleConstStatusOrAccessorMemberCall(Expr, RecordLoc, Result, State))
781+
transferStatusOrReturningCall(Expr, State);
782+
}
783+
static void handleConstStatusOrPointerAccessorMemberCall(
784+
const CallExpr *Expr, RecordStorageLocation *RecordLoc,
785+
const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
786+
if (RecordLoc == nullptr)
787+
return;
788+
auto *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, Expr,
789+
State.Env);
790+
State.Env.setValue(*Expr, *Val);
791+
}
792+
793+
static void
794+
transferConstStatusOrAccessorMemberCall(const CXXMemberCallExpr *Expr,
795+
const MatchFinder::MatchResult &Result,
796+
LatticeTransferState &State) {
797+
handleConstStatusOrAccessorMemberCall(
798+
Expr, getImplicitObjectLocation(*Expr, State.Env), Result, State);
799+
}
800+
801+
static void transferConstStatusOrAccessorMemberOperatorCall(
802+
const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &Result,
803+
LatticeTransferState &State) {
804+
auto *RecordLoc = cast_or_null<RecordStorageLocation>(
805+
State.Env.getStorageLocation(*Expr->getArg(0)));
806+
handleConstStatusOrAccessorMemberCall(Expr, RecordLoc, Result, State);
807+
}
808+
809+
static void transferConstStatusOrPointerAccessorMemberCall(
810+
const CXXMemberCallExpr *Expr, const MatchFinder::MatchResult &Result,
811+
LatticeTransferState &State) {
812+
handleConstStatusOrPointerAccessorMemberCall(
813+
Expr, getImplicitObjectLocation(*Expr, State.Env), Result, State);
814+
}
815+
816+
static void transferConstStatusOrPointerAccessorMemberOperatorCall(
817+
const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &Result,
818+
LatticeTransferState &State) {
819+
auto *RecordLoc = cast_or_null<RecordStorageLocation>(
820+
State.Env.getStorageLocation(*Expr->getArg(0)));
821+
handleConstStatusOrPointerAccessorMemberCall(Expr, RecordLoc, Result, State);
822+
}
823+
824+
static void handleNonConstMemberCall(const CallExpr *Expr,
825+
RecordStorageLocation *RecordLoc,
826+
const MatchFinder::MatchResult &Result,
827+
LatticeTransferState &State) {
828+
if (RecordLoc) {
829+
State.Lattice.clearConstMethodReturnValues(*RecordLoc);
830+
State.Lattice.clearConstMethodReturnStorageLocations(*RecordLoc);
831+
}
832+
if (isStatusOrType(Expr->getType()))
833+
transferStatusOrReturningCall(Expr, State);
834+
}
835+
836+
static void transferNonConstMemberCall(const CXXMemberCallExpr *Expr,
837+
const MatchFinder::MatchResult &Result,
838+
LatticeTransferState &State) {
839+
handleNonConstMemberCall(Expr, getImplicitObjectLocation(*Expr, State.Env),
840+
Result, State);
841+
}
842+
843+
static void
844+
transferNonConstMemberOperatorCall(const CXXOperatorCallExpr *Expr,
845+
const MatchFinder::MatchResult &Result,
846+
LatticeTransferState &State) {
847+
auto *RecordLoc = cast_or_null<RecordStorageLocation>(
848+
State.Env.getStorageLocation(*Expr->getArg(0)));
849+
handleNonConstMemberCall(Expr, RecordLoc, Result, State);
850+
}
851+
701852
CFGMatchSwitch<LatticeTransferState>
702853
buildTransferMatchSwitch(ASTContext &Ctx,
703854
CFGMatchSwitchBuilder<LatticeTransferState> Builder) {
@@ -755,6 +906,23 @@ buildTransferMatchSwitch(ASTContext &Ctx,
755906
transferLoggingGetReferenceableValueCall)
756907
.CaseOfCFGStmt<CallExpr>(isLoggingCheckEqImpl(),
757908
transferLoggingCheckEqImpl)
909+
// const accessor calls
910+
.CaseOfCFGStmt<CXXMemberCallExpr>(isConstStatusOrAccessorMemberCall(),
911+
transferConstStatusOrAccessorMemberCall)
912+
.CaseOfCFGStmt<CXXOperatorCallExpr>(
913+
isConstStatusOrAccessorMemberOperatorCall(),
914+
transferConstStatusOrAccessorMemberOperatorCall)
915+
.CaseOfCFGStmt<CXXMemberCallExpr>(
916+
isConstStatusOrPointerAccessorMemberCall(),
917+
transferConstStatusOrPointerAccessorMemberCall)
918+
.CaseOfCFGStmt<CXXOperatorCallExpr>(
919+
isConstStatusOrPointerAccessorMemberOperatorCall(),
920+
transferConstStatusOrPointerAccessorMemberOperatorCall)
921+
// non-const member calls that may modify the state of an object.
922+
.CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
923+
transferNonConstMemberCall)
924+
.CaseOfCFGStmt<CXXOperatorCallExpr>(isNonConstMemberOperatorCall(),
925+
transferNonConstMemberOperatorCall)
758926
// N.B. These need to come after all other CXXConstructExpr.
759927
// These are there to make sure that every Status and StatusOr object
760928
// have their ok boolean initialized when constructed. If we were to

‎clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3270,6 +3270,179 @@ TEST_P(UncheckedStatusOrAccessModelTest, ConstructStatusFromReference) {
32703270
)cc");
32713271
}
32723272

3273+
TEST_P(UncheckedStatusOrAccessModelTest, AccessorCall) {
3274+
// Accessor returns reference.
3275+
ExpectDiagnosticsFor(
3276+
R"cc(
3277+
#include "unchecked_statusor_access_test_defs.h"
3278+
3279+
struct Foo {
3280+
STATUSOR_INT sor_;
3281+
3282+
const STATUSOR_INT& sor() const { return sor_; }
3283+
};
3284+
3285+
void target(Foo foo) {
3286+
if (foo.sor().ok()) foo.sor().value();
3287+
}
3288+
)cc");
3289+
3290+
// Uses an operator
3291+
ExpectDiagnosticsFor(
3292+
R"cc(
3293+
#include "unchecked_statusor_access_test_defs.h"
3294+
3295+
struct Foo {
3296+
STATUSOR_INT sor_;
3297+
3298+
const STATUSOR_INT& operator()() const { return sor_; }
3299+
};
3300+
3301+
void target(Foo foo) {
3302+
if (foo().ok()) foo().value();
3303+
}
3304+
)cc");
3305+
3306+
// Calls nonconst method in between.
3307+
ExpectDiagnosticsFor(
3308+
R"cc(
3309+
#include "unchecked_statusor_access_test_defs.h"
3310+
3311+
struct Foo {
3312+
STATUSOR_INT sor_;
3313+
3314+
void invalidate() {}
3315+
3316+
const STATUSOR_INT& sor() const { return sor_; }
3317+
};
3318+
3319+
void target(Foo foo) {
3320+
if (foo.sor().ok()) {
3321+
foo.invalidate();
3322+
foo.sor().value(); // [[unsafe]]
3323+
}
3324+
}
3325+
)cc");
3326+
3327+
// Calls nonconst operator in between.
3328+
ExpectDiagnosticsFor(
3329+
R"cc(
3330+
#include "unchecked_statusor_access_test_defs.h"
3331+
3332+
struct Foo {
3333+
STATUSOR_INT sor_;
3334+
3335+
void operator()() {}
3336+
3337+
const STATUSOR_INT& sor() const { return sor_; }
3338+
};
3339+
3340+
void target(Foo foo) {
3341+
if (foo.sor().ok()) {
3342+
foo();
3343+
foo.sor().value(); // [[unsafe]]
3344+
}
3345+
}
3346+
)cc");
3347+
3348+
// Accessor returns copy.
3349+
ExpectDiagnosticsFor(
3350+
R"cc(
3351+
#include "unchecked_statusor_access_test_defs.h"
3352+
3353+
struct Foo {
3354+
STATUSOR_INT sor_;
3355+
3356+
STATUSOR_INT sor() const { return sor_; }
3357+
};
3358+
3359+
void target(Foo foo) {
3360+
if (foo.sor().ok()) foo.sor().value();
3361+
}
3362+
)cc");
3363+
3364+
// Non-const accessor.
3365+
ExpectDiagnosticsFor(
3366+
R"cc(
3367+
#include "unchecked_statusor_access_test_defs.h"
3368+
3369+
struct Foo {
3370+
STATUSOR_INT sor_;
3371+
3372+
const STATUSOR_INT& sor() { return sor_; }
3373+
};
3374+
3375+
void target(Foo foo) {
3376+
if (foo.sor().ok()) foo.sor().value(); // [[unsafe]]
3377+
}
3378+
)cc");
3379+
3380+
// Non-const rvalue accessor.
3381+
ExpectDiagnosticsFor(
3382+
R"cc(
3383+
#include "unchecked_statusor_access_test_defs.h"
3384+
3385+
struct Foo {
3386+
STATUSOR_INT sor_;
3387+
3388+
STATUSOR_INT&& sor() { return std::move(sor_); }
3389+
};
3390+
3391+
void target(Foo foo) {
3392+
if (foo.sor().ok()) foo.sor().value(); // [[unsafe]]
3393+
}
3394+
)cc");
3395+
3396+
// const pointer accessor.
3397+
ExpectDiagnosticsFor(
3398+
R"cc(
3399+
#include "unchecked_statusor_access_test_defs.h"
3400+
3401+
struct Foo {
3402+
STATUSOR_INT sor_;
3403+
3404+
const STATUSOR_INT* sor() const { return &sor_; }
3405+
};
3406+
3407+
void target(Foo foo) {
3408+
if (foo.sor()->ok()) foo.sor()->value();
3409+
}
3410+
)cc");
3411+
3412+
// const pointer operator.
3413+
ExpectDiagnosticsFor(
3414+
R"cc(
3415+
#include "unchecked_statusor_access_test_defs.h"
3416+
3417+
struct Foo {
3418+
STATUSOR_INT sor_;
3419+
3420+
const STATUSOR_INT* operator->() const { return &sor_; }
3421+
};
3422+
3423+
void target(Foo foo) {
3424+
if (foo->ok()) foo->value();
3425+
}
3426+
)cc");
3427+
3428+
// We copy the result of the accessor.
3429+
ExpectDiagnosticsFor(R"cc(
3430+
#include "unchecked_statusor_access_test_defs.h"
3431+
3432+
struct Foo {
3433+
STATUSOR_INT sor_;
3434+
3435+
const STATUSOR_INT& sor() const { return sor_; }
3436+
};
3437+
void target() {
3438+
Foo foo;
3439+
if (!foo.sor().ok()) return;
3440+
const auto sor = foo.sor();
3441+
sor.value();
3442+
}
3443+
)cc");
3444+
}
3445+
32733446
} // namespace
32743447

32753448
std::string

0 commit comments

Comments
 (0)