[PATCH] D159106: [analyzer] ArrayBoundCheckerV2 should listen to check::Bind as well

[Balázs Benics via Phabricator via cfe-commits]

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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D159106.554352.patch
Type: text/x-patch
Size: 2607 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230829/0f4ba0f4/attachment.bin>