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

[Tobias Grosser via cfe-dev]

On Thu, Sep 8, 2016, at 06:26 AM, Devin Coughlin via cfe-dev wrote:
> Tobias,

Devin, this was fast. Thank you!
 
> > 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.

Thanks for confirming.
 
> > 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.

Great. I could make it work for my uses cases (see patch for reference).

> 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.

Not sure I know how to use the analyzer config flags yet. My patch
currently also still causes a segfault on Analysis/retain-release.m and
changes a test case, where I am not sure it should.

I probably need some time to get this tested and get more confidence in
what it is doing. I will then put it up for review and we can see how to
add an analyzer-config flag.

Best,
Tobias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-parameter-checking-in-Retaincounter.patch
Type: text/x-patch
Size: 3815 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160908/f70d1d7c/attachment.bin>