[cfe-dev] Fwd: Ownership attribute for malloc etc. checking

[Andrew McGregor]

Oops, forgot to reply-all...

---------- Forwarded message ----------
From: Andrew McGregor <andrewmcgr at gmail.com>
Date: Mon, Jun 28, 2010 at 12:04 PM
Subject: Re: Ownership attribute for malloc etc. checking
To: Jordy Rose <jediknil at belkadan.com>


Hi,

Thanks for the review.  Comments inline, new patch attached.

Andrew

On Sun, Jun 27, 2010 at 9:33 AM, Jordy Rose <jediknil at belkadan.com> wrote:

>
> Looking good! Some remaining comments below.
>
> I'm also a reasonably new committer, so someone else should probably look
> over this as well (particularly the Attr hunks, which I'm not very familiar
> with).
>
> Jordy
>
>
> General:
> - It would be nice to make sure one argument isn't both ownership_takes
> and ownership_holds.
>

True, will do.


> - LLVM's convention is to use spaces rather than tabs.
> - There's a hunk where the only thing that changed is indentation; it'd be
> nice to take that out.
>

Oops, neither of these were intentional.


> - Two more useful test would be free(p); my_free(p) and free(p);
> my_hold(p). Both of which should warn, of course.
>

Instead of crashing, yes.  Well spotted.


>  (MallocChecker::EvalCallExpr)
> +    for (const Attr *attr = FD->getAttrs(); attr; attr = attr->getNext())
> {
> +      if (const OwnershipReturnsAttr* Att =
> dyn_cast<OwnershipReturnsAttr>(attr)) {
> +        MallocMemReturnsAttr(C, CE, Att);
> +        rv = true;
> +      }
> +      if (const OwnershipTakesAttr* Att = dyn_cast<OwnershipTakesAttr>
> (attr)) {
> +        FreeMemTakesAttr(C, CE, Att);
> +        rv = true;
> +      }
> +      if (const OwnershipHoldsAttr* Att = dyn_cast<OwnershipHoldsAttr>
> (attr)) {
> +        FreeMemHoldsAttr(C, CE, Att);
> +        rv = true;
> +      }
> +    }
>
> Here's the dispatch from EvalCallExpr(). I think even though it's
> technically a little less efficient, you should just simplify this and use
> FD->getAttr<OwnershipTakesAttr>(), etc. It's a little simpler, and it lets
> you order the attributes.
>

However, I did it this way because I wanted to allow two or more attributes
of the same kind (for the benefit of macros).

In other words, __attribute((ownership_holds, 1))
__attribute((ownership_holds, 2)) should be the same
as __attribute((ownership_holds, 1, 2)).


> Why does that matter? Because an ownership_takes() or ownership_holds()
> function might have a return value! If it's also ownership_returns(), then
> you're fine, but otherwise you need to set a symbolic return value. (You
> can see how this is done in MallocMemAux(), or in StreamChecker.cpp with
> fopen.) There should probably be a test for this as well.


It isn't necessarily the case that an ownership_takes() function that
returns a pointer generated that pointer from an ownership_takes argument,
or allocated anything, so what would you set the region to, in the absence
of other information?  They default to Unknown, yes?


> Actually, because these attributes don't affect the return value, they
> should probably go in a PreVisitCallExpr() callback, rather than
> EvalCallExpr(). But for now it's probably okay, as long as you handle that
> return value.
>
>
> (MallocChecker::MallocMemReturnsAttr)
> +  for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I,
> ++count) {
> +    const GRState *state =
> +        MallocMemAux(C, CE, CE->getArg(*I), UndefinedVal(),
> C.getState());
> +    C.addTransition(state);
> +  }
> +  if (count == 0) {
> +    const GRState *state = MallocMemAux(C, CE, UndefinedVal(),
> UndefinedVal(),
> +                                        C.getState());
> +    C.addTransition(state);
> +  }
>
> What does it mean to have multiple size fields? There can still only be
> one return value. Maybe you should only allow zero or one positions for
> ownership_returns (which would make this section simpler).
>

The intention here was to implement something for calloc-like functions and
matrix allocators; one argument is a special case, more than one the size of
the region is the product of the arguments times sizeof(pointee of the
return value), taking sizeof(void) as 1.  If it isn't worth doing that, fair
enough, but that's what I was thinking this could lead to.

However, the parser only allows the one argument at the moment, so this is
redundant.


> (MallocChecker::PreVisitReturnStmt)
> +  if (RS->isReleased()) {
> +    ExplodedNode *N = C.GenerateSink();
> +    if (!BT_UseFree)
> +      BT_UseFree = new BuiltinBug("Use dynamically allocated memory
> after"
> +                                  " it is freed.");
>
> +    BugReport *R = new BugReport(*BT_UseFree,
> BT_UseFree->getDescription(),
> +                                 N);
> +    C.EmitReport(R);
> +  }
>
> This section checks whether the return value is a released address.
> Unfortunately this isn't a 100% unambiguous case; consider the following
> (contrived) example:
>
> struct Node *freeSmallest () {
>  struct Node *min;
>  // find the minimum node
>  free(min);
>  return min; // so the caller knows the ID of the deleted node
> }
>
> As long as the caller doesn't dereference the returned pointer, it could
> still be a useful value. 95% of the time, this check would be useful, but
> since a false positive is worse than a false negative for the analyzer, we
> should probably leave it out.
>

Ok, I see.  Out it is, then.

Andrew
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100629/7b3199d5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-ownership-withtests-r1.patch
Type: text/x-patch
Size: 31470 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100629/7b3199d5/attachment.bin>