steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun, donat.nagy, Szelethus. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. Herald added a project: All. steakhal requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits.
Turns out check::Location is not always called when storing a value to a location. See the details here: https://discourse.llvm.org/t/checklocation-vs-checkbind-when-isload-false/72728 To catch the remaining cases when binding to a location, subscribe to the check::Bind callback. (CWE-122 Heap Based Buffer Overflow: CWE-805-class-loop) Depends on D554351 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D159106 Files: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp clang/test/Analysis/out-of-bounds-new.cpp Index: clang/test/Analysis/out-of-bounds-new.cpp =================================================================== --- clang/test/Analysis/out-of-bounds-new.cpp +++ clang/test/Analysis/out-of-bounds-new.cpp @@ -154,3 +154,25 @@ unsigned *U = nullptr; U = new unsigned[m + n + 1]; } + + +int rng(); +struct ManyInts { + int a, b, c, d, e, f, g; +}; +void test_trivial_copy_move_is_checked_by_the_checker() { + ManyInts v; + ManyInts *p = &v; + + switch (rng()) { + case 0: + *p = ManyInts{3,2,1}; // ok + break; + case -1: + *--p = ManyInts{3,2,1}; // expected-warning {{Out of bound memory access (accessed memory precedes memory block)}} + break; + case 1: + *++p = ManyInts{3,2,1}; // expected-warning {{Out of bound memory access (access exceeds upper limit of memory block)}} + break; + } +} Index: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp +++ clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp @@ -30,8 +30,7 @@ using namespace taint; namespace { -class ArrayBoundCheckerV2 : - public Checker<check::Location> { +class ArrayBoundCheckerV2 : public Checker<check::Location, check::Bind> { mutable std::unique_ptr<BugType> BT; mutable std::unique_ptr<BugType> TaintBT; @@ -44,9 +43,12 @@ static bool isFromCtypeMacro(const Stmt *S, ASTContext &AC); + void impl(SVal Loc, bool isLoad, const Stmt *S, CheckerContext &C) const; + public: void checkLocation(SVal l, bool isLoad, const Stmt *S, CheckerContext &C) const; + void checkBind(SVal Loc, SVal, const Stmt *S, CheckerContext &) const; }; // FIXME: Eventually replace RegionRawOffset with this class. @@ -143,6 +145,17 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad, const Stmt* LoadS, CheckerContext &checkerContext) const { + if (isLoad) + impl(location, isLoad, LoadS, checkerContext); +} + +void ArrayBoundCheckerV2::checkBind(SVal Loc, SVal, const Stmt *S, + CheckerContext &C) const { + impl(Loc, /*isLoad=*/false, S, C); +} + +void ArrayBoundCheckerV2::impl(SVal location, bool isLoad, const Stmt *LoadS, + CheckerContext &checkerContext) const { // NOTE: Instead of using ProgramState::assumeInBound(), we are prototyping // some new logic here that reasons directly about memory region extents.
Index: clang/test/Analysis/out-of-bounds-new.cpp =================================================================== --- clang/test/Analysis/out-of-bounds-new.cpp +++ clang/test/Analysis/out-of-bounds-new.cpp @@ -154,3 +154,25 @@ unsigned *U = nullptr; U = new unsigned[m + n + 1]; } + + +int rng(); +struct ManyInts { + int a, b, c, d, e, f, g; +}; +void test_trivial_copy_move_is_checked_by_the_checker() { + ManyInts v; + ManyInts *p = &v; + + switch (rng()) { + case 0: + *p = ManyInts{3,2,1}; // ok + break; + case -1: + *--p = ManyInts{3,2,1}; // expected-warning {{Out of bound memory access (accessed memory precedes memory block)}} + break; + case 1: + *++p = ManyInts{3,2,1}; // expected-warning {{Out of bound memory access (access exceeds upper limit of memory block)}} + break; + } +} Index: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp +++ clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp @@ -30,8 +30,7 @@ using namespace taint; namespace { -class ArrayBoundCheckerV2 : - public Checker<check::Location> { +class ArrayBoundCheckerV2 : public Checker<check::Location, check::Bind> { mutable std::unique_ptr<BugType> BT; mutable std::unique_ptr<BugType> TaintBT; @@ -44,9 +43,12 @@ static bool isFromCtypeMacro(const Stmt *S, ASTContext &AC); + void impl(SVal Loc, bool isLoad, const Stmt *S, CheckerContext &C) const; + public: void checkLocation(SVal l, bool isLoad, const Stmt *S, CheckerContext &C) const; + void checkBind(SVal Loc, SVal, const Stmt *S, CheckerContext &) const; }; // FIXME: Eventually replace RegionRawOffset with this class. @@ -143,6 +145,17 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad, const Stmt* LoadS, CheckerContext &checkerContext) const { + if (isLoad) + impl(location, isLoad, LoadS, checkerContext); +} + +void ArrayBoundCheckerV2::checkBind(SVal Loc, SVal, const Stmt *S, + CheckerContext &C) const { + impl(Loc, /*isLoad=*/false, S, C); +} + +void ArrayBoundCheckerV2::impl(SVal location, bool isLoad, const Stmt *LoadS, + CheckerContext &checkerContext) const { // NOTE: Instead of using ProgramState::assumeInBound(), we are prototyping // some new logic here that reasons directly about memory region extents.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits