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

From: Jeff King <peff@peff.net>
Date: Sep 7, 2017, 09:00 UTC
Message-ID: <20170907090018.t6ogu2vufdfhz3n6@sigill.intra.peff.net>
In-Reply-To: <CAN0heSpYZT7cm=XNpfgcsGFa9FOR6SdaF=vXJ+M7NNaA6Mnb3g@mail.gmail.com>

On Wed, Sep 06, 2017 at 07:16:00PM +0200, Martin Ågren wrote:

Show 22 quoted lines

> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index b3b04f5dd3..de775d906c 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -1819,5 +1819,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> >                 print_summary(prefix, &oid, !current_head);
> >
> >         strbuf_release(&err);
> > +       UNLEAK(sb);
> >         return 0;
> >  }
> 
> These are both strbufs, so this ends up being a bit inconsistent. What
> would be the ideal end state for these two and all other such
> structures? My guess is "always UNLEAK", as opposed to carefully judging
> whether foo_release() would/could add any significant overhead.
> 
> In other words, it would be ok/wanted with changes such as "let's UNLEAK
> bar, because ..., and while at it, convert the existing foo_release to
> UNLEAK for consistency" (or per policy, for smaller binary, whatever).
> Or "if it ain't broken, don't fix it"? Did you think about this, or was
> it more a random choice?

To be honest, I didn't really think that deeply about it. I had a hammer in my hand, and LSAN kept showing me nails to pound.

I agree that these two strbufs should probably be treated the same.

In general, I think I prefer using UNLEAK() because it's hard to get it wrong (i.e., you don't have to care about double-frees or uninitialized pointers). For strbufs, though, that's less of an issue because they are always maintained in a consistent state.

As an aside, I'm pretty sure that "err" can never have been allocated here, and this release is always a noop. It's filled in only when we get an error from the ref update, which also causes us to die(). But in general I'd prefer the code that causes readers to think the least (i.e., just calling free or UNLEAK here rather than forcing the reader to figure out whether it's possible to leak).

-Peff