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

[Anna Zaks]

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/20130207/b05d56a6/attachment.html>