Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Jeff King <peff@peff.net>]

On Tue, Oct 02, 2018 at 12:44:09PM -0700, Stefan Beller wrote:

> > My worry is that one of these would seem to be true:
> >
> > * UNLEAK is unsuitable for the job. Whenever we have a `die()` as we do
> >   here, we can UNLEAK the variables we know of, but we can't do anything
> >   about the allocations we have made higher up the call-chain.
> 
> IMHO that is the issue of the functions higher up the call chain and ought
> to not affect this patch. By doing the right thing here locally the code base
> will approach a good state eventually.

But it's impossible. If I do this:

  foo = xstrdup(bar);
  subfunction(foo);

then I cannot protect myself from leaking "foo" when subfunction() calls
die(). It must be valid when I enter the function, and I have no
opportunity to run code when it leaves (because it never does).

> > * We add code with no purpose. In this case, we're not talking a lot of
> >   lines, but across the code base, if they bring no gain, they are bound
> >   to provide a negative net value given enough time.
> 
> I see. I did not estimate its negative impact to be high enough, as the
> UNLEAK near a die() call was obvious good thing (locally).
> 
> I don't know what the best way to proceed is in this case.

My preference is to avoid them in the name of simplicity. If you're
using "make SANITIZE=leak test" to check for leaks, it will skip these
cases. If you're using valgrind, I think these may be reported as
"reachable". But that number already isn't useful for finding real
leaks, because it includes cases like the "foo" above as well as
program-lifetime globals.

The only argument (IMHO) for such an UNLEAK() is that it annotates the
location for when somebody later changes the function to "return -1"
instead of dying. But if we are going to do such annotation, we may as
well actually call free(), which is what the "return" version will
ultimately have to do.

I'd actually be _more_ favorable to calling free() instead of UNLEAK()
there, but I'm still mildly negative, just because it may go stale (and
our leak-checking wouldn't usefully notice these cases). Anybody
converting that die() to a return needs to re-analyze the function for
what might need to be released (and that includes non-memory bits like
descriptors, too).

-Peff