[cfe-commits] r150846 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/CStringChecker.cpp lib/StaticAnalyzer/Checkers/InterCheckerAPI.h lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/malloc.c

Wed Feb 22 09:36:06 PST 2012

On Feb 18, 2012, at 5:34 AM, Jordy Rose wrote:

> Does MallocChecker really "need" to know this? I mean, I doubt CStringCheckerBasic is a huge performance hit, but it seems like someone who wants the analyzer to model C library string functions should enable CStringChecker themselves.

Malloc Checker directly depends on the evaluation of the CString functions. In particular, it needs to know that when strcpy is evaluated, the return value is equal to the input parameter. You can see the malloc false positive fixed by this commit at the very end of the diff.

> 
> (I see that someone on Radar thought the answer was "yes", but...)
> 
> Also, separately...is this the way we're going to do inter-checker dependencies? I like how simple it is! And since all the checker registration is done with vectors, it even handles ordering.

I don't think this handles ordering.. The ordering still depends on the order in which the checkers are defined in the Checker.td file. That's something which needs to be fixed. 

> But I don't know how it will scale, and it does sort of compel us to expose a registration function for /every/ checker. (Just like warning flags, if we decide it's okay not to have a registration function for some checker, we'll end up in a situation where we need it.)
> 

We do not have the dependency mechanism set in stone. We can revisit this later on. I view this approach as something that we can use to define dependencies in special cases. Currently, this is the only dependency we have, though it might quickly change..

Anna.

> Jordy
> 
> 
> On Feb 18, 2012, at 5:35, Anna Zaks wrote:
> 
>> Author: zaks
>> Date: Fri Feb 17 16:35:31 2012
>> New Revision: 150846
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=150846&view=rev
>> Log:
>> [analyzer] Fix another false positive in the Malloc Checker, by making
>> it aware of CString APIs that return the input parameter.
>> 
>> Malloc Checker needs to know how the 'strcpy' function is
>> evaluated. Introduce the dependency on CStringChecker for that.
>> CStringChecker knows all about these APIs.
>> 
>> Addresses radar://10864450
>> 
>> Added:
>>  cfe/trunk/lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
>> Modified:
>>  cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
>>  cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>>  cfe/trunk/test/Analysis/malloc.c
>> 
>> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp?rev=150846&r1=150845&r2=150846&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp Fri Feb 17 16:35:31 2012
>> @@ -13,6 +13,7 @@
>> //===----------------------------------------------------------------------===//
>> 
>> #include "ClangSACheckers.h"
>> +#include "InterCheckerAPI.h"
>> #include "clang/StaticAnalyzer/Core/Checker.h"
>> #include "clang/StaticAnalyzer/Core/CheckerManager.h"
>> #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
>> @@ -1924,3 +1925,7 @@
>> REGISTER_CHECKER(CStringOutOfBounds)
>> REGISTER_CHECKER(CStringBufferOverlap)
>> REGISTER_CHECKER(CStringNotNullTerm)
>> +
>> +void ento::registerCStringCheckerBasic(CheckerManager &Mgr) {
>> +  registerCStringNullArg(Mgr);
>> +}
>> 
>> Added: cfe/trunk/lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/InterCheckerAPI.h?rev=150846&view=auto
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/InterCheckerAPI.h (added)
>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/InterCheckerAPI.h Fri Feb 17 16:35:31 2012
>> @@ -0,0 +1,22 @@
>> +//==--- InterCheckerAPI.h ---------------------------------------*- C++ -*-==//
>> +//
>> +//                     The LLVM Compiler Infrastructure
>> +//
>> +// This file is distributed under the University of Illinois Open Source
>> +// License. See LICENSE.TXT for details.
>> +//
>> +//===----------------------------------------------------------------------===//
>> +// This file allows introduction of checker dependencies. It contains APIs for
>> +// inter-checker communications.
>> +//===----------------------------------------------------------------------===//
>> +
>> +#ifndef INTERCHECKERAPI_H_
>> +#define INTERCHECKERAPI_H_
>> +namespace clang {
>> +namespace ento {
>> +
>> +/// Register the checker which evaluates CString API calls.
>> +void registerCStringCheckerBasic(CheckerManager &Mgr);
>> +
>> +}}
>> +#endif /* INTERCHECKERAPI_H_ */
>> 
>> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=150846&r1=150845&r2=150846&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Fri Feb 17 16:35:31 2012
>> @@ -13,6 +13,7 @@
>> //===----------------------------------------------------------------------===//
>> 
>> #include "ClangSACheckers.h"
>> +#include "InterCheckerAPI.h"
>> #include "clang/StaticAnalyzer/Core/Checker.h"
>> #include "clang/StaticAnalyzer/Core/CheckerManager.h"
>> #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
>> @@ -1130,6 +1131,7 @@
>> 
>> #define REGISTER_CHECKER(name) \
>> void ento::register##name(CheckerManager &mgr) {\
>> +  registerCStringCheckerBasic(mgr); \
>> mgr.registerChecker<MallocChecker>()->Filter.C##name = true;\
>> }
>> 
>> 
>> Modified: cfe/trunk/test/Analysis/malloc.c
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.c?rev=150846&r1=150845&r2=150846&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/malloc.c (original)
>> +++ cfe/trunk/test/Analysis/malloc.c Fri Feb 17 16:35:31 2012
>> @@ -594,6 +594,26 @@
>> strcpy(p, s); // expected-warning {{leak}}
>> }
>> 
>> +// Rely on the CString checker evaluation of the strcpy API to convey that the result of strcpy is equal to p.
>> +void symbolLostWithStrcpy(char *s) {
>> +  char *p = malloc(12);
>> +  p = strcpy(p, s);
>> +  free(p);
>> +}
>> +
>> +
>> +// The same test as the one above, but with what is actually generated on a mac.
>> +static __inline char *
>> +__inline_strcpy_chk (char *restrict __dest, const char *restrict __src)
>> +{
>> +  return __builtin___strcpy_chk (__dest, __src, __builtin_object_size (__dest, 2 > 1));
>> +}
>> +
>> +void symbolLostWithStrcpy_InlineStrcpyVersion(char *s) {
>> +  char *p = malloc(12);
>> +  p = ((__builtin_object_size (p, 0) != (size_t) -1) ? __builtin___strcpy_chk (p, s, __builtin_object_size (p, 2 > 1)) : __inline_strcpy_chk (p, s));
>> +  free(p);
>> +}
>> // Below are the known false positives.
>> 
>> // TODO: There should be no warning here. This one might be difficult to get rid of.
>> @@ -627,13 +647,6 @@
>> return p;// expected-warning {{Memory is never released; potential memory leak}}
>> }
>> 
>> -// TODO: This is a false positve that should be fixed by making CString checker smarter.
>> -void symbolLostWithStrcpy(char *s) {
>> -  char *p = malloc(12);
>> -  p = strcpy(p, s);
>> -  free(p);// expected-warning {{leak}}
>> -}
>> -
>> // False negatives.
>> 
>> // TODO: This requires tracking symbols stored inside the structs/arrays.
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 