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

[Branden Archer]

>
> 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?

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