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

[Branden Archer]

Anna,

Thanks for the feedback.

I think I fixed all the coding style issues you pointed out. Let me know if
I missed any.

4) I think it does not make sense to peel the super region in ReportBadFree
> before reporting the new warning. The MR region is used for naming whatever
> is passed to free. It does not currently trigger since we are unable to
> print/summarize the region. Also, the loop can be replaced with MR =
> MR->getBaseRegion().
>

Do you mean that you do not see a reason to find the base region at all? I
think that the base region is found and used, as there are some decision
made about the type of bug which require it. I attempted to remove the loop
just to see what would happen, and I noticed that a few of the older unit
tests failed. I do agree with replacing the loop with the getBaseRegion
call to simplify things. As that change is unrelated to the other changes,
I have attached a third patch which handles it.

Also, thanks for the additional test case.

- Branden

On Wed, Jan 16, 2013 at 3:20 PM, Anna Zaks <ganna at apple.com> wrote:

> Branden,
>
> Thanks for working on this! Everything looks good in general. Here are
> some minor issues.
>
> Patch 01:
> 1) There are several occurrences of 80 column rule violations. For
> example, the definition of "ProgramStateRefMallocChecker::checkPointerEscape
> ".
> 2) Make the style of malloc test cases consistent :
> int *testOffsetAllocate(size_t size)
> {
> ->
> int *testOffsetAllocate(size_t size) {
>
> 3) The spacing in the following if statement looks odd (should be no space
> between '(' and 'offset':
>         if (  offset.isValid()
>
> 4) I think it does not make sense to peel the super region in
> ReportBadFree before reporting the new warning. The MR region is used for
> naming whatever is passed to free. It does not currently trigger since we
> are unable to print/summarize the region. Also, the loop can be replaced
> with MR = MR->getBaseRegion().
>
> 5)Modify the following comment to start with "The reason for pointer
> escape is unknown. For example,...".
>   /// A checker invalidates a region and intends the invalidation
>   /// to cause a pointer escape event.
>   PSK_InvalidatedRegionOther
>
> 6) Additional test case.
> void testOffsetZeroDoubleFree() {
>   int *array = malloc(sizeof(int)*2);
>   int *p = &array[0];
>   free(p);
>   free(&array[0]); // expected-warning{{Attempt to free released memory}}
> }
>
> Cheers,
> Anna.
>
> On Jan 15, 2013, at 6:32 PM, Branden Archer <b.m.archer4 at gmail.com> wrote:
>
> 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
>>>
>>>
>>>
>>
>>
> <01-modify-checkPointerEscape.patch><02-update-malloc-checker.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/f5ea3d37/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01-modify-checkPointerEscape.patch
Type: application/octet-stream
Size: 10308 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/f5ea3d37/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02-update-malloc-checker.patch
Type: application/octet-stream
Size: 4949 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/f5ea3d37/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 03-use-getbaseregion.patch
Type: application/octet-stream
Size: 640 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/f5ea3d37/attachment-0002.obj>