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

[Jordan Rose]

This is looking great!


On Feb 27, 2013, at 3:59 , Anton Yartsev <anton.yartsev at gmail.com> wrote:

> I am slightly suspicious of bit-fields as too much is implementation-defined (again, when I used the enum type 'Kind' instead of the 'unsigned' in 'unsigned State : 2' the State failed to hold enum values for some reason. Haven't dig this, maybe the case is related to "A bit-field shall have integral or enumeration type (3.9.1). It is implementation-defined whether a plain (neither explicitly signed nor unsigned) char, short, int, long, or long long bit-field is signed or unsigned."), but this approach is much more convenient and readable.

Yeah, enum-typed bitfields can be a bit dangerous. We do use unsigned bitfields to store enums quite a bit in LLVM, though, so this is an approach that is safe.


One last question for you before jumping into patch review... Right now, the new diagnostic looks like this:

> Argument to operator delete[] was allocated by malloc(), not operator new[]

I wonder if this is more helpful, or some variation of this:

> Memory allocated by malloc() should be deallocated by free(), not operator delete[].


I'm really not sure whether it's better to mention the deallocator's expected allocator, or the allocated region's expected deallocator. Which mistake do you think people are more likely to make?

(Also, I came up with that phrasing in ten seconds; it could totally change.)


Okay, review comments:

> +  /// \brief Print expected name of an allocator based on the deallocators
> +  /// family.

"deallocator's"


> -                                     ProgramStateRef state) {
> +                                     ProgramStateRef state,
> +                                     AllocationFamilies family = AF_Malloc) {

I know the file isn't consistent, including the existing line here, but we try to use UpperCamelCase for variable and parameter names now in LLVM. This appears in a couple places.


> +  static ProgramStateRef MallocUpdateRefState(
> +                                         CheckerContext &C,
> +                                         const Expr *E,
> +                                         ProgramStateRef state,
> +                                         AllocationFamilies family = AF_Malloc);

We don't have great rules for when function declaration lines get too long, but generally what happens elsewhere in the analyzer is we put a newline after the return type. If not, please just double-indent the first parameter and line up the rest below, rather than trying to indent as far as possible.


> +  if (FD->getDeclName().getNameKind() != DeclarationName::CXXOperatorName)
> +    return false;
> +
> +  OverloadedOperatorKind kind = FD->getDeclName().getCXXOverloadedOperator();

Turns out you can collapse this down using FD->getOverloadedOperator(). Also, capital letter on "Kind".


> +  // Return true if tested operator is a standard placement nothrow operator.
> +  if (FD->getNumParams() == 2) {
> +    QualType T = FD->getParamDecl(1)->getType().getUnqualifiedType();
> +    if (const IdentifierInfo *II = T.getBaseTypeIdentifier()) {
> +      return II->getName().equals("nothrow_t");
> +    }
> +  }

Clever...I wouldn't have thought of this case. You can simplify a bit by dropping the getUnqualifiedType.

I'm a bit worried about doing a string compare for this, but I guess that's probably premature optimization. People don't use nothrow new that often.

LLVM coding style says to leave out the braces around a single-line if-statement.


> +      // Process direct calls to operator new/new[]/delete/delete[] functions
> +      // as distinct from new/new[]/delete/delete[] expressions that are 
> +      // processed by  the checkPostStmt callbacks for CXXNewExpr and 
> +      // CXXDeleteExpr.

Extra space after "by".


> +  bool ReleasedAllocated = false;
> +  State = FreeMemAux(C, DE->getArgument(), DE, State,
> +                     /*Hold*/false, ReleasedAllocated);

No need to initialize this to 'false', since we're not going to use the value anyway. (I see that other places have the unneeded initialization as well.)


> +      OverloadedOperatorKind kind = 
> +          FD->getDeclName().getCXXOverloadedOperator();

Same as before.


> +  switch(Family) {


Space before the open paren.


> +    case AF_Malloc: os << "malloc()"; return;
> +    case AF_CXXNew: os << "operator new"; return;
> +    case AF_CXXNewArray: os << "operator new[]"; return;
> +    case AF_None: llvm_unreachable("unknown allocation family");

Since it's not a default case anymore, the message should probably be changed. Now it's something like "not a deallocation expression".


> -  // Parameters, locals, statics, and globals shouldn't be freed.
> +  // Parameters, locals, statics, globals, and memory returned by alloca() 
> +  // shouldn't be freed.

Thanks for fixing this while you're here. :-)


> +  if (!printAllocDeallocName(os, C, DeallocExpr))
> +    os << "a deallocator";

Although "Argument to a deallocator is offset from the start of memory..." is better English, in this case "a" isn't exactly appropriate because we know which deallocator we're talking about. (We just don't know how to print its name.) I think just dropping the "a" ("deallocator") is probably fine, and seems reasonably consistent with other analyzer and compiler messages.

(This appears twice.)


> +       os << " was allocated by ";
> +       if(!printAllocDeallocName(os, C, AllocExpr))
> +         os << "an unknown allocator";
> +       os << ", not ";

This is not as nice as it could be. If we can't print the allocator, we should have a phrasing that just avoids mentioning it at all.


> -     << " from the start of memory allocated by malloc()";
> +     << " from the start of memory allocated by ";
> +  if (AllocExpr)
> +    if(!printAllocDeallocName(os, C, AllocExpr))
> +      os << "an allocator";
> +  else
> +    printExpectedAllocName(os, C, DeallocExpr);

This is also not quite as nice as it could be (same reason). (Also, the "else" case doesn't match the right "if"!)


> +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"
> +}

Hm. Any idea why this is not working? Is it not showing up as a standard operator delete?


Jordan