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

[Jordan Rose]

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

> 
> On Jan 2, 2013, at 2:37 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> 
>> 
>> 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;
> 
> What's not right?
> 
>> in general, an indirect invalidation can result in a freed pointer.
> 
> Correct. That is what is happening here - the general logic is triggered.
> 
>> It's 'free' that's special here, not the indirect access.
> 
> Free is special but neither the MallocChecker nor the callback are dealing with it.
> 
> - The callback could pass non-NULL Call and a special flag saying that the invalidation is not direct.
> - The malloc could check if Call is to 'free' and never act upon a pointer escape if it is. This could probably be generalized for other functions we know about. Ex, it's not malloc checker or 'free' specific - we always know that 'free' could only touch the argument. It might be better to find a more general solution for this. 
> 
>> And...we should already be handling that with isMemFunction, right?

Hm. You're right that the knowledge about "free" here isn't specific to MallocChecker—what we want to say is that 'free' doesn't cause an invalidation of its indirect regions. (This is not generally true, of course. I don't think it's even generally true for system functions.) I guess this is a problem with not using evalCall, which promises to handle all aspects of a call. However, shouldn't the invalidation still happen after the pre-call check, even now?

Jordan


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