[cfe-dev] [Static Analyzer] Retain count checker does not warn about parameters that might leak

[Devin Coughlin via cfe-dev]

Tobias,

> On Sep 7, 2016, at 4:25 PM, Tobias Grosser via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> 
> Hi,
> 
> I have just been looking into using the clang analyzer retain counter to
> check the use of a reference counting library we use as part of the LLVM
> Polly project. This seems to work surprisingly well for now and most of
> the basic (and not so basic) errors are correctly found. However, it
> seems that function arguments are never considered as possibly leaking.
> Here a small example:
> 
> struct obj;                                                              
> typedef struct obj obj;                                                  
> 
> #define ___give __attribute__((cf_returns_retained))                     
> #define ___take __attribute__((cf_consumed))                             
> #define ___take                          
> 
> __give obj *alloc();
> __give obj *obj_copy(___keep obj *o);                                    
> void obj_free(___take obj *o);                                           
> 
> void error_reported() {                                                  
>  obj *o = alloc();                                                      
> }                                                                        
> 
> void leak_ignored(___take obj *o) {                                      
> } 
> 
> For the function error_reported, the memory leak is correctly warned
> about. However, for leak_ignored, no warning is issued. Is this behavior
> correct or should the checker report a memory leak here as well?

This behavior is not correct — ideally it would report a memory leak. My understanding is that this functionality just hasn't been added to the RetainCountChecker.

> For my use case, I would like that all function arguments passed with
> __take (consumed) are required to be freed in the function (or passed
> back as return value), whereas arguments passed as __keep (not consumed)
> would need to be copied before they can be used as return value or
> passed to any __isl_take function.
> 
> My feeling is that I can add this functionality easily by correctly
> initializing the state of function arguments. However, as a newcomer to
> the clang static analyzer I am not sure where to do this. Is
> checkBeginFunction the right location (I fail to even find a way to
> obtain the function we currently work on here) or are there more
> specialized callbacks that are called when each of the functions
> arguments is evaluated?
> 
> I probably need to dig deeper into this to make this happen, but as this
> is likely a pretty trivial problem for our static analysis experts, I
> wanted to check if anybody has some input or hints that might help me
> here?

There a couple of things to note:

1) The RetainCountChecker (which is the checker that understands the cf_consumed and cf_returns_retained attributes) was designed for checking a reference counting scheme that relies heavily on communicating ownership via the names of functions. The checker uses these naming rules to determine ownership convention. For example, if a function contains the word ‘Copy’ or ‘Create’ it is assumed to act like cf_returns_retained. Typically the attributes are only used when a function violates the naming conventions. There is more information about these naming conventions in the CoreFoundation memory management guide <https://developer.apple.com/library/mac/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html>.

2) As for where to start: I would take a look at the logic in void RetainCountChecker::checkSummary() that deals with the return value of a function call and apply it to parameters. You are right that checkBeginFunction() is a good place for this. You can take a look at void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) to see how another checker gets at the parameter values. Note that it is really important that this only be done when the function is at the top level — we don’t check retain counts for inlined functions.

3) Extending the RetainCountChecker to properly check parameters is something we’re definitely interested in — but you'll have to be a bit careful here because the checker is already widely used. It would be good to keep this extra checking behind an analyzer-config flag at first and off by default until it is fully evaluated.

Hope this helps!
Devin