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

[Branden Archer]

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