[clang] 6808856 - [analyzer] MallocOverflow should consider comparisons only preceding malloc

Balazs Benics via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 27 05:41:49 PDT 2021


Author: Balazs Benics
Date: 2021-08-27T14:41:26+02:00
New Revision: 68088563fbadba92a153cbe03c1586033b19322d

URL: https://github.com/llvm/llvm-project/commit/68088563fbadba92a153cbe03c1586033b19322d
DIFF: https://github.com/llvm/llvm-project/commit/68088563fbadba92a153cbe03c1586033b19322d.diff

LOG: [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

Added: 
    

Modified: 
    clang/docs/analyzer/checkers.rst
    clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
    clang/test/Analysis/malloc-overflow.c

Removed: 
    


################################################################################
diff  --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index dc8698b8f0c8a..858c8e1303e82 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/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.
 

diff  --git a/clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
index e31630f63b5ac..7f1d0ac354f9b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
+++ b/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 @@ class MallocOverflowSecurityChecker : public Checker<check::ASTCodeBody> {
                         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 @@ class CheckOverflowOps :
     }
 
     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());
           }
         }

diff  --git a/clang/test/Analysis/malloc-overflow.c b/clang/test/Analysis/malloc-overflow.c
index d8ad062840d58..dada27bccbfd6 100644
--- a/clang/test/Analysis/malloc-overflow.c
+++ b/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


        


More information about the cfe-commits mailing list