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

[Jordan Rose]

Hi, Brandon. It's a good idea, but unfortunately it has some problems. In C, it's totally legal to do this:

> int *allocate(size_t size) {

>   int *memoryBlock = (int *)malloc(size + sizeof(int));
>   *memoryBlock = SECRET_CODE;
>   return &memoryBlock[1];
> }

> void deallocate(int *memoryBlock) {
>   assert(memoryBlock[-1] == SECRET_CODE);
>   free(&memoryBlock[-1]);
> }


I'm not sure of the best way to solve this. IIRC, by default the region '*memoryBlock' will be a SymbolicRegion backed by a RegionValueSymbol, but if 'deallocate' has been inlined the backing symbol could be a DerivedRegionSymbol or a ConjuredSymbol instead. So it'd be very hard to differentiate these cases without actually seeing the call to allocate().

What you could try is seeing if the base region already has malloc information. That will miss some true bugs, but it should also drastically lower the rate of false positives, since we'll only be warning about regions we know can be freed.

As far as the patch itself, your logic seems reasonable, but your style doesn't match the rest of the file or the LLVM Coding Standards. In particular, please put a space after 'if', put operators at the end of the previous line instead of the start of the next line, and fit your lines in 80 columns. I'd also prefer '!offset.hasSymbolicOffset()' over 'offset.hasSymbolicOffset() == false'.

You'll probably want more test cases: cases where the input parameter does not come from malloc but has an offset, and at least one case where the input parameter comes from outside the function and has an offset.

Thanks for working on this!
Jordan



On Dec 15, 2012, at 21:58 , Branden Archer <b.m.archer4 at gmail.com> wrote:

> I have recently started looking into clang, and was interested in participating. After taking a look at the potential projects, the static checking functionality seemed interesting. Specifically, I have taken a look at the checker "memory.LeakPtrValChanged" mentioned on the list of potential checkers page.
> 
> Warning: As this is my first attempt at hacking clang, I may have gone a different route than someone with more experience in the project. If something in my description or patch seems out of place, please let me know!
> 
> From the description, the proposed memory.LeakPtrValChanged checker was to only consider a pointer to newly allocated data losing its original value. Through some investigation, I find that MemRegion objects which track pointers to memory allocations can also maintain any offset currently applied to the pointer. Using this information, the checker can reason about invalidated pointers beyond being 'newly allocated'. For example, the following case can be caught:
> 
> int * x = malloc(sizeof(int));
> x += 1;
> free(x);
> 
> However, the following is valid:
> 
> int * x = malloc(sizeof(int));
> x += 1;
> free(x-1);
> 
> The attached patch uses the RegionOffset of freed malloc allocations to determine if the freed pointer has a non-zero offset, and post a warning in this case. If the offset is symbolic (and thus not known to be non-zero), no warning is posted. There are tests included to verify the proposed changes. 
> 
> Note that memory.LeakPtrValChanged mentioned checking both malloc/free and new/delete, but this patch only considers malloc/free.
> 
> Please let me know if the attached patch is appropriate, or if it is missing something or there is another solution which may be a better fit.
> 
> - Branden
> 
> <leakPtrValChanged.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121217/58d51758/attachment.html>