r248446 - [analyzer] Discard malloc-overflow bug-report when a known size is malloc'ed.
Devin Coughlin via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 23 16:27:56 PDT 2015
Author: dcoughlin
Date: Wed Sep 23 18:27:55 2015
New Revision: 248446
URL: http://llvm.org/viewvc/llvm-project?rev=248446&view=rev
Log:
[analyzer] Discard malloc-overflow bug-report when a known size is malloc'ed.
This patch ignores malloc-overflow bug in two cases:
Case1:
x = a/b; where n < b
malloc (x*n); Then x*n will not overflow.
Case2:
x = a; // when 'a' is a known value.
malloc (x*n);
Also replaced isa with dyn_cast.
Reject multiplication by zero cases in MallocOverflowSecurityChecker
Currently MallocOverflowSecurityChecker does not catch cases like:
malloc(n * 0 * sizeof(int));
This patch rejects such cases.
Two test cases added. malloc-overflow2.c has an example inspired from a code
in linux kernel where the current checker flags a warning while it should not.
A patch by Aditya Kumar!
Differential Revision: http://reviews.llvm.org/D9924
Added:
cfe/trunk/test/Analysis/malloc-overflow2.c
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
cfe/trunk/test/Analysis/malloc-overflow.c
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp?rev=248446&r1=248445&r2=248446&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp Wed Sep 23 18:27:55 2015
@@ -23,19 +23,22 @@
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/SmallVector.h"
using namespace clang;
using namespace ento;
+using llvm::APInt;
+using llvm::APSInt;
namespace {
struct MallocOverflowCheck {
const BinaryOperator *mulop;
const Expr *variable;
+ APSInt maxVal;
- MallocOverflowCheck (const BinaryOperator *m, const Expr *v)
- : mulop(m), variable (v)
- {}
+ MallocOverflowCheck(const BinaryOperator *m, const Expr *v, APSInt val)
+ : mulop(m), variable(v), maxVal(val) {}
};
class MallocOverflowSecurityChecker : public Checker<check::ASTCodeBody> {
@@ -54,6 +57,11 @@ public:
};
} // end anonymous namespace
+// Return true for computations which evaluate to zero: e.g., mult by 0.
+static inline bool EvaluatesToZero(APSInt &Val, BinaryOperatorKind op) {
+ return (op == BO_Mul) && (Val == 0);
+}
+
void MallocOverflowSecurityChecker::CheckMallocArgument(
SmallVectorImpl<MallocOverflowCheck> &PossibleMallocOverflows,
const Expr *TheArgument,
@@ -64,13 +72,14 @@ void MallocOverflowSecurityChecker::Chec
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 = TheArgument;
const BinaryOperator * mulop = nullptr;
+ APSInt maxVal;
for (;;) {
+ maxVal = 0;
e = e->IgnoreParenImpCasts();
- if (isa<BinaryOperator>(e)) {
- const BinaryOperator * binop = dyn_cast<BinaryOperator>(e);
+ if (const BinaryOperator *binop = dyn_cast<BinaryOperator>(e)) {
BinaryOperatorKind opc = binop->getOpcode();
// TODO: ignore multiplications by 1, reject if multiplied by 0.
if (mulop == nullptr && opc == BO_Mul)
@@ -80,12 +89,18 @@ void MallocOverflowSecurityChecker::Chec
const Expr *lhs = binop->getLHS();
const Expr *rhs = binop->getRHS();
- if (rhs->isEvaluatable(Context))
+ if (rhs->isEvaluatable(Context)) {
e = lhs;
- else if ((opc == BO_Add || opc == BO_Mul)
- && lhs->isEvaluatable(Context))
+ maxVal = rhs->EvaluateKnownConstInt(Context);
+ if (EvaluatesToZero(maxVal, opc))
+ return;
+ } else if ((opc == BO_Add || opc == BO_Mul) &&
+ lhs->isEvaluatable(Context)) {
+ maxVal = lhs->EvaluateKnownConstInt(Context);
+ if (EvaluatesToZero(maxVal, opc))
+ return;
e = rhs;
- else
+ } else
return;
}
else if (isa<DeclRefExpr>(e) || isa<MemberExpr>(e))
@@ -103,7 +118,7 @@ void MallocOverflowSecurityChecker::Chec
// TODO: Could push this into the innermost scope where 'e' is
// defined, rather than the whole function.
- PossibleMallocOverflows.push_back(MallocOverflowCheck(mulop, e));
+ PossibleMallocOverflows.push_back(MallocOverflowCheck(mulop, e, maxVal));
}
namespace {
@@ -126,33 +141,84 @@ private:
return false;
}
- void CheckExpr(const Expr *E_p) {
- const Expr *E = E_p->IgnoreParenImpCasts();
+ const Decl *getDecl(const DeclRefExpr *DR) { return DR->getDecl(); }
+
+ const Decl *getDecl(const MemberExpr *ME) { return ME->getMemberDecl(); }
+ template <typename T1>
+ void Erase(const T1 *DR, std::function<bool(theVecType::iterator)> pred) {
theVecType::iterator i = toScanFor.end();
theVecType::iterator e = toScanFor.begin();
-
- if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E)) {
- const Decl * EdreD = DR->getDecl();
- while (i != e) {
- --i;
- if (const DeclRefExpr *DR_i = dyn_cast<DeclRefExpr>(i->variable)) {
- if (DR_i->getDecl() == EdreD)
- i = toScanFor.erase(i);
- }
+ while (i != e) {
+ --i;
+ if (const T1 *DR_i = dyn_cast<T1>(i->variable)) {
+ if ((getDecl(DR_i) == getDecl(DR)) && pred(i))
+ i = toScanFor.erase(i);
}
}
+ }
+
+ void CheckExpr(const Expr *E_p) {
+ auto PredTrue = [](theVecType::iterator) -> bool { return true; };
+ const Expr *E = E_p->IgnoreParenImpCasts();
+ if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
+ Erase<DeclRefExpr>(DR, PredTrue);
else if (const auto *ME = dyn_cast<MemberExpr>(E)) {
- // No points-to analysis, just look at the member
- const Decl *EmeMD = ME->getMemberDecl();
- while (i != e) {
- --i;
- if (const auto *ME_i = dyn_cast<MemberExpr>(i->variable)) {
- if (ME_i->getMemberDecl() == EmeMD)
- i = toScanFor.erase (i);
- }
+ Erase<MemberExpr>(ME, PredTrue);
+ }
+ }
+
+ // Check if the argument to malloc is assigned a value
+ // which cannot cause an overflow.
+ // e.g., malloc (mul * x) and,
+ // case 1: mul = <constant value>
+ // case 2: mul = a/b, where b > x
+ void CheckAssignmentExpr(BinaryOperator *AssignEx) {
+ bool assignKnown = false;
+ bool numeratorKnown = false, denomKnown = false;
+ APSInt denomVal;
+ denomVal = 0;
+
+ // Erase if the multiplicand was assigned a constant value.
+ const Expr *rhs = AssignEx->getRHS();
+ if (rhs->isEvaluatable(Context))
+ assignKnown = true;
+
+ // Discard the report if the multiplicand was assigned a value,
+ // that can never overflow after multiplication. e.g., the assignment
+ // is a division operator and the denominator is > other multiplicand.
+ const Expr *rhse = rhs->IgnoreParenImpCasts();
+ if (const BinaryOperator *BOp = dyn_cast<BinaryOperator>(rhse)) {
+ if (BOp->getOpcode() == BO_Div) {
+ const Expr *denom = BOp->getRHS()->IgnoreParenImpCasts();
+ if (denom->EvaluateAsInt(denomVal, Context))
+ denomKnown = true;
+ const Expr *numerator = BOp->getLHS()->IgnoreParenImpCasts();
+ if (numerator->isEvaluatable(Context))
+ numeratorKnown = true;
}
}
+ if (!assignKnown && !denomKnown)
+ return;
+ auto denomExtVal = denomVal.getExtValue();
+
+ // Ignore negative denominator.
+ if (denomExtVal < 0)
+ return;
+
+ const Expr *lhs = AssignEx->getLHS();
+ const Expr *E = lhs->IgnoreParenImpCasts();
+
+ auto pred = [assignKnown, numeratorKnown,
+ denomExtVal](theVecType::iterator i) {
+ return assignKnown ||
+ (numeratorKnown && (denomExtVal >= i->maxVal.getExtValue()));
+ };
+
+ if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
+ Erase<DeclRefExpr>(DR, pred);
+ else if (const auto *ME = dyn_cast<MemberExpr>(E))
+ Erase<MemberExpr>(ME, pred);
}
public:
@@ -162,11 +228,13 @@ private:
const Expr * rhs = E->getRHS();
// Ignore comparisons against zero, since they generally don't
// protect against an overflow.
- if (!isIntZeroExpr(lhs) && ! isIntZeroExpr(rhs)) {
+ if (!isIntZeroExpr(lhs) && !isIntZeroExpr(rhs)) {
CheckExpr(lhs);
CheckExpr(rhs);
}
}
+ if (E->isAssignmentOp())
+ CheckAssignmentExpr(E);
EvaluatedExprVisitor<CheckOverflowOps>::VisitBinaryOperator(E);
}
@@ -243,12 +311,12 @@ void MallocOverflowSecurityChecker::chec
const FunctionDecl *FD = TheCall->getDirectCallee();
if (!FD)
- return;
+ continue;
// Get the name of the callee. If it's a builtin, strip off the prefix.
IdentifierInfo *FnInfo = FD->getIdentifier();
if (!FnInfo)
- return;
+ continue;
if (FnInfo->isStr ("malloc") || FnInfo->isStr ("_MALLOC")) {
if (TheCall->getNumArgs() == 1)
Modified: cfe/trunk/test/Analysis/malloc-overflow.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc-overflow.c?rev=248446&r1=248445&r2=248446&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/malloc-overflow.c (original)
+++ cfe/trunk/test/Analysis/malloc-overflow.c Wed Sep 23 18:27:55 2015
@@ -102,7 +102,7 @@ void * f13(struct s13 *s)
{
if (s->n > 10)
return NULL;
- return malloc(s->n * sizeof(int)); // no warning
+ return malloc(s->n * sizeof(int)); // no-warning
}
void * f14(int n)
Added: cfe/trunk/test/Analysis/malloc-overflow2.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc-overflow2.c?rev=248446&view=auto
==============================================================================
--- cfe/trunk/test/Analysis/malloc-overflow2.c (added)
+++ cfe/trunk/test/Analysis/malloc-overflow2.c Wed Sep 23 18:27:55 2015
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.MallocOverflow,unix -verify %s
+
+typedef __typeof__(sizeof(int)) size_t;
+extern void *malloc(size_t);
+extern void free(void *ptr);
+
+void *malloc(unsigned long s);
+
+struct table {
+ int nentry;
+ unsigned *table;
+ unsigned offset_max;
+};
+
+static int table_build(struct table *t) {
+
+ t->nentry = ((t->offset_max >> 2) + 31) / 32;
+ t->table = (unsigned *)malloc(sizeof(unsigned) * t->nentry); // expected-warning {{the computation of the size of the memory allocation may overflow}}
+
+ int n;
+ n = 10000;
+ int *p = malloc(sizeof(int) * n); // no-warning
+
+ free(p);
+ return t->nentry;
+}
+
+static int table_build_1(struct table *t) {
+ t->nentry = (sizeof(struct table) * 2 + 31) / 32;
+ t->table = (unsigned *)malloc(sizeof(unsigned) * t->nentry); // no-warning
+ return t->nentry;
+}
+
+void *f(int n) {
+ return malloc(n * 0 * sizeof(int)); // expected-warning {{Call to 'malloc' has an allocation size of 0 bytes}}
+}
More information about the cfe-commits
mailing list