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

[Anna Zaks]

On Jan 18, 2013, at 7:02 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> (see previous e-mail, "Re: Add PointerEscapeKind to checkPointerEscape callback")
> 
> I'd definitely like to see this improvement to MallocChecker! Let's see...
> 
> +
> +  const MemRegion *Rbase = R->getBaseRegion();
>    
> -  const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R);
> +  const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(Rbase);
> 
> I would just collapse these into one line. More importantly, I think you should rename SR and possibly Sym to indicate that they're the base region/symbol, which isn't the same as the region actually passed to free.
> 
> 
> +  // Check if the memory location being freed is the actual location
> +  // allocated, or an offset.
> +  RegionOffset offset = R->getAsOffset();
> 
> LLVM style guide is Germanic; all nouns are capitalized, even local variables.
> 
> 
> +  if ( RS && RS->isAllocated() &&
> 
> Conventionally we have no space after the open paren for an 'if'.
> 
> +       offset.isValid() &&
> +       offset.hasSymbolicOffset() == false &&
> 
> Conventionally written "!offset.hasSymbolicOffset()"
> 
> 
> +      // Get the offset into the memory region before
> +      // getting the super region, as retrieving the
> +      // super region will ignore the offset.
> +      RegionOffset offset = MR->getAsOffset();
> 
> Capital letter, again, but...
> 
> 
> +
> +        if (  offset.isValid()
> +           && !offset.hasSymbolicOffset()
> +           && offset.getOffset() != 0) {
> +          os << "; the memory location passed is an offset of an allocated location";
> +        }
> +
> 
> ...this test is not correct; it will fire for free(&local[1]). Given that the message isn't great either, and that you've already established what kind of value is being freed, I think you should just make a new helper function to report the error, ReportOffsetFree or something. Then you can make a nicer error message, like "Argument to free() is an offset of an allocation". (I don't think this message is perfect either -- it doesn't mention malloc(), "an offset" isn't really the correct term -- but at least it's more concise.)
> 
> Also, please put the && at the end of the line, not the start.
> 
> And while you're in there...
> 
>        while (const ElementRegion *ER = dyn_cast<ElementRegion>(MR))
>          MR = ER->getSuperRegion();
> 
> ...this should probably be replaced by either MR->getBaseRegion(), so that we strip off struct field layers as well, or MR->StripCasts(), so that we don't say that '&x[1]' is the address of 'x'.
> 

In one of the later patches Branden does replace the loop with MR = MR->getBaseRegion(). I think he submitted it separately.

Branden, can you send us just the final 2 patches?

Thanks!
Anna.

> Okay, I guess you're not in there anymore. I'll file a bug.
> 
> 
> +void freeOffsetPointerPassedToFunction()
> +{
> +  int *p = malloc(sizeof(int)*2);
> +  p[1] = 0;
> +  p += 1;
> +  myfooint(*p); // not passing the pointer, only a value pointed by pointer
> +  free(p); // expected-warning {{Argument to free() is not memory allocated by malloc(); the memory location passed is an offset of an allocated location}}
> +}
> 
> A better test would be to pass 'p' as a const pointer, which means the contents of the region get invalidated but 'p' itself will not.
> 
> 
> Okay, I think that's it! Thanks, Branden.
> Jordan
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130118/9ff59e39/attachment.html>