[cfe-commits] unix.Malloc static checker improvement: memory.LeakPtrValChanged

[Anna Zaks]

On Jan 2, 2013, at 11:22 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Dec 27, 2012, at 14:31 , Branden Archer <b.m.archer4 at gmail.com> wrote:
> 
>> Jordan,
>> 
>> Your explanation makes sense. It seems that the only piece missing from my previous patch is checking if the data is actually data know to come from malloc (or some other allocating function); if so and the region has an offset, then report the problem.
>> 
>> I find that there is something in the malloc checker which removes the symbol from a previous malloc from the RegionState map. Specifically, the checkPointerEscape callback. This was not used when I submitted my original patch, but was added since
> 
> Yep, this has been recently added to handle a common pattern across checkers. Hopefully it just simplifies things and doesn't make other things more complicated.
> 
> 
>> It seems that this callback is invoked just before an invalidated symbol is passed to free. For example, the following results in a checkPointerEscape callback:
>> 
>> {
>> int * data = malloc(sizeof(int));
>> data += 1;
>> free(data);
>> }
>> 
>> However, If the correct pointer is given to free even if it is manipulated, checkPointerEscape is not called:
>> 
>> {
>> int * array = malloc(sizeof(int)*5);
>> array += 1;
>> free(&array[-1]);
>> }
>> 
>> If checkPointerEscape removes the malloc information of an offending symbol from the RegionState map, then there is nothing to check inside of the FreeMemAux function called by the checkPostStmt callback. However, maybe it would be best to post a BugReport when the symbol is invalided, as that when the bug occurs. Do you know of anything the checkPointerEscape callback may catch that should not result in a BugReport if a malloc'ed symbol that is currently allocated is being invalidated?
> 
> That sounds like a bug; checkPointerEscape should be called in both cases. (CC-ing Anna, who designed checkPointerEscape.) However, note that the checkPointerEscape callback does check to make sure that the function being called isn't free-like or malloc-like, so it shouldn't actually be causing any ill effects here. This is because it's called for any function call (see below).
> 

I debugged the examples. The callback is called in both cases.

In the second case, we correctly determine that the pointer is being freed.

In the first case, we are not as precise as we could be. We decide that the pointer escapes, when 'data' is passed to 'free'. This is part of general pessimistic logic, which decides that everything accessible though a pointer passed to a function can escape. In this case, we could have added more logic to assume that 'free' does not invalidate any pointers. In order to do this, we would need to extend the pointerEscapes callback with a bool that let's us know if the invalidation is direct or not. Currently, we always set Call to NULL, when the invalidation is indirect.

> 
>> One more question. I notice that checkPointerEscape callback does not pass a CheckerContext, unlike the other checkers. Does this mean that a BugReport cannot be posted from within checkPointerEscape?
> 
> That is mostly correct. Unlike other callbacks, checkPointerEscape does not allow you to create new nodes in the analyzer's state exploration graph, only to update the state along the current path. That's the main reason there's no CheckerContext. But it's also not reasonable to report a bug here, both because you don't have an ExplodedNode to attach the bug to (to give good path notes) and because symbols escape all the time, for a variety of reasons, and I can't see how you could prove that a particular escape constitutes a bug.
> 
> (Note that escaping is not the same thing as leaking; in fact, they are almost opposites. Escaping means the value may have been stored into long-term memory of some kind, or that it may have been freed by a helper function in a library somewhere, and so the analyzer can never really prove that the value has leaked.)
> 
>> 
>> - Branden
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130102/73b841f4/attachment.html>