From 374ae320b87c15b0262c40e5c46e8990111df5ca Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 10 May 2013 17:07:16 +0000 Subject: [PATCH] [analyzer] Indirect invalidation counts as an escape for leak checkers. Consider this example: char *p = malloc(sizeof(char)); systemFunction(&p); free(p); In this case, when we call systemFunction, we know (because it's a system function) that it won't free 'p'. However, we /don't/ know whether or not it will /change/ 'p', so the analyzer is forced to invalidate 'p', wiping out any bindings it contains. But now the malloc'd region looks like a leak, since there are no more bindings pointing to it, and we'll get a spurious leak warning. The fix for this is to notice when something is becoming inaccessible due to invalidation (i.e. an imperfect model, as opposed to being explicitly overwritten) and stop tracking it at that point. Currently, the best way to determine this for a call is the "indirect escape" pointer-escape kind. In practice, all the patch does is take the "system functions don't free memory" special case and limit it to direct parameters, i.e. just the arguments to a call and not other regions accessible to them. This is a conservative change that should only cause us to escape regions more eagerly, which means fewer leak warnings. This isn't perfect for several reasons, the main one being that this example is treated the same as the one above: char **p = malloc(sizeof(char *)); systemFunction(p + 1); // leak Currently, "addresses accessible by offsets of the starting region" and "addresses accessible through bindings of the starting region" are both considered "indirect" regions, hence this uniform treatment. Another issue is our longstanding problem of not distinguishing const and non-const bindings; if in the first example systemFunction's parameter were a char * const *, we should know that the function will not overwrite 'p', and thus we can safely report the leak. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@181607 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 3 +- .../Checkers/SimpleStreamChecker.cpp | 4 +- .../Analysis/Inputs/system-header-simulator.h | 3 +- test/Analysis/malloc.c | 42 +++++++++++++++---- test/Analysis/simple-stream-checks.c | 4 +- 5 files changed, 40 insertions(+), 16 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 5d3eb65148..11abe9a41b 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -2012,8 +2012,7 @@ ProgramStateRef MallocChecker::checkPointerEscapeAux(ProgramStateRef State, bool(*CheckRefState)(const RefState*)) const { // If we know that the call does not free memory, or we want to process the // call later, keep tracking the top level arguments. - if ((Kind == PSK_DirectEscapeOnCall || - Kind == PSK_IndirectEscapeOnCall) && + if (Kind == PSK_DirectEscapeOnCall && doesNotFreeMemOrInteresting(Call, State)) { return State; } diff --git a/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp b/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp index 1ccf339bac..c2786e184b 100644 --- a/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp @@ -259,9 +259,7 @@ SimpleStreamChecker::checkPointerEscape(ProgramStateRef State, const CallEvent *Call, PointerEscapeKind Kind) const { // If we know that the call cannot close a file, there is nothing to do. - if ((Kind == PSK_DirectEscapeOnCall || - Kind == PSK_IndirectEscapeOnCall) && - guaranteedNotToCloseFile(*Call)) { + if (Kind == PSK_DirectEscapeOnCall && guaranteedNotToCloseFile(*Call)) { return State; } diff --git a/test/Analysis/Inputs/system-header-simulator.h b/test/Analysis/Inputs/system-header-simulator.h index dd1cd4942f..ef9c1c7e51 100644 --- a/test/Analysis/Inputs/system-header-simulator.h +++ b/test/Analysis/Inputs/system-header-simulator.h @@ -77,8 +77,9 @@ void xpc_connection_set_context(xpc_connection_t connection, void *context); void xpc_connection_set_finalizer_f(xpc_connection_t connection, xpc_finalizer_t finalizer); void xpc_connection_resume(xpc_connection_t connection); -//The following is a fake system header function +//The following are fake system header functions for generic testing. void fakeSystemHeaderCallInt(int *); +void fakeSystemHeaderCallIntPtr(int **); typedef struct __SomeStruct { char * p; diff --git a/test/Analysis/malloc.c b/test/Analysis/malloc.c index 6071d1d435..76f59fda19 100644 --- a/test/Analysis/malloc.c +++ b/test/Analysis/malloc.c @@ -1057,12 +1057,6 @@ void testPassConstPointerIndirectly() { return; // expected-warning {{leak}} } -void testPassToSystemHeaderFunctionIndirectly() { - int *p = malloc(4); - p++; - fakeSystemHeaderCallInt(p); -} // expected-warning {{leak}} - void testPassConstPointerIndirectlyStruct() { struct HasPtr hp; hp.p = malloc(10); @@ -1073,8 +1067,32 @@ void testPassConstPointerIndirectlyStruct() { void testPassToSystemHeaderFunctionIndirectlyStruct() { SomeStruct ss; ss.p = malloc(1); - fakeSystemHeaderCall(&ss); -} // expected-warning {{Potential leak of memory pointed to by 'ss.p'}} + fakeSystemHeaderCall(&ss); // invalidates ss, making ss.p unreachable + // Technically a false negative here -- we know the system function won't free + // ss.p, but nothing else will either! +} // no-warning + +void testPassToSystemHeaderFunctionIndirectlyStructFree() { + SomeStruct ss; + ss.p = malloc(1); + fakeSystemHeaderCall(&ss); // invalidates ss, making ss.p unreachable + free(ss.p); +} // no-warning + +void testPassToSystemHeaderFunctionIndirectlyArray() { + int *p[1]; + p[0] = malloc(sizeof(int)); + fakeSystemHeaderCallIntPtr(p); // invalidates p, making p[0] unreachable + // Technically a false negative here -- we know the system function won't free + // p[0], but nothing else will either! +} // no-warning + +void testPassToSystemHeaderFunctionIndirectlyArrayFree() { + int *p[1]; + p[0] = malloc(sizeof(int)); + fakeSystemHeaderCallIntPtr(p); // invalidates p, making p[0] unreachable + free(p[0]); +} // no-warning int *testOffsetAllocate(size_t size) { int *memoryBlock = (int *)malloc(size + sizeof(int)); @@ -1200,3 +1218,11 @@ void testMallocWithParam(int **p) { void testMallocWithParam_2(int **p) { *p = (int*) malloc(sizeof(int)); // no-warning } + +void testPassToSystemHeaderFunctionIndirectly() { + int *p = malloc(4); + p++; + fakeSystemHeaderCallInt(p); + // FIXME: This is a leak: if we think a system function won't free p, it + // won't free (p-1) either. +} diff --git a/test/Analysis/simple-stream-checks.c b/test/Analysis/simple-stream-checks.c index ce57fa7ac3..2f725745a5 100644 --- a/test/Analysis/simple-stream-checks.c +++ b/test/Analysis/simple-stream-checks.c @@ -87,5 +87,5 @@ void testPassConstPointer() { void testPassToSystemHeaderFunctionIndirectly() { FileStruct fs; fs.p = fopen("myfile.txt", "w"); - fakeSystemHeaderCall(&fs); -} // expected-warning {{Opened file is never closed; potential resource leak}} + fakeSystemHeaderCall(&fs); // invalidates fs, making fs.p unreachable +} // no-warning -- 2.40.0