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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
