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

[steakhal]

Summary

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 D159105

[donat.nagy]

This seems to be a straightforward improvement over the current situation; LGTM if you test(ed) it on some real-life code (to ensure that it doesn't introduce a corner case that crashes).

clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp

46

I'd call this function performCheck or something similar, but "impl" is also fine especially if it's a traditional name (that I haven't encountered yet in clang SA code).

[steakhal]

clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp

46

Yes, I'll rename it. I was already thinking about it.

[steakhal]

This patch would cause FPs on this code:

struct S {};
void zero_size_array() {
  S arr[0];
  (void)arr;
}

Being short on time, I'll just drop this commit from the stack and come back in a late future.
This might be related to D131501.

clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp

 #include "llvm/Support/raw_ostream.h"
 #include <optional>
 
 using namespace clang;
 using namespace ento;
 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;
 
   enum OOB_Kind { OOB_Precedes, OOB_Excedes };
 
   void reportOOB(CheckerContext &C, ProgramStateRef errorState,
                  OOB_Kind kind) const;
   void reportTaintOOB(CheckerContext &C, ProgramStateRef errorState,
                       SVal TaintedSVal) const;
 
   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.
 class RegionRawOffsetV2 {
 private:
   const SubRegion *baseRegion;
   NonLoc byteOffset;
 
if (BelowThreshold)
     return State->assume(*BelowThreshold);
 
   return {nullptr, nullptr};
 }
 
 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.
   // Once that logic is more mature, we can bring it back to assumeInBound()
   // for all clients to use.
   //
   // The algorithm we are using here for bounds checking is to see if the
   // memory access is within the extent of the base region.  Since we

clang/test/Analysis/out-of-bounds-new.cpp

void test_dynamic_size(int s) {
   buf[0] = 1; // no-warning
 }
 //Tests complex arithmetic
 //in new expression
 void test_dynamic_size2(unsigned m,unsigned n){
   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;
+  }
+}