[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