Some day we'll look inside callback blocks, too, but this is good. How about Cocoa methods with callback selectors, like -[NSOpenPanel beginSheet:modalForWindow:modalDelegate:didEndSelector:contextInfo:]? Then again, there's a fairly strong case against it at http://boredzo.org/blog/archives/2009-04-03/dont-pass-objects-as-context-pointers .
And we really do need a central place for pointer escape... Jordy On May 3, 2012, at 19:50, Anna Zaks wrote: > Author: zaks > Date: Thu May 3 18:50:28 2012 > New Revision: 156134 > > URL: http://llvm.org/viewvc/llvm-project?rev=156134&view=rev > Log: > [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.) > > Modified: > cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h > cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp > cfe/trunk/lib/StaticAnalyzer/Core/ObjCMessage.cpp > cfe/trunk/test/Analysis/malloc.c > cfe/trunk/test/Analysis/malloc.cpp > cfe/trunk/test/Analysis/malloc.m > cfe/trunk/test/Analysis/malloc.mm > cfe/trunk/test/Analysis/system-header-simulator-objc.h > cfe/trunk/test/Analysis/system-header-simulator.h > > Modified: > cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h > (original) > +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h > Thu May 3 18:50:28 2012 > @@ -164,6 +164,9 @@ > 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 @@ > 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 @@ > // > // 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); > }; > > } > > Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Thu May 3 > 18:50:28 2012 > @@ -1285,6 +1285,11 @@ > 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 @@ > 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; > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp Thu May 3 > 18:50:28 2012 > @@ -323,12 +323,14 @@ > // 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 @@ > if (const ObjCMethodDecl *MDecl = dyn_cast<ObjCMethodDecl>(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)) > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/ObjCMessage.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ObjCMessage.cpp?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/ObjCMessage.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/ObjCMessage.cpp Thu May 3 18:50:28 2012 > @@ -88,3 +88,69 @@ > 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<const CXXConstructExpr *>()) > + FD = Ctor->getConstructor(); > + > + const CallExpr * CE = CallE.get<const CallExpr *>(); > + FD = dyn_cast<FunctionDecl>(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; > +} > + > + > > Modified: cfe/trunk/test/Analysis/malloc.c > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.c?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/malloc.c (original) > +++ cfe/trunk/test/Analysis/malloc.c Thu May 3 18:50:28 2012 > @@ -798,6 +798,27 @@ > 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. > > > Modified: cfe/trunk/test/Analysis/malloc.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.cpp?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/malloc.cpp (original) > +++ cfe/trunk/test/Analysis/malloc.cpp Thu May 3 18:50:28 2012 > @@ -14,3 +14,13 @@ > 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); > +} > > Modified: cfe/trunk/test/Analysis/malloc.m > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.m?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/malloc.m (original) > +++ cfe/trunk/test/Analysis/malloc.m Thu May 3 18:50:28 2012 > @@ -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; > > Modified: cfe/trunk/test/Analysis/malloc.mm > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.mm?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/malloc.mm (original) > +++ cfe/trunk/test/Analysis/malloc.mm Thu May 3 18:50:28 2012 > @@ -153,4 +153,29 @@ > 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 > > Modified: cfe/trunk/test/Analysis/system-header-simulator-objc.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/system-header-simulator-objc.h?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/system-header-simulator-objc.h (original) > +++ cfe/trunk/test/Analysis/system-header-simulator-objc.h Thu May 3 > 18:50:28 2012 > @@ -112,3 +112,5 @@ > 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); > > Modified: cfe/trunk/test/Analysis/system-header-simulator.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/system-header-simulator.h?rev=156134&r1=156133&r2=156134&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/system-header-simulator.h (original) > +++ cfe/trunk/test/Analysis/system-header-simulator.h Thu May 3 18:50:28 2012 > @@ -36,3 +36,4 @@ > fpos_t (*)(void *, fpos_t, int), > int (*)(void *)); > > +int sqlite3_bind_text_my(int, const char*, int n, void(*)(void*)); > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
