From: Anna Zaks Date: Tue, 9 Apr 2013 00:30:28 +0000 (+0000) Subject: [analyzer] Keep tracking the pointer after the escape to more aggressively report... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0413023bed8ec91d3642cd6ff114957badf51f31;p=clang [analyzer] Keep tracking the pointer after the escape to more aggressively report mismatched deallocator Test that the path notes do not change. I don’t think we should print a note on escape. Also, I’ve removed a check that assumed that the family stored in the RefStete could be AF_None and added an assert in the constructor. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@179075 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 51205d863a..cc5c6040f3 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -50,7 +50,12 @@ class RefState { Released, // The responsibility for freeing resources has transfered from // this reference. A relinquished symbol should not be freed. - Relinquished }; + Relinquished, + // We are no longer guaranteed to have observed all manipulations + // of this pointer/memory. For example, it could have been + // passed as a parameter to an opaque function. + Escaped + }; const Stmt *S; unsigned K : 2; // Kind enum, but stored as a bitfield. @@ -58,12 +63,15 @@ class RefState { // family. RefState(Kind k, const Stmt *s, unsigned family) - : S(s), K(k), Family(family) {} + : S(s), K(k), Family(family) { + assert(family != AF_None); + } public: bool isAllocated() const { return K == Allocated; } bool isReleased() const { return K == Released; } bool isRelinquished() const { return K == Relinquished; } - AllocationFamily getAllocationFamily() const { + bool isEscaped() const { return K == Escaped; } + AllocationFamily getAllocationFamily() const { return (AllocationFamily)Family; } const Stmt *getStmt() const { return S; } @@ -81,6 +89,9 @@ public: static RefState getRelinquished(unsigned family, const Stmt *s) { return RefState(Relinquished, s, family); } + static RefState getEscaped(const RefState *RS) { + return RefState(Escaped, RS->getStmt(), RS->getAllocationFamily()); + } void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); @@ -1008,37 +1019,37 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C, if (RsBase) { - bool DeallocMatchesAlloc = - RsBase->getAllocationFamily() == AF_None || - RsBase->getAllocationFamily() == getAllocationFamily(C, ParentExpr); - - // Check if an expected deallocation function matches the real one. - if (!DeallocMatchesAlloc && RsBase->isAllocated()) { - ReportMismatchedDealloc(C, ArgExpr->getSourceRange(), ParentExpr, RsBase, - SymBase); - return 0; - } - - // Check double free. - if (DeallocMatchesAlloc && - (RsBase->isReleased() || RsBase->isRelinquished()) && + // Check for double free first. + if ((RsBase->isReleased() || RsBase->isRelinquished()) && !didPreviousFreeFail(State, SymBase, PreviousRetStatusSymbol)) { ReportDoubleFree(C, ParentExpr->getSourceRange(), RsBase->isReleased(), SymBase, PreviousRetStatusSymbol); return 0; - } - // Check if the memory location being freed is the actual location - // allocated, or an offset. - RegionOffset Offset = R->getAsOffset(); - if (RsBase->isAllocated() && - Offset.isValid() && - !Offset.hasSymbolicOffset() && - Offset.getOffset() != 0) { - const Expr *AllocExpr = cast(RsBase->getStmt()); - ReportOffsetFree(C, ArgVal, ArgExpr->getSourceRange(), ParentExpr, - AllocExpr); - return 0; + // If the pointer is allocated or escaped, but we are now trying to free it, + // check that the call to free is proper. + } else if (RsBase->isAllocated() || RsBase->isEscaped()) { + + // Check if an expected deallocation function matches the real one. + bool DeallocMatchesAlloc = + RsBase->getAllocationFamily() == getAllocationFamily(C, ParentExpr); + if (!DeallocMatchesAlloc) { + ReportMismatchedDealloc(C, ArgExpr->getSourceRange(), + ParentExpr, RsBase, SymBase); + return 0; + } + + // Check if the memory location being freed is the actual location + // allocated, or an offset. + RegionOffset Offset = R->getAsOffset(); + if (Offset.isValid() && + !Offset.hasSymbolicOffset() && + Offset.getOffset() != 0) { + const Expr *AllocExpr = cast(RsBase->getStmt()); + ReportOffsetFree(C, ArgVal, ArgExpr->getSourceRange(), ParentExpr, + AllocExpr); + return 0; + } } } @@ -1992,8 +2003,10 @@ ProgramStateRef MallocChecker::checkPointerEscapeAux(ProgramStateRef State, SymbolRef sym = *I; if (const RefState *RS = State->get(sym)) { - if (RS->isAllocated() && CheckRefState(RS)) + if (RS->isAllocated() && CheckRefState(RS)) { State = State->remove(sym); + State = State->set(sym, RefState::getEscaped(RS)); + } } } return State; diff --git a/test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp b/test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp index 666ff966fe..b1ee4c85cf 100644 --- a/test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp +++ b/test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp @@ -73,3 +73,35 @@ void testNewOffsetFree() { int *p = new int; operator delete(++p); // expected-warning{{Argument to operator delete is offset by 4 bytes from the start of memory allocated by 'new'}} } + +//---------------------------------------------------------------- +// Test that we check for free errors on escaped pointers. +//---------------------------------------------------------------- +void changePtr(int **p); +static int *globalPtr; +void changePointee(int *p); + +void testMismatchedChangePtrThroughCall() { + int *p = (int*)malloc(sizeof(int)*4); + changePtr(&p); + delete p; // no-warning the value of the pointer might have changed +} + +void testMismatchedChangePointeeThroughCall() { + int *p = (int*)malloc(sizeof(int)*4); + changePointee(p); + delete p; // expected-warning{{Memory allocated by malloc() should be deallocated by free(), not 'delete'}} +} + +void testShouldReportDoubleFreeNotMismatched() { + int *p = (int*)malloc(sizeof(int)*4); + globalPtr = p; + free(p); + delete globalPtr; // expected-warning {{Attempt to free released memory}} +} + +void testMismatchedChangePointeeThroughAssignment() { + int *arr = new int[4]; + globalPtr = arr; + delete arr; // expected-warning{{Memory allocated by 'new[]' should be deallocated by 'delete[]', not 'delete'}} +} \ No newline at end of file diff --git a/test/Analysis/MismatchedDeallocator-path-notes.cpp b/test/Analysis/MismatchedDeallocator-path-notes.cpp index 3c2b88f054..369d8f6975 100644 --- a/test/Analysis/MismatchedDeallocator-path-notes.cpp +++ b/test/Analysis/MismatchedDeallocator-path-notes.cpp @@ -2,156 +2,158 @@ // RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.MismatchedDeallocator -analyzer-output=plist %s -o %t.plist // RUN: FileCheck --input-file=%t.plist %s +void changePointee(int *p); void test() { int *p = new int[1]; // expected-note@-1 {{Memory is allocated}} + changePointee(p); delete p; // expected-warning {{Memory allocated by 'new[]' should be deallocated by 'delete[]', not 'delete'}} // expected-note@-1 {{Memory allocated by 'new[]' should be deallocated by 'delete[]', not 'delete'}} } -// CHECK: diagnostics -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: path -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: kindcontrol -// CHECK-NEXT: edges -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: start -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line6 -// CHECK-NEXT: col3 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line6 -// CHECK-NEXT: col5 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: end -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line6 -// CHECK-NEXT: col12 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line6 -// CHECK-NEXT: col14 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: kindevent -// CHECK-NEXT: location +// CHECK: diagnostics +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: path +// CHECK-NEXT: // CHECK-NEXT: -// CHECK-NEXT: line6 -// CHECK-NEXT: col12 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: ranges -// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges // CHECK-NEXT: // CHECK-NEXT: -// CHECK-NEXT: line6 -// CHECK-NEXT: col12 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line6 -// CHECK-NEXT: col21 -// CHECK-NEXT: file0 +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line7 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line7 +// CHECK-NEXT: col5 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line7 +// CHECK-NEXT: col12 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line7 +// CHECK-NEXT: col14 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: // CHECK-NEXT: // CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: depth0 -// CHECK-NEXT: extended_message -// CHECK-NEXT: Memory is allocated -// CHECK-NEXT: message -// CHECK-NEXT: Memory is allocated -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: kindcontrol -// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line7 +// CHECK-NEXT: col12 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges // CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: start -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line6 -// CHECK-NEXT: col12 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line6 -// CHECK-NEXT: col14 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: end -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line8 -// CHECK-NEXT: col3 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line8 -// CHECK-NEXT: col8 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line7 +// CHECK-NEXT: col12 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line7 +// CHECK-NEXT: col21 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: // CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: kindevent -// CHECK-NEXT: location -// CHECK-NEXT: -// CHECK-NEXT: line8 -// CHECK-NEXT: col3 -// CHECK-NEXT: file0 +// CHECK-NEXT: depth0 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Memory is allocated +// CHECK-NEXT: message +// CHECK-NEXT: Memory is allocated // CHECK-NEXT: -// CHECK-NEXT: ranges -// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges // CHECK-NEXT: // CHECK-NEXT: -// CHECK-NEXT: line8 -// CHECK-NEXT: col10 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: line8 -// CHECK-NEXT: col10 -// CHECK-NEXT: file0 +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line7 +// CHECK-NEXT: col12 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line7 +// CHECK-NEXT: col14 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line10 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line10 +// CHECK-NEXT: col8 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: // CHECK-NEXT: // CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: depth0 -// CHECK-NEXT: extended_message -// CHECK-NEXT: Memory allocated by 'new[]' should be deallocated by 'delete[]', not 'delete' -// CHECK-NEXT: message -// CHECK-NEXT: Memory allocated by 'new[]' should be deallocated by 'delete[]', not 'delete' -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: descriptionMemory allocated by 'new[]' should be deallocated by 'delete[]', not 'delete' -// CHECK-NEXT: categoryMemory Error -// CHECK-NEXT: typeBad deallocator -// CHECK-NEXT: issue_context_kindfunction -// CHECK-NEXT: issue_contexttest -// CHECK-NEXT: issue_hash3 -// CHECK-NEXT: location -// CHECK-NEXT: -// CHECK-NEXT: line8 -// CHECK-NEXT: col3 -// CHECK-NEXT: file0 -// CHECK-NEXT: -// CHECK-NEXT: -// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line10 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line10 +// CHECK-NEXT: col10 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line10 +// CHECK-NEXT: col10 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: depth0 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Memory allocated by 'new[]' should be deallocated by 'delete[]', not 'delete' +// CHECK-NEXT: message +// CHECK-NEXT: Memory allocated by 'new[]' should be deallocated by 'delete[]', not 'delete' +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: descriptionMemory allocated by 'new[]' should be deallocated by 'delete[]', not 'delete' +// CHECK-NEXT: categoryMemory Error +// CHECK-NEXT: typeBad deallocator +// CHECK-NEXT: issue_context_kindfunction +// CHECK-NEXT: issue_contexttest +// CHECK-NEXT: issue_hash4 +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line10 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: diff --git a/test/Analysis/malloc.c b/test/Analysis/malloc.c index fae3a4dddb..6071d1d435 100644 --- a/test/Analysis/malloc.c +++ b/test/Analysis/malloc.c @@ -1107,7 +1107,7 @@ void testOffsetOfRegionFreedAfterFunctionCall() { int *p = malloc(sizeof(int)*2); p += 1; myfoo(p); - free(p); // no-warning + free(p); // expected-warning{{Argument to free() is offset by 4 bytes from the start of memory allocated by malloc()}} } void testFixManipulatedPointerBeforeFree() {