Index: lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp +++ lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp @@ -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 { @@ -54,6 +57,11 @@ }; } // 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 &PossibleMallocOverflows, const Expr *TheArgument, @@ -64,13 +72,14 @@ 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(e)) { - const BinaryOperator * binop = dyn_cast(e); + if (const BinaryOperator *binop = dyn_cast(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 @@ 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(e) || isa(e)) @@ -103,7 +118,7 @@ // 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 @@ 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 + void Erase(const T1 *DR, std::function pred) { theVecType::iterator i = toScanFor.end(); theVecType::iterator e = toScanFor.begin(); - - if (const DeclRefExpr *DR = dyn_cast(E)) { - const Decl * EdreD = DR->getDecl(); - while (i != e) { - --i; - if (const DeclRefExpr *DR_i = dyn_cast(i->variable)) { - if (DR_i->getDecl() == EdreD) - i = toScanFor.erase(i); - } + while (i != e) { + --i; + if (const T1 *DR_i = dyn_cast(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(E)) + Erase(DR, PredTrue); else if (const auto *ME = dyn_cast(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(i->variable)) { - if (ME_i->getMemberDecl() == EmeMD) - i = toScanFor.erase (i); - } + Erase(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 = + // 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(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(E)) + Erase(DR, pred); + else if (const auto *ME = dyn_cast(E)) + Erase(ME, pred); } public: @@ -162,11 +228,13 @@ 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::VisitBinaryOperator(E); } @@ -243,12 +311,12 @@ 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) Index: test/Analysis/malloc-overflow.c =================================================================== --- test/Analysis/malloc-overflow.c +++ test/Analysis/malloc-overflow.c @@ -102,7 +102,7 @@ { 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) Index: test/Analysis/malloc-overflow2.c =================================================================== --- /dev/null +++ test/Analysis/malloc-overflow2.c @@ -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}} +}