Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives

[Jeff King <peff@peff.net>]

On Tue, Sep 05, 2017 at 03:05:12PM -0700, Stefan Beller wrote:

> On Tue, Sep 5, 2017 at 6:05 AM, Jeff King <peff@peff.net> wrote:
> 
> >   int main(void)
> 
> nit of the day:
>   s/void/int argc, char *argv/ or in case we do not
>   want to emphasize the argument list s/void//
>   as that adds no uninteresting things.

That really is a nit. I chose not to provide argv because it's longer
than "void" and I wasn't going to use the arguments. And I chose not to
use an empty argument list because it violates our style (as well as
arguably the C standard, though it leaves room for implementations to
take other forms of main).

> > In other words, you can do:
> >
> >   int main(void)
> >   {
> >         char *p = some_function();
> >         printf("%s", p);
> >         UNLEAK(p);
> >         return 0;
> >   }
> >
> > to annotate "p" and suppress the leak report.
> 
> This sounds really cool so far.
> 
> After having a sneak peak at the implementation
> it is O(1) in runtime for each added element, and the
> space complexity is O(well).

I'm not sure if your "well" is "this does well" or "well, it could be
quite a lot". :)

It certainly has the potential to grow the heap without bound (since
after all, it's whole point is to make a giant list of variables that
are going out of scope). But in practice we'd sprinkle this over a
handful of variables just before program exit (and remember that it's
copying only what's on the stack already; so pointers get copied, not
whole heap-allocated blocks).

Plus it does nothing at all when not compiled with leak-checking. So I'm
not too worried about the extra memory usage or performance.

> >   1. It can be compiled conditionally. There's no need in
> >      normal runs to do this free(), and it just wastes time.
> >      By using a macro, we can get the benefit for leak-check
> >      builds with zero cost for normal builds (this patch
> >      uses a compile-time check, though we could clearly also
> >      make it a run-time check at very low cost).
> >
> >      Of course one could also hide free() behind a macro, so
> >      this is really just arguing for having UNLEAK(), not
> >      for its particular implementation.
> 
> This is only a real argument in combination with (2), or in other
> words you seem to hint at situations like these:

Well, the numbered list was meant to be a set of arguments, each of
which contributes to the overall conclusion. :) I agree that (1) is the
weakest. Since both you and Martin seemed to get hung up on it, I'll
re-organize it a bit for the re-roll.

>   5. It's not just about worrying if we can call UNLEAK
>       once (in 4), but we also do not have to worry about
>       calling it twice, or recursively. (This argument can be bad
>       for cargo cult programmers, but we don't have these ;-)

True. I didn't come across that case in any of the ones I converted. As
a more general rule, UNLEAK() doesn't access any pointed-to memory at
all. So it's fine with already-freed or even uninitialized memory (which
of course is technically wrong according to the standard, but in
practice would be fine, as we'd copy garbage that does not match a heap
block).

> > +#ifdef SUPPRESS_ANNOTATED_LEAKS
> > +extern void unleak_memory(const void *ptr, size_t len);
> > +#define UNLEAK(var) unleak_memory(&(var), sizeof(var));
> 
> As always with macros we have to be careful about its arguments.
> 
>   UNLEAK(a++)
>   UNLEAK(baz())
> 
> won't work as intended.

Yes, I intended this to be used only for actual variables. I couldn't
think of a way to enforce that at compile time with some kind of
BUILD_ASSERT (even requiring an lvalue isn't quite strict enough).

-Peff