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

[Anna Zaks]

On Jan 10, 2013, at 8:57 PM, Branden Archer <b.m.archer4 at gmail.com> wrote:

> Anna,
> 
> I am looking in ExprEngine.cpp in the methods that call runCheckersForPointerEscape. Looking into adding an enum to the checkPointerEscape callbacks, there appears to be four cases:
>   - in processPointerEscapedOnBind there is only one: pointer is lost on a bind.
>   - in processPointerEscapedOnInvalidateRegions there are three. The last two are for direct and indirect invalidating due to some function call. The first one I do not quite get. Can you give an example of a region being invalidated that does not involve a function (e.g. Call is NULL). I am not clear what to call this case.
> 

Looks like the third case is never triggered. Thanks for finding this.

The ProgramState API allows checkers to trigger such events. (For example, if a checker invalidates a region and intends the invalidation to cause pointer escape event, like in CStringChecker::InvalidateBuffer,but with /*CausedPointerEscape*/ true). However, none of the checkers currently do this and I do not have a specific case where it would be used. 

Our options are : 
 - restrict the ProgramState::invalidateRegions not to allow such combination
 - leave the handling of the third case and allow the callers of ProgramState::invalidateRegions to set the reason for invalidation. We could add Unknown into enum.

I am leaning toward the second option.
Anna.

> - Branden
> 
> On Mon, Jan 7, 2013 at 9:08 PM, Anna Zaks <ganna at apple.com> wrote:
> 
> On Jan 7, 2013, at 5:51 PM, Branden Archer <b.m.archer4 at gmail.com> wrote:
> 
>> Anna & Jordan,
>> 
>> I have a better idea on what is being the checkPointerEscape callback. Thanks for your description.
>> 
>> The discussion below is about how we could special case it. Ex: check the callback to actually pass the Call parameter and use another argument to specify if the invalidation is direct (a pointer is an argument) or not. I am not sure how generic this solution is and how important it is to handle this. 
>> 
>> I cannot think of another case where knowing if an invalidation is direct or indirect would be important. I imagine that any checker monitoring a function that takes a pointer to ensure that the passed pointer was valid could make use of the information. This would beg the question of would it be worth writing such checkers, but that would be a separate issue. The callback could be made even more general, for example by passing an enum that represented the type of invalidation that has occurred. However, I do not know how much use something like that would be, especially if nothing else may use it.
>> 
>> For the purpose of modifying the malloc checker to detect pointers with non-zero offsets, would modifying the checkPointerEscape callback to pass a boolean such as isDirectInvalidation be acceptable?
> 
> Absolutely. An enum with more options would also be fine. 
> 
> What I meant by a generic solution is that it might be possible and better not to perform the indirect invalidation in cases we know it will not happen. For example, many calls to system APIs ('free' including) only invalidate the pointers that are being directly passed in. If we go with the proposed solution, the pointer will get invalidated, but we will keep tracking it in the malloc checker. However, it is possible not to invalidate the pointer altogether, which is the most precise. This would be a more intrusive solution and I am not 100% sure how it should be. One option would be to model all such functions. Another just add the special casing in the core as we do for pointer to const arguments. This solution is better and more generic but possibly more difficult to implement.
> 
>> There are only two checkers using the callback currently, so making the change would not be that extensive. I could prepare a patch with that change and another where the free error I am interested in is detecting, and submit both for review.
>> 
>> - Branden
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130111/9b359ab8/attachment.html>