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

[Jordy Rose]

On Fri, 2 Jul 2010 12:56:30 +1200, Andrew McGregor <andrewmcgr at gmail.com>
wrote:
> On Tue, Jun 29, 2010 at 1:03 PM, Jordy Rose <jediknil at belkadan.com>
wrote:
> 
>>
>> >> 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?
>>
>> I believe so, but for return values from function calls we prefer to
use
>> a
>> "conjured symbol value". This is a kind of value that can be reasoned
>> about
>> symbolically in later statements -- for example, we can bound its value
>> if
>> it's an integer, or mark that it's null or non-null if it's a pointer.
> 
> 
> So, I'm still not sure I understand what you mean me to do here... could
> you
> elaborate?
> 
> Since you seem to be the symbolic value expert, I'll ask about the case
I'm
> trying to solve now.  I added some symbolic checks to the free handling,
> similar to realloc, to prevent warnings in the common case of error
> handling
> for allocation handlers.

Sorry, I should have been clearer! If you take a look at MallocMemAux,
you'll see these lines:

  unsigned Count = C.getNodeBuilder().getCurrentBlockCount();
  SVal RetVal = ValMgr.getConjuredSymbolVal(NULL, CE, CE->getType(),
Count);
  state = state->BindExpr(CE, RetVal);

This sort of code is used for any function call that returns a value. It's
better than just leaving the expression as UnknownVal because it lets us
track assumptions about the value from then on.

Consider this code:

int freeNode (Node *n) __attribute__(ownership_takes(1)) {
  int id = n->id;
  free(n);
  return id;
}

// in some other function
int x = freeNode(last);
if (x < 0) {
  log("invalid node: %d", x);
  return;
}
// from here on x is known to be positive

So all you would need to add is those three lines, for the case in which
you /are/ handling the function (returning true from EvalCallExpr), /not/
calling MallocMemAux (which is already handling the return value), and the
function's return type isn't void.

Alternately, go ahead and make the ownership_takes/ownership_holds cases
part of a new PreVisitCallExpr method. PreVisitCallExpr doesn't require you
to handle the return value, and allows you to stack multiple checks. This
is probably more correct anyway -- it would let users annotate methods that
may eventually need another checker to actually do the evaluation.

As for the example you showed, it should work, except:

> struct it *  __attribute((ownership_returns(malloc))) foo(void) {
>   struct it *rv = malloc(sizeof(struct it));
>   if (!rv)
>     return NULL; // Does not warn here.
>   char *textString = malloc(128*sizeof(char));
>   if(textString == NULL)
>     free(rv);
>     return NULL; // Warns about a memory leak here
>   rv->s = textString;
>   return rv; // Does NOT warn here
> }

...the code is just missing braces around the second if -- the second
"return NULL" is unconditional!

Clang should catch this. Filing a bug. *grin*