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

[Branden Archer]

I agree with keeping the third option open in case a checker in the future
needs to invalidate a region.

Attached are two patches: one to add an enum to the checkPointerEscape
callback describing the type of pointer escape, and another to update the
malloc checker to catch the case when a malloc'ed pointer is free'd with a
pointer with an offset.

Please take a look at the patches and let me know if they are acceptable
and properly update the malloc checker to catch freeing pointers with an
offset. I have included several test cases in test/Analysis/malloc.c, but
if you can think of additional situations that I did not catch, let me know.

- Branden

On Fri, Jan 11, 2013 at 4:04 PM, Anna Zaks <ganna at apple.com> wrote:

>
> 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/20130115/c0d692a9/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01-modify-checkPointerEscape.patch
Type: application/octet-stream
Size: 9758 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130115/c0d692a9/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02-update-malloc-checker.patch
Type: application/octet-stream
Size: 4779 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130115/c0d692a9/attachment-0001.obj>