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

[Jordan Rose]

On Jan 2, 2013, at 14:34 , Anna Zaks <ganna at apple.com> wrote:

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

That's not right; in general, an indirect invalidation can result in a freed pointer. It's 'free' that's special here, not the indirect access. And...we should already be handling that with isMemFunction, right?

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