[analyzer] MallocOverflow should consider comparisons only preceding malloc

MallocOverflow works in two phases:

1) Collects suspicious malloc calls, whose argument is a multiplication
2) Filters the aggregated list of suspicious malloc calls by iterating
   over the BasicBlocks of the CFG looking for comparison binary
   operators over the variable constituting in any suspicious malloc.

Consequently, it suppressed true-positive cases when the comparison
check was after the malloc call.
In this patch the checker will consider the relative position of the
relation check to the malloc call.

E.g.:

```lang=C++
void *check_after_malloc(int n, int x) {
  int *p = NULL;
  if (x == 42)
    p = malloc(n * sizeof(int)); // Previously **no** warning, now it
                                 // warns about this.

  // The check is after the allocation!
  if (n > 10) {
    // Do something conditionally.
  }
  return p;
}
```

Reviewed By: martong

Differential Revision: https://reviews.llvm.org/D107804

clang/docs/analyzer/checkers.rst

@@ -2190,10 +2190,6 @@ Limitations:
    not tighten the domain to prevent the overflow in the subsequent
    multiplication operation.
 
- - If the variable ``n`` participates in a comparison anywhere in the enclosing
-   function's scope, even after the ``malloc()``, the report will be still
-   suppressed.
-
  - It is an AST-based checker, thus it does not make use of the
    path-sensitive taint-analysis.
 

clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp

@@ -32,12 +32,14 @@ using llvm::APSInt;
 
 namespace {
 struct MallocOverflowCheck {
+  const CallExpr *call;
   const BinaryOperator *mulop;
   const Expr *variable;
   APSInt maxVal;
 
-  MallocOverflowCheck(const BinaryOperator *m, const Expr *v, APSInt val)
-      : mulop(m), variable(v), maxVal(std::move(val)) {}
+  MallocOverflowCheck(const CallExpr *call, const BinaryOperator *m,
+                      const Expr *v, APSInt val)
+      : call(call), mulop(m), variable(v), maxVal(std::move(val)) {}
 };
 
 class MallocOverflowSecurityChecker : public Checker<check::ASTCodeBody> {
@@ -46,8 +48,8 @@ public:
                         BugReporter &BR) const;
 
   void CheckMallocArgument(
-    SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
-    const Expr *TheArgument, ASTContext &Context) const;
+      SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
+      const CallExpr *TheCall, ASTContext &Context) const;
 
   void OutputPossibleOverflows(
     SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
@@ -62,16 +64,15 @@ static inline bool EvaluatesToZero(APSInt &Val, BinaryOperatorKind op) {
 }
 
 void MallocOverflowSecurityChecker::CheckMallocArgument(
-  SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
-  const Expr *TheArgument,
-  ASTContext &Context) const {
+    SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
+    const CallExpr *TheCall, ASTContext &Context) const {
 
   /* Look for a linear combination with a single variable, and at least
    one multiplication.
    Reject anything that applies to the variable: an explicit cast,
    conditional expression, an operation that could reduce the range
    of the result, or anything too complicated :-).  */
-  const Expr *e = TheArgument;
+  const Expr *e = TheCall->getArg(0);
   const BinaryOperator * mulop = nullptr;
   APSInt maxVal;
 
@@ -115,9 +116,8 @@ void MallocOverflowSecurityChecker::CheckMallocArgument(
   // the data so when the body of the function is completely available
   // we can check for comparisons.
 
-  // TODO: Could push this into the innermost scope where 'e' is
-  // defined, rather than the whole function.
-  PossibleMallocOverflows.push_back(MallocOverflowCheck(mulop, e, maxVal));
+  PossibleMallocOverflows.push_back(
+      MallocOverflowCheck(TheCall, mulop, e, maxVal));
 }
 
 namespace {
@@ -158,12 +158,15 @@ private:
     }
 
     void CheckExpr(const Expr *E_p) {
-      auto PredTrue = [](const MallocOverflowCheck &) { return true; };
       const Expr *E = E_p->IgnoreParenImpCasts();
+      const auto PrecedesMalloc = [E, this](const MallocOverflowCheck &c) {
+        return Context.getSourceManager().isBeforeInTranslationUnit(
+            E->getExprLoc(), c.call->getExprLoc());
+      };
       if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
-        Erase<DeclRefExpr>(DR, PredTrue);
+        Erase<DeclRefExpr>(DR, PrecedesMalloc);
       else if (const auto *ME = dyn_cast<MemberExpr>(E)) {
-        Erase<MemberExpr>(ME, PredTrue);
+        Erase<MemberExpr>(ME, PrecedesMalloc);
       }
     }
 
@@ -322,7 +325,7 @@ void MallocOverflowSecurityChecker::checkASTCodeBody(const Decl *D,
 
           if (FnInfo->isStr ("malloc") || FnInfo->isStr ("_MALLOC")) {
             if (TheCall->getNumArgs() == 1)
-              CheckMallocArgument(PossibleMallocOverflows, TheCall->getArg(0),
+              CheckMallocArgument(PossibleMallocOverflows, TheCall,
                                   mgr.getASTContext());
           }
         }

clang/test/Analysis/malloc-overflow.c

@@ -111,3 +111,40 @@ void * f14(int n)
     return NULL;
   return malloc(n * sizeof(int));  // expected-warning {{the computation of the size of the memory allocation may overflow}}
 }
+
+void *check_before_malloc(int n, int x) {
+  int *p = NULL;
+  if (n > 10)
+    return NULL;
+  if (x == 42)
+    p = malloc(n * sizeof(int)); // no-warning, the check precedes the allocation
+
+  // Do some other stuff, e.g. initialize the memory.
+  return p;
+}
+
+void *check_after_malloc(int n, int x) {
+  int *p = NULL;
+  if (x == 42)
+    p = malloc(n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
+
+  // The check is after the allocation!
+  if (n > 10) {
+    // Do something conditionally.
+  }
+  return p;
+}
+
+#define GREATER_THAN(lhs, rhs) (lhs > rhs)
+void *check_after_malloc_using_macros(int n, int x) {
+  int *p = NULL;
+  if (x == 42)
+    p = malloc(n * sizeof(int)); // expected-warning {{the computation of the size of the memory allocation may overflow}}
+
+  if (GREATER_THAN(n, 10))
+    return NULL;
+
+  // Do some other stuff, e.g. initialize the memory.
+  return p;
+}
+#undef GREATER_THAN