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

[Branden Archer]

The attached patch removes memory.LeakPtrValChanged from the potential
checker list.

On Fri, Feb 8, 2013 at 11:57 AM, Anna Zaks <ganna at apple.com> wrote:

>
> On Feb 8, 2013, at 6:13 AM, Branden Archer <b.m.archer4 at gmail.com> wrote:
>
> Anna,
>
> Thanks for committing these!
>
> Who should I contact to have the LeakPtrValChanged section of the
> following page updated/removed, as the checker is now in?
> http://clang-analyzer.llvm.org/potential_checkers.html
>
> Just send in a patch. The html page is in the clang repository.
>
> Anna.
>
> - Branden
>
> On Thu, Feb 7, 2013 at 6:09 PM, Anna Zaks <ganna at apple.com> wrote:
>
>> Branden,
>>
>> Thanks for working on this and polishing it up! I've committed the
>> changes in r174677 and r 174678.
>>
>> I had to change the tests from the first patch slightly since we've made
>> analyzer more pessimistic as a quick fix to a regression in r174468. This
>> resulted in the expected leaks not being reported.
>>
>> Cheers,
>> Anna.
>> On Feb 3, 2013, at 8:10 PM, Branden Archer <b.m.archer4 at gmail.com> wrote:
>>
>> Here is the commit message for the first patch:
>> [analyzer] Pass pointer escape type to checkPointerEscape checker
>>
>> The checkPointerEscape checker previously did not specify how a
>> pointer escaped. This change includes an enum which describes the
>> different ways a pointer may escape. This enum is passed to the
>> checkPointerEscape checker when a pointer escapes. If the escape
>> is due to a function call, the call is passed. This changes
>> previous behavior where the call is passed as NULL if the escape
>> was due to indirectly invalidating the region the pointer referenced.
>>
>> Here is the commit message for the second patch:
>> [analyzer] malloc checker reports bugs when freeing memory with offset
>> pointer
>>
>> The malloc checker will now catch the case when a previously malloc'ed
>> region is freed, but the pointer passed to free does not point to the
>> start of the allocated memory. For example:
>>
>> int *p1 = malloc(sizeof(int));
>> p1++;
>> free(p1); // warn
>>
>> From the "memory.LeakPtrValChanged enhancement to unix.Malloc" entry
>> in the list of potential checkers.
>>
>>
>> What I meant is that the tests should not pass before the patch is
>>> applied. Then, after you apply the patch, the tests should start passing.
>>>
>>
>> Thanks for the reminder. The other tests for the second patch checked
>> out, but what I was trying for the first patch was not. I decided to take a
>> more direct approach to testing the first patch. Instead of trying to
>> figure out a system header function that would take a struct containing a
>> pointer (or for the simple stream checker, a FILE pointer), I decided to
>> 'create my own' by adding a fake system header function in the header
>> included in the test files. In doing so, I can easily see that previous to
>> the change there was a false negative, but after the change a bug was found.
>>
>> - Branden
>>
>> On Fri, Feb 1, 2013 at 1:20 PM, Anna Zaks <ganna at apple.com> wrote:
>>
>>>
>>> On Jan 31, 2013, at 7:20 PM, Branden Archer <b.m.archer4 at gmail.com>
>>> wrote:
>>>
>>> Oh, one other thing. I do not know who does the committing when the
>>> patch is approved. Should I prove a patch that has my commit message in it
>>> and username? Would either of you be pushing the change, or would I be
>>> pushing it?
>>>
>>>
>>> One of us will push this and mention that the patch is by you in the
>>> commit message. Please do send the commit message!
>>>
>>> Also, thanks for your comments and discussion on this patch, and for
>>> keeping with it for so long! I appreciate your feedback and the opportunity
>>> to learn some about the static analyzer of clang.
>>>
>>> - Branden
>>>
>>> On Thu, Jan 31, 2013 at 10:10 PM, Branden Archer <b.m.archer4 at gmail.com>wrote:
>>>
>>>> Have you double checked that the tests did not generate the warnings
>>>>> before the patch?
>>>>>
>>>>
>>>> When you mentioned this, I had a moment of doubt. I checked for
>>>> compiler warnings and the 'make test' before I emailed my patches, and all
>>>> came back clean. Just to be sure I updated my source and compiled again,
>>>> all still clean. Did you see something in particular, or just wanted to
>>>> make sure?
>>>>
>>>>
>>> What I meant is that the tests should not pass before the patch is
>>> applied. Then, after you apply the patch, the tests should start passing.
>>>
>>>
>>>> +void testPassConstPointerIndirectly() {
>>>>> +  struct HasPtr hp;
>>>>> +  hp.p = fopen("myfile.txt", "w");
>>>>> +  fputc(0, (FILE *)&hp);
>>>>> +  return; // expected-warning {{Opened file is never closed;
>>>>> potential resource leak}}
>>>>> +}
>>>>>
>>>>> Heh. Did you really want this test case? It's not actually valid (&hp
>>>>> is a FILE**, not a FILE*):
>>>>>
>>>>
>>>> I knew the test was not proper code, but it was the only way I could
>>>> think of to pass a structure to a known library function that was known to
>>>> not close a file. I replaced it in the attached patches with passing the
>>>> HasPtr structure to a function that accepts it as a const parameter. I am
>>>> not sure this still tests the same thing (as I do not fully appreciate how
>>>> the analyzer knows that a function will not close the stream. I am hoping
>>>> that if the parameter is passed as a const that it will assume this).
>>>>
>>>> - Branden
>>>>
>>>>
>>>> On Thu, Jan 31, 2013 at 8:26 PM, Jordan Rose <jordan_rose at apple.com>wrote:
>>>>
>>>>>
>>>>> On Jan 31, 2013, at 5:21 , Branden Archer <b.m.archer4 at gmail.com>
>>>>> wrote:
>>>>>
>>>>> Basically, you need to pass a pointer which we are tracking to a
>>>>>> function call indirectly (ex: as a field in a struct..). You should pass it
>>>>>> to a function which is known not to free memory or close stream. Finally,
>>>>>> you leak that resource/pointer.
>>>>>>
>>>>>> Previously, we would have a false negative - no leak would be
>>>>>> reported. Now, we should be catching the leak.
>>>>>>
>>>>>
>>>>> Ah, got you. See the first attached patch for these added cases.
>>>>>
>>>>> - Branden
>>>>>
>>>>>
>>>>> +void testPassConstPointerIndirectly() {
>>>>> +  struct HasPtr hp;
>>>>> +  hp.p = fopen("myfile.txt", "w");
>>>>> +  fputc(0, (FILE *)&hp);
>>>>> +  return; // expected-warning {{Opened file is never closed;
>>>>> potential resource leak}}
>>>>> +}
>>>>>
>>>>> Heh. Did you really want this test case? It's not actually valid (&hp
>>>>> is a FILE**, not a FILE*):
>>>>>
>>>>>
>>>>> A few remaining comments for the MallocChecker patch:
>>>>>
>>>>> +  if (ExplodedNode *N = C.generateSink()) {
>>>>>
>>>>> Please use an early return here.
>>>>>
>>>>>
>>>>> +    int offsetBytes =
>>>>> Offset.getOffset()/C.getASTContext().getCharWidth();
>>>>>
>>>>> *Very* nitpicky, but can you put spaces around the /?
>>>>>
>>>>>
>>>>> +       << ((abs(offsetBytes) > 1) ? "bytes" : "byte")
>>>>>
>>>>> Perfect!
>>>>>
>>>>> Jordan
>>>>>
>>>>
>>>>
>>>
>>>
>> <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/20130208/9e65442c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01-remove-from-list.patch
Type: application/octet-stream
Size: 955 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130208/9e65442c/attachment.obj>