Index: lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp +++ lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp @@ -23,18 +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) {} }; @@ -54,6 +58,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 +73,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,11 +90,19 @@ const Expr *lhs = binop->getLHS(); const Expr *rhs = binop->getRHS(); - if (rhs->isEvaluatable(Context)) + if (rhs->isEvaluatable(Context)) { e = lhs; + maxVal = rhs->EvaluateKnownConstInt(Context); + if (EvaluatesToZero(maxVal, opc)) + return; + } else if ((opc == BO_Add || opc == BO_Mul) - && lhs->isEvaluatable(Context)) + && lhs->isEvaluatable(Context)) { + maxVal = lhs->EvaluateKnownConstInt(Context); + if (EvaluatesToZero(maxVal, opc)) + return; e = rhs; + } else return; } @@ -103,7 +121,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 { @@ -133,9 +151,65 @@ theVecType::iterator e = toScanFor.begin(); if (const DeclRefExpr *DR = dyn_cast(E)) { - const Decl * EdreD = DR->getDecl(); + 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); + } + } + } + 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); + } + } + } + } + // 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 eraseAssign = false, eraseDiv = false; + APSInt denomVal; + denomVal = 0; + // Erase if the multiplicand was assigned a constant value. + const Expr *rhs = AssignEx->getRHS(); + if (rhs->isEvaluatable(Context)) + eraseAssign = true; + // Erase 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)) + eraseDiv = true; + } + } + if (!eraseAssign && !eraseDiv) + return; + const Expr *lhs = AssignEx->getLHS(); + const Expr *E = lhs->IgnoreParenImpCasts(); + + 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 (eraseDiv && (denomVal.getExtValue() < i->maxVal.getExtValue())) + continue; if (const DeclRefExpr *DR_i = dyn_cast(i->variable)) { if (DR_i->getDecl() == EdreD) i = toScanFor.erase(i); @@ -147,6 +221,8 @@ const Decl *EmeMD = ME->getMemberDecl(); while (i != e) { --i; + if (eraseDiv && (denomVal.getExtValue() < i->maxVal.getExtValue())) + continue; if (const auto *ME_i = dyn_cast(i->variable)) { if (ME_i->getMemberDecl() == EmeMD) i = toScanFor.erase (i); @@ -162,11 +238,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 +321,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,34 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.MallocOverflow,unix -verify %s + +typedef __typeof__(sizeof(int)) size_t; +extern void * malloc(size_t); + +void *malloc(unsigned long s); + +struct table { + int nentry; + unsigned *table; + unsigned offset_max; +}; + +static int table_build(struct table *t) { + + struct offset *offset; + unsigned i, m; + + t->nentry = ((t->offset_max >> 2) + 31) / 32; + t->table = (unsigned *)malloc(sizeof(unsigned) * t->nentry); // no-warning + + + int n; + n = 10000; + int *p = malloc(sizeof(int) * n); // no-warning + +return 0; +} + +void * f(int n) +{ + return malloc(n * 0 * sizeof(int)); // expected-warning {{Call to 'malloc' has an allocation size of 0 bytes}} +} +