From: Anna Zaks Date: Thu, 3 May 2012 23:50:28 +0000 (+0000) Subject: [analyzer] Allow pointers escape through calls containing callback args. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=aca0ac58d2ae80d764e3832456667d7322445e0c;p=clang [analyzer] Allow pointers escape through calls containing callback args. (Since we don't have a generic pointer escape callback, modify ExprEngineCallAndReturn as well as the malloc checker.) git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@156134 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h index d8aec0991d..0b260e4879 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h @@ -164,6 +164,9 @@ class CallOrObjCMessage { ObjCMessage Msg; ProgramStateRef State; const LocationContext *LCtx; + + bool isCallbackArg(unsigned Idx, const Type *T) const; + public: CallOrObjCMessage(const CallExpr *callE, ProgramStateRef state, const LocationContext *lctx) @@ -258,6 +261,10 @@ public: return Msg.getReceiverSourceRange(); } + /// \brief Check if one of the arguments might be a callback. + bool hasNonZeroCallbackArg() const; + + /// \brief Check if the name corresponds to a CoreFoundation or CoreGraphics /// function that allows objects to escape. /// @@ -273,18 +280,7 @@ public: // // TODO: To reduce false negatives here, we should track the container // allocation site and check if a proper deallocator was set there. - static bool isCFCGAllowingEscape(StringRef FName) { - if (FName[0] == 'C' && (FName[1] == 'F' || FName[1] == 'G')) - if (StrInStrNoCase(FName, "InsertValue") != StringRef::npos|| - StrInStrNoCase(FName, "AddValue") != StringRef::npos || - StrInStrNoCase(FName, "SetValue") != StringRef::npos || - StrInStrNoCase(FName, "WithData") != StringRef::npos || - StrInStrNoCase(FName, "AppendValue") != StringRef::npos|| - StrInStrNoCase(FName, "SetAttribute") != StringRef::npos) { - return true; - } - return false; - } + static bool isCFCGAllowingEscape(StringRef FName); }; } diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 55c32ec1eb..1415184033 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -1285,6 +1285,11 @@ bool MallocChecker::doesNotFreeMemory(const CallOrObjCMessage *Call, if (FName.startswith("NS") && (FName.find("Insert") != StringRef::npos)) return false; + // If the call has a callback as an argument, assume the memory + // can be freed. + if (Call->hasNonZeroCallbackArg()) + return false; + // Otherwise, assume that the function does not free memory. // Most system calls, do not free the memory. return true; @@ -1312,6 +1317,11 @@ bool MallocChecker::doesNotFreeMemory(const CallOrObjCMessage *Call, return false; } + // If the call has a callback as an argument, assume the memory + // can be freed. + if (Call->hasNonZeroCallbackArg()) + return false; + // Otherwise, assume that the function does not free memory. // Most system calls, do not free the memory. return true; diff --git a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 4d0da06427..963dc90cdf 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -323,12 +323,14 @@ static void findPtrToConstParams(llvm::SmallSet &PreserveArgs, // allocators/deallocators upon container construction. // - NSXXInsertXX, for example NSMapInsertIfAbsent, since they can // be deallocated by NSMapRemove. + // - Any call that has a callback as one of the arguments. if (FName == "pthread_setspecific" || FName == "funopen" || FName.endswith("NoCopy") || (FName.startswith("NS") && (FName.find("Insert") != StringRef::npos)) || - Call.isCFCGAllowingEscape(FName)) + Call.isCFCGAllowingEscape(FName) || + Call.hasNonZeroCallbackArg()) return; } @@ -344,6 +346,10 @@ static void findPtrToConstParams(llvm::SmallSet &PreserveArgs, if (const ObjCMethodDecl *MDecl = dyn_cast(CallDecl)) { assert(MDecl->param_size() <= Call.getNumArgs()); unsigned Idx = 0; + + if (Call.hasNonZeroCallbackArg()) + return; + for (clang::ObjCMethodDecl::param_const_iterator I = MDecl->param_begin(), E = MDecl->param_end(); I != E; ++I, ++Idx) { if (isPointerToConst(*I)) diff --git a/lib/StaticAnalyzer/Core/ObjCMessage.cpp b/lib/StaticAnalyzer/Core/ObjCMessage.cpp index 65cdcd9d99..dc24e818ff 100644 --- a/lib/StaticAnalyzer/Core/ObjCMessage.cpp +++ b/lib/StaticAnalyzer/Core/ObjCMessage.cpp @@ -88,3 +88,69 @@ const Decl *CallOrObjCMessage::getDecl() const { return 0; } +bool CallOrObjCMessage::isCallbackArg(unsigned Idx, const Type *T) const { + // Should we dig into struct fields, arrays ect? + if (T->isBlockPointerType() || T->isFunctionPointerType()) + if (!getArgSVal(Idx).isZeroConstant()) + return true; + return false; +} + +bool CallOrObjCMessage::hasNonZeroCallbackArg() const { + unsigned NumOfArgs = getNumArgs(); + + // Process ObjC message first. + if (!CallE) { + const ObjCMethodDecl *D = Msg.getMethodDecl(); + unsigned Idx = 0; + for (ObjCMethodDecl::param_const_iterator I = D->param_begin(), + E = D->param_end(); I != E; ++I, ++Idx) { + if (NumOfArgs <= Idx) + break; + + if (isCallbackArg(Idx, (*I)->getType().getTypePtr())) + return true; + } + return false; + } + + // Else, assume we are dealing with a Function call. + const FunctionDecl *FD = 0; + if (const CXXConstructExpr *Ctor = + CallE.dyn_cast()) + FD = Ctor->getConstructor(); + + const CallExpr * CE = CallE.get(); + FD = dyn_cast(CE->getCalleeDecl()); + + // If calling using a function pointer, assume the function does not + // have a callback. TODO: We could check the types of the arguments here. + if (!FD) + return false; + + unsigned Idx = 0; + for (FunctionDecl::param_const_iterator I = FD->param_begin(), + E = FD->param_end(); I != E; ++I, ++Idx) { + if (NumOfArgs <= Idx) + break; + + if (isCallbackArg(Idx, (*I)->getType().getTypePtr())) + return true; + } + return false; +} + +bool CallOrObjCMessage::isCFCGAllowingEscape(StringRef FName) { + if (FName[0] == 'C' && (FName[1] == 'F' || FName[1] == 'G')) + if (StrInStrNoCase(FName, "InsertValue") != StringRef::npos|| + StrInStrNoCase(FName, "AddValue") != StringRef::npos || + StrInStrNoCase(FName, "SetValue") != StringRef::npos || + StrInStrNoCase(FName, "WithData") != StringRef::npos || + StrInStrNoCase(FName, "AppendValue") != StringRef::npos|| + StrInStrNoCase(FName, "SetAttribute") != StringRef::npos) { + return true; + } + return false; +} + + diff --git a/test/Analysis/malloc.c b/test/Analysis/malloc.c index 9c09051c31..f5bff4fa43 100644 --- a/test/Analysis/malloc.c +++ b/test/Analysis/malloc.c @@ -798,6 +798,27 @@ void radar_11358224_test_double_assign_ints_positive_2() ptr = ptr; // expected-warning {{leak}} } +// Assume that functions which take a function pointer can free memory even if +// they are defined in system headers and take the const pointer to the +// allocated memory. (radar://11160612) +int const_ptr_and_callback(int, const char*, int n, void(*)(void*)); +void r11160612_1() { + char *x = malloc(12); + const_ptr_and_callback(0, x, 12, free); // no - warning +} + +// Null is passed as callback. +void r11160612_2() { + char *x = malloc(12); + const_ptr_and_callback(0, x, 12, 0); // expected-warning {{leak}} +} + +// Callback is passed to a function defined in a system header. +void r11160612_4() { + char *x = malloc(12); + sqlite3_bind_text_my(0, x, 12, free); // no - warning +} + // ---------------------------------------------------------------------------- // Below are the known false positives. diff --git a/test/Analysis/malloc.cpp b/test/Analysis/malloc.cpp index 8f80b2b76f..f36d8fcb7b 100644 --- a/test/Analysis/malloc.cpp +++ b/test/Analysis/malloc.cpp @@ -14,3 +14,13 @@ struct Foo { Foo aFunction() { return malloc(10); } + +// Assume that functions which take a function pointer can free memory even if +// they are defined in system headers and take the const pointer to the +// allocated memory. (radar://11160612) +// Test default parameter. +int const_ptr_and_callback_def_param(int, const char*, int n, void(*)(void*) = 0); +void r11160612_3() { + char *x = (char*)malloc(12); + const_ptr_and_callback_def_param(0, x, 12); +} diff --git a/test/Analysis/malloc.m b/test/Analysis/malloc.m index 6c94118286..08206f3bf1 100644 --- a/test/Analysis/malloc.m +++ b/test/Analysis/malloc.m @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc -analyzer-store=region -verify -Wno-objc-root-class %s +// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc -analyzer-store=region -verify -Wno-objc-root-class -fblocks %s #include "system-header-simulator-objc.h" @class NSString; diff --git a/test/Analysis/malloc.mm b/test/Analysis/malloc.mm index 3515a4f99a..ef1adda5a1 100644 --- a/test/Analysis/malloc.mm +++ b/test/Analysis/malloc.mm @@ -153,4 +153,29 @@ static void releaseDataCallback (void *info, const void *data, size_t size) { void testCGDataProviderCreateWithData() { void* b = calloc(8, 8); CGDataProviderRef p = CGDataProviderCreateWithData(0, b, 8*8, releaseDataCallback); +} + +// Assume that functions which take a function pointer can free memory even if +// they are defined in system headers and take the const pointer to the +// allocated memory. (radar://11160612) +extern CGDataProviderRef UnknownFunWithCallback(void *info, + const void *data, size_t size, + CGDataProviderReleaseDataCallback releaseData) + __attribute__((visibility("default"))); +void testUnknownFunWithCallBack() { + void* b = calloc(8, 8); + CGDataProviderRef p = UnknownFunWithCallback(0, b, 8*8, releaseDataCallback); +} + +// Test blocks. +void acceptBlockParam(void *, void (^block)(void *), unsigned); +void testCallWithBlockCallback() { + void *l = malloc(12); + acceptBlockParam(l, ^(void *i) { free(i); }, sizeof(char *)); +} + +// Test blocks in system headers. +void testCallWithBlockCallbackInSystem() { + void *l = malloc(12); + SystemHeaderFunctionWithBlockParam(l, ^(void *i) { free(i); }, sizeof(char *)); } \ No newline at end of file diff --git a/test/Analysis/system-header-simulator-objc.h b/test/Analysis/system-header-simulator-objc.h index 92d5899abf..4626a4ec4f 100644 --- a/test/Analysis/system-header-simulator-objc.h +++ b/test/Analysis/system-header-simulator-objc.h @@ -112,3 +112,5 @@ extern void CFRelease(CFTypeRef cf); extern CFMutableStringRef CFStringCreateMutableWithExternalCharactersNoCopy(CFAllocatorRef alloc, UniChar *chars, CFIndex numChars, CFIndex capacity, CFAllocatorRef externalCharactersAllocator); extern CFStringRef CFStringCreateWithCStringNoCopy(CFAllocatorRef alloc, const char *cStr, CFStringEncoding encoding, CFAllocatorRef contentsDeallocator); extern void CFStringAppend(CFMutableStringRef theString, CFStringRef appendedString); + +void SystemHeaderFunctionWithBlockParam(void *, void (^block)(void *), unsigned); diff --git a/test/Analysis/system-header-simulator.h b/test/Analysis/system-header-simulator.h index 6212131071..c910ad9694 100644 --- a/test/Analysis/system-header-simulator.h +++ b/test/Analysis/system-header-simulator.h @@ -36,3 +36,4 @@ FILE *funopen(const void *, fpos_t (*)(void *, fpos_t, int), int (*)(void *)); +int sqlite3_bind_text_my(int, const char*, int n, void(*)(void*));