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

[Anna Zaks]

On Feb 19, 2013, at 10:18 PM, Anton Yartsev <anton.yartsev at gmail.com> wrote:

> Hi, Jordan. Thanx for the review!
> 
> Attached is the new version of the patch with all the comments addressed. Also added support for directly called operator new()/new[]() and operator delete()
> 
> There is currently one problem with handling of operator delete(). The following test
> 
> void testDeleteOp1() {
>  int *p = (int *)malloc(sizeof(int));
>  operator delete(p); // FIXME: should complain "Argument to operator delete() was allocated by malloc(), not operator new"
> }
> 
> produce no warnings as attached RefState seem to be missing at the point when checkPostStmt(const CallExpr *CE, CheckerContext &C) callback is called for operator delete(p).
> I haven't investigated the problem deeply yet, intend to address it parallel with the review.
> 
>> +  if (NE->getNumPlacementArgs())
>> +    return;
>> +  // skip placement new operators as they may not allocate memory
>> 
>> 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.)
> I'll try. Unfortunately I am not as good in English as I want to be, so sorry for my grammar, syntax, and punctuation.
> 
> -- 
> Anton
> 
> <MallocChecker_v2.patch>

Anton, it's great that you are working on this! See the comments/feedback below.

+// Numeration is added for convenient mapping to RefState::Kind
Should be "Enumeration"

   enum Kind { // Reference to allocated memory.
               Allocated,
....
+
+              // Mapped DeallocatorKind. 
+              // Expected kind of a deallocator; used to check if a real 
+              // kind of a deallocator matches expected one
+              Free = 0x4,
+              Delete = 0x8,
+              DeleteArray = 0xC

Wouldn't it be cleaner and simpler if the Kind enumeration just had something like this:
 AllocatedWithMalloc 
 AllocatedWithNew
 AllocatedWithNewArray

Or, if we want the name to be based on the deallocator function, we could use something like "ToBeReleasedWithFree, ToBeReleasedWithNew, ..". 

Just adding another kind as extra enumeration values does not seem right. Another option is to make Kind be a pointer to a static array, which contains objects recording all necessary info about each kind (MacOSKeychainAPIChecker uses this approach). This is probably an overkill for now, but is another option.

+  const FunctionDecl *getCalleeDecl() const { return CalleeDecl; }
Do we only store the name of the allocator declaration here? Do we need to store this in the state? (Growing states implies memory overhead.) Can't this be easily implied from the kind?

One thing to keep in mind here, is that it would be beneficial to make this all work well with ownership annotations (even though they are not supported now, they are something we might want to have in the future). So feel free to push back on simplification suggestions if there is a clear benefit with respect to the ownership annotations support. On the other hand, we don't need to design a solution that would fit the annotations now either.

+  bool isDefaultNonptrplacementNewDelete(const FunctionDecl *FD,
Camel case should be used

+    } else if (isDefaultNonptrplacementNewDelete(FD, C)) {
+      OverloadedOperatorKind K = FD->getDeclName().getCXXOverloadedOperator();
+      if (K == OO_New)
+        State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
+                             D_delete);
+      else if (K == OO_Array_New)
+        State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
+                             D_deleteArray);
+      else
+        State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
In the last else, we assume that delete/delete[] was called based on the logic in isDefaultNonptrplacementNewDelete. Please either check the kind again or assert this. We want to make sure they don't go out of sync.

+void MallocChecker::checkPostStmt(const CXXNewExpr *NE, 
+                                  CheckerContext &C) const {NE
+
+  FunctionDecl *OperatorNew = NE->getOperatorNew();
+  if (!OperatorNew)
+    return;
+
+  // Skip custom new operators
+  if (!OperatorNew->isImplicit() &&
+      !C.getSourceManager().isInSystemHeader(OperatorNew->getLocStart()) &&
+      !NE->isGlobalNew())
+    return;
+
+  // Skip standard global placement operator new/new[](std::size_t, void * p);
+  // process all other standard new/new[] operators including placement
+  // operators new/new[](std::size_t, const std::nothrow_t&)
+  if (OperatorNew->isReservedGlobalPlacementOperator())
+    return;

Is there a reason why we first process each operator new in "checkPostStmt(const callExpr" and finish processing in "checkPostStmt(const CXXNewExpr" ? I think the code would be simpler if everything could be done in a single callback. 

Also, the tests above are similar to what is done in isDefaultNonptrplacementNewDelete. These should be factored out into a subroutine to avoid inconsistency. (Ex: Shouldn't "NE->isGlobalNew()" check be done in both?)

Same remarks go for "checkPreStmt(const CXXDeleteExpr".

+void MallocChecker::PrintExpectedAllocName(raw_ostream &os, CheckerContext &C, 
+                                           const Expr *E) const {
+  DeallocatorKind dKind = GetDeallocKind(C, E);
+
+  switch(dKind) {
+    case D_free: os << "malloc()"; return;
+    case D_delete: os << "operator new"; return;
+    case D_deleteArray: os << "operator new[]"; return;
+    case D_unknown: os << "unknown means"; return;

This function is used to form user visible warnings. Do we ever expect it to print "unknown means"? Can this be based on the Kind stored inside of RefState, where there is no D_unknown?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130220/36a7b37b/attachment.html>