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

[Jordan Rose]

(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'.

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/32a7d606/attachment.html>