[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators

[Jordan Rose]

Hi, Anton. This is a lot cleaner than I expected it to be! Nice work, and I'd love to get this in soon.

I'm going to mix style critique and design comments inline with your patch, but basically I think this is a good approach.

+enum AllocDeallocKind {
+  AD_malloc,
+  AD_new,
+  AD_newArr,
+  AD_free,
+  AD_delete,
+  AD_deleteArr,
+  AD_unknown
+};

Okay, biggest thing first. Anna, Ted, and I have discussed before about tracking allocators vs. deallocators, particularly in the context of the experimental "ownership_returns" attribute added a while back. While the design of that attribute can certainly change (and probably will), it does bring up an interesting point: to find bugs, the only information that's absolutely necessary to track is the expected deallocator. So rather than tracking both allocator and deallocator kinds, why not just track deallocators, and only produce the allocator kind when we're constructing a diagnostic message? (We need to do extra work there anyway.)

Also, I think we should spell out "deleteArray" in whole. The two-character savings doesn't make up for the slight loss in readability to me.


+  std::string GetAllocDeallocName(AllocDeallocKind kind) const {
+    return AllocDeallocNames[kind];
+  }

Please use StringRef here, since these strings are persistent. Also, it might make sense to store the strings in a static local, if you really think people should be going through this function.


+  std::string GetAllocDeallocName(CheckerContext &C, const Expr *E) const;
+  std::string GetProperAllocName(CheckerContext &C, 
+                                 const Expr *DeallocExpr) const {
+    return GetAllocDeallocName(GetProperAllocKind(C, DeallocExpr));
+  }

Similar comments. If you must return a dynamic string, it's probably better to pass in a SmallVectorImpl<char> or raw_ostream and fill it rather than copying around std::strings. (The shackles of C++03...)


+  // skip placement new operators as they may not allocate memory
+  if (NE->getNumPlacementArgs())
+    return;

Two comments here:
- Please make sure all comments are complete, capitalized, and punctuated sentences. (This has the important one, "complete"....just missing capitalization and punctuation.)
- This isn't actually correct -- any custom operator new will make this invalid, even without placement args. I think you need to check that there are no placement args and that it's the global operator new. (Note that IIRC isGlobalNew in CXXNewExpr does not test this -- that's whether it was actually written ::new.)


   // Set the symbol's state to Allocated.
-  return state->set<RegionState>(Sym, RefState::getAllocated(CE));
+  return state->set<RegionState>(Sym, RefState::getAllocated(E));

Given that you're making these changes, it makes sense to now put the DeallocKind in the RefState, so that we don't have to rederive it every time. There's plenty of space, since we're already wasting an entire word on the single-byte RefState::Kind.


+  if (isa<ObjCMessageExpr>(E))
+    return AD_malloc;

This probably deserves a comment, since there's no reason to assume that Objective-C methods all return memory that needs to be free()d. It just happens to be the case for the methods we recognize. This won't be true if/when we support custom deallocator annotations.


+  if (const CallExpr *CE = dyn_cast_or_null<CallExpr>(E)) {

The _or_null isn't necessary, since you've already tested that E is non-null. (Same in the two cases below.)


+    const FunctionDecl *FD = CE->getDirectCallee();

I'm not so happy about this, because an indirect call to malloc() won't show up as a direct callee. CheckerContext::getCalleeDecl is safer.

Eventually, I'm hoping this whole infrastructure will switch over to uniformly using CallEvent, but for that we need to resolve the "allocator call" CFG issue discussed in PR12014.


+  if (kind == AD_malloc) {
+    // get the exact name of an allocation function
+    if (const CallExpr *CE = dyn_cast_or_null<CallExpr>(E)) {
+      if (const FunctionDecl *FD = CE->getDirectCallee()) {
+        if (FD->getKind() == Decl::Function) {
+          if (IdentifierInfo *II = FD->getIdentifier()) {
+            std::string name = II->getName().str() + "()";
+            return name;
+          }
+        }
+      }
+    } else if (isa<ObjCMessageExpr>(E)) {
+      return "ObjC method";
+    }
+  }

The structure of this might change given my comments but:
- You can just print the FunctionDecl to a raw_ostream and get the right formatted output, rather than jumping down to the identifier.
- In this case, CheckerContext::getCalleeDecl won't get the right callee (because it relies on the contents of the ProgramState, which have probably been cleaned up as dead bindings by now). To fix this, you could store the callee Decl in the RefState as well...which would also allow you to print Objective-C methods as well.
- Please spell out "Objective-C" in user-visible messages.


+    std::string ProperAllocName = GetProperAllocName(C, DeallocExpr);
+    std::string DeallocName = GetAllocDeallocName(C, DeallocExpr);

Again, in your next version of the patch, please avoid std::string when it's possible to stick to stack-based structures like SmallString.


 void MallocChecker::ReportOffsetFree(CheckerContext &C, SVal ArgVal,
-                                     SourceRange Range) const {
+                                     SourceRange Range, const Expr *DeallocExpr,
+                                     const Expr *AllocExpr /* = 0 */) const {

We conventionally don't include the default arguments in the function definition.


That's plenty to chew on for now, but I'd really like to see this go in! I think Anna may also have some comments but is waiting for the second round of the patch.

Thank you,
Jordan


On Feb 11, 2013, at 21:19 , Anton Yartsev <anton.yartsev at gmail.com> wrote:

> Hi all,
> 
> the attached patch has three goals:
> 1) Bring handling of new/delete operators to the unix.Malloc checker. 
> This enables memory.LeakNeverReleased, memory.MultipleDelete, memory.DeallocateNonPtr and memory.LeakPtrValChanged checkers from the List of potential checkers to check memory allocated with new/delete. PR15237 created to track progress on the task.
> 2) Add memory.MismatchedFree and memory.MismatchedDelete checks. ( PR15238 )
> 3) Display real allocator/deallocator names instead of hardcoded 'malloc()' and 'free()' if possible.
> Vital for 1) and 2)
> 
> please review
> 
>  -- 
> Anton
> <MallocChecker.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/20130215/8be72637/attachment.html>