From 5b5c7cec081e29f8019e967333a8d893f0c83da5 Mon Sep 17 00:00:00 2001 From: Anton Yartsev Date: Thu, 19 Feb 2015 13:36:20 +0000 Subject: [PATCH] [analyzer] Different handling of alloca(). + separate bug report for "Free alloca()" error to be able to customize checkers responsible for this error. + Muted "Free alloca()" error for NewDelete checker that is not responsible for c-allocated memory, turned on for unix.MismatchedDeallocator checker. + RefState for alloca() - to be able to detect usage of zero-allocated memory by upcoming ZeroAllocDereference checker. + AF_Alloca family to handle alloca() consistently - keep proper family in RefState, handle 'alloca' by getCheckIfTracked() facility, etc. + extra tests. llvm-svn: 229850 --- .../lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 108 +++++++++++++++------ .../Analysis/MismatchedDeallocator-checker-test.mm | 5 + clang/test/Analysis/NewDelete-checker-test.cpp | 5 - clang/test/Analysis/NewDelete-intersections.mm | 11 ++- clang/test/Analysis/free.c | 15 ++- 5 files changed, 103 insertions(+), 41 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index a49247b..0930808 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -43,7 +43,8 @@ enum AllocationFamily { AF_Malloc, AF_CXXNew, AF_CXXNewArray, - AF_IfNameIndex + AF_IfNameIndex, + AF_Alloca }; class RefState { @@ -160,10 +161,11 @@ class MallocChecker : public Checker BT_Leak[CK_NumCheckKinds]; mutable std::unique_ptr BT_UseFree[CK_NumCheckKinds]; mutable std::unique_ptr BT_BadFree[CK_NumCheckKinds]; + mutable std::unique_ptr BT_FreeAlloca[CK_NumCheckKinds]; mutable std::unique_ptr BT_MismatchedDealloc; mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds]; - mutable IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc, - *II_valloc, *II_reallocf, *II_strndup, *II_strdup, - *II_kmalloc, *II_if_nameindex, *II_if_freenameindex; + mutable IdentifierInfo *II_alloca, *II_alloca_builtin, *II_malloc, *II_free, + *II_realloc, *II_calloc, *II_valloc, *II_reallocf, + *II_strndup, *II_strdup, *II_kmalloc, *II_if_nameindex, + *II_if_freenameindex; mutable Optional KernelZeroFlagVal; void initIdentifierInfo(ASTContext &C) const; @@ -343,6 +347,8 @@ private: static bool SummarizeRegion(raw_ostream &os, const MemRegion *MR); void ReportBadFree(CheckerContext &C, SVal ArgVal, SourceRange Range, const Expr *DeallocExpr) const; + void ReportFreeAlloca(CheckerContext &C, SVal ArgVal, + SourceRange Range) const; void ReportMismatchedDealloc(CheckerContext &C, SourceRange Range, const Expr *DeallocExpr, const RefState *RS, SymbolRef Sym, bool OwnershipTransferred) const; @@ -501,6 +507,8 @@ public: void MallocChecker::initIdentifierInfo(ASTContext &Ctx) const { if (II_malloc) return; + II_alloca = &Ctx.Idents.get("alloca"); + II_alloca_builtin = &Ctx.Idents.get("__builtin_alloca"); II_malloc = &Ctx.Idents.get("malloc"); II_free = &Ctx.Idents.get("free"); II_realloc = &Ctx.Idents.get("realloc"); @@ -521,6 +529,9 @@ bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const { if (isCMemFunction(FD, C, AF_IfNameIndex, MemoryOperationKind::MOK_Any)) return true; + if (isCMemFunction(FD, C, AF_Alloca, MemoryOperationKind::MOK_Any)) + return true; + if (isStandardNewDelete(FD, C)) return true; @@ -564,6 +575,11 @@ bool MallocChecker::isCMemFunction(const FunctionDecl *FD, if (FunI == II_if_nameindex) return true; } + + if (Family == AF_Alloca && CheckAlloc) { + if (FunI == II_alloca || FunI == II_alloca_builtin) + return true; + } } if (Family != AF_Malloc) @@ -747,8 +763,10 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { State = MallocUpdateRefState(C, CE, State); } else if (FunI == II_strndup) { State = MallocUpdateRefState(C, CE, State); - } - else if (isStandardNewDelete(FD, C.getASTContext())) { + } else if (FunI == II_alloca || FunI == II_alloca_builtin) { + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State, + AF_Alloca); + } else if (isStandardNewDelete(FD, C.getASTContext())) { // Process direct calls to operator new/new[]/delete/delete[] functions // as distinct from new/new[]/delete/delete[] expressions that are // processed by the checkPostStmt callbacks for CXXNewExpr and @@ -1096,6 +1114,9 @@ AllocationFamily MallocChecker::getAllocationFamily(CheckerContext &C, if (isCMemFunction(FD, Ctx, AF_IfNameIndex, MemoryOperationKind::MOK_Any)) return AF_IfNameIndex; + if (isCMemFunction(FD, Ctx, AF_Alloca, MemoryOperationKind::MOK_Any)) + return AF_Alloca; + return AF_None; } @@ -1160,6 +1181,7 @@ void MallocChecker::printExpectedAllocName(raw_ostream &os, CheckerContext &C, case AF_CXXNew: os << "'new'"; return; case AF_CXXNewArray: os << "'new[]'"; return; case AF_IfNameIndex: os << "'if_nameindex()'"; return; + case AF_Alloca: case AF_None: llvm_unreachable("not a deallocation expression"); } } @@ -1171,7 +1193,8 @@ void MallocChecker::printExpectedDeallocName(raw_ostream &os, case AF_CXXNew: os << "'delete'"; return; case AF_CXXNewArray: os << "'delete[]'"; return; case AF_IfNameIndex: os << "'if_freenameindex()'"; return; - case AF_None: llvm_unreachable("suspicious AF_None argument"); + case AF_Alloca: + case AF_None: llvm_unreachable("suspicious argument"); } } @@ -1225,8 +1248,7 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C, const MemSpaceRegion *MS = R->getMemorySpace(); - // Parameters, locals, statics, globals, and memory returned by alloca() - // shouldn't be freed. + // Parameters, locals, statics and globals shouldn't be freed. if (!(isa(MS) || isa(MS))) { // FIXME: at the time this code was written, malloc() regions were // represented by conjured symbols, which are all in UnknownSpaceRegion. @@ -1252,6 +1274,12 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C, if (RsBase) { + // Memory returned by alloca() shouldn't be freed. + if (RsBase->getAllocationFamily() == AF_Alloca) { + ReportFreeAlloca(C, ArgVal, ArgExpr->getSourceRange()); + return nullptr; + } + // Check for double free first. if ((RsBase->isReleased() || RsBase->isRelinquished()) && !didPreviousFreeFail(State, SymBase, PreviousRetStatusSymbol)) { @@ -1327,7 +1355,8 @@ MallocChecker::getCheckIfTracked(MallocChecker::CheckKind CK, switch (Family) { case AF_Malloc: - case AF_IfNameIndex: { + case AF_IfNameIndex: + case AF_Alloca: { // C checkers. if (CK == CK_MallocOptimistic || CK == CK_MallocPessimistic) { @@ -1502,23 +1531,19 @@ void MallocChecker::ReportBadFree(CheckerContext &C, SVal ArgVal, while (const ElementRegion *ER = dyn_cast_or_null(MR)) MR = ER->getSuperRegion(); - if (MR && isa(MR)) - os << "Memory allocated by alloca() should not be deallocated"; - else { - os << "Argument to "; - if (!printAllocDeallocName(os, C, DeallocExpr)) - os << "deallocator"; - - os << " is "; - bool Summarized = MR ? SummarizeRegion(os, MR) - : SummarizeValue(os, ArgVal); - if (Summarized) - os << ", which is not memory allocated by "; - else - os << "not memory allocated by "; + os << "Argument to "; + if (!printAllocDeallocName(os, C, DeallocExpr)) + os << "deallocator"; - printExpectedAllocName(os, C, DeallocExpr); - } + os << " is "; + bool Summarized = MR ? SummarizeRegion(os, MR) + : SummarizeValue(os, ArgVal); + if (Summarized) + os << ", which is not memory allocated by "; + else + os << "not memory allocated by "; + + printExpectedAllocName(os, C, DeallocExpr); BugReport *R = new BugReport(*BT_BadFree[*CheckKind], os.str(), N); R->markInteresting(MR); @@ -1527,6 +1552,29 @@ void MallocChecker::ReportBadFree(CheckerContext &C, SVal ArgVal, } } +void MallocChecker::ReportFreeAlloca(CheckerContext &C, SVal ArgVal, + SourceRange Range) const { + + auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic, + CK_MallocPessimistic, + CK_MismatchedDeallocatorChecker), + AF_Alloca); + if (!CheckKind.hasValue()) + return; + + if (ExplodedNode *N = C.generateSink()) { + if (!BT_FreeAlloca[*CheckKind]) + BT_FreeAlloca[*CheckKind].reset( + new BugType(CheckNames[*CheckKind], "Free alloca()", "Memory Error")); + + BugReport *R = new BugReport(*BT_FreeAlloca[*CheckKind], + "Memory allocated by alloca() should not be deallocated", N); + R->markInteresting(ArgVal.getAsRegion()); + R->addRange(Range); + C.emitReport(R); + } +} + void MallocChecker::ReportMismatchedDealloc(CheckerContext &C, SourceRange Range, const Expr *DeallocExpr, diff --git a/clang/test/Analysis/MismatchedDeallocator-checker-test.mm b/clang/test/Analysis/MismatchedDeallocator-checker-test.mm index 0df5db5..15815e8 100644 --- a/clang/test/Analysis/MismatchedDeallocator-checker-test.mm +++ b/clang/test/Analysis/MismatchedDeallocator-checker-test.mm @@ -59,6 +59,11 @@ void testMalloc8() { operator delete[](p); // expected-warning{{Memory allocated by malloc() should be deallocated by free(), not operator delete[]}} } +void testAlloca() { + int *p = (int *)__builtin_alloca(sizeof(int)); + delete p; // expected-warning{{Memory allocated by alloca() should not be deallocated}} +} + //--------------- test new family void testNew1() { int *p = new int; diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp index 84176c9..3f28c2e 100644 --- a/clang/test/Analysis/NewDelete-checker-test.cpp +++ b/clang/test/Analysis/NewDelete-checker-test.cpp @@ -144,11 +144,6 @@ void testUseThisAfterDelete() { c->f(0); // expected-warning{{Use of memory after it is freed}} } -void testDeleteAlloca() { - int *p = (int *)__builtin_alloca(sizeof(int)); - delete p; // expected-warning{{Memory allocated by alloca() should not be deallocated}} -} - void testDoubleDelete() { int *p = new int; delete p; diff --git a/clang/test/Analysis/NewDelete-intersections.mm b/clang/test/Analysis/NewDelete-intersections.mm index 886df12..7b3fb6e 100644 --- a/clang/test/Analysis/NewDelete-intersections.mm +++ b/clang/test/Analysis/NewDelete-intersections.mm @@ -5,6 +5,7 @@ typedef __typeof__(sizeof(int)) size_t; extern "C" void *malloc(size_t); +extern "C" void *alloca(size_t); extern "C" void free(void *); //---------------------------------------------------------------------------- @@ -29,11 +30,17 @@ void testMallocFreeNoWarn() { int *p4 = (int *)malloc(sizeof(int)); free(p4); int j = *p4; // no warn + + int *p5 = (int *)alloca(sizeof(int)); + free(p5); // no warn } void testDeleteMalloced() { - int *p = (int *)malloc(sizeof(int)); - delete p; // no warn + int *p1 = (int *)malloc(sizeof(int)); + delete p1; // no warn + + int *p2 = (int *)__builtin_alloca(sizeof(int)); + delete p2; // no warn } //----- Test free standard new diff --git a/clang/test/Analysis/free.c b/clang/test/Analysis/free.c index 1dfc108..6f33732 100644 --- a/clang/test/Analysis/free.c +++ b/clang/test/Analysis/free.c @@ -1,6 +1,8 @@ // RUN: %clang_cc1 -analyze -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify %s // RUN: %clang_cc1 -analyze -analyzer-store=region -analyzer-checker=core,alpha.unix.MallocWithAnnotations -fblocks -verify %s +typedef __typeof(sizeof(int)) size_t; void free(void *); +void *alloca(size_t); void t1 () { int a[] = { 1 }; @@ -49,24 +51,29 @@ void t10 () { } void t11 () { - char *p = (char*)__builtin_alloca(2); + char *p = (char*)alloca(2); free(p); // expected-warning {{Memory allocated by alloca() should not be deallocated}} } void t12 () { + char *p = (char*)__builtin_alloca(2); + free(p); // expected-warning {{Memory allocated by alloca() should not be deallocated}} +} + +void t13 () { free(^{return;}); // expected-warning {{Argument to free() is a block, which is not memory allocated by malloc()}} } -void t13 (char a) { +void t14 (char a) { free(&a); // expected-warning {{Argument to free() is the address of the parameter 'a', which is not memory allocated by malloc()}} } static int someGlobal[2]; -void t14 () { +void t15 () { free(someGlobal); // expected-warning {{Argument to free() is the address of the global variable 'someGlobal', which is not memory allocated by malloc()}} } -void t15 (char **x, int offset) { +void t16 (char **x, int offset) { // Unknown value free(x[offset]); // no-warning } -- 2.7.4