Re: [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

From: Jeff King <peff@peff.net>
Date: Jun 30, 2021, 17:26 UTC
Message-ID: <YNypPeoZTRiOxPPQ@coredump.intra.peff.net>
In-Reply-To: <patch-1.3-3d0d7a8e8b5-20210630T140339Z-avarab@gmail.com>

On Wed, Jun 30, 2021 at 04:06:14PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 10 quoted lines

> Fix a memory leak from the prefix_filename() function introduced with
> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
> 2017-03-20).
> 
> As noted in that commit the leak was intentional as a part of being
> sloppy about freeing resources just before we exit, I'm changing this
> because I'll be fixing other memory leaks in the bundle API (including
> the library version) in subsequent commits. It's easier to reason
> about those fixes if valgrind runs cleanly at the end without any
> leaks whatsoever.

Thanks, this looks good to me.

One thing, though...

Show 5 quoted lines

> An earlier version of this change went out of its way to not leak
> memory on the die() codepaths here, but that was deemed too verbose to
> worry about in a built-in that's dying anyway. The only reason we'd
> need that is to appease a mode like SANITIZE=leak within the scope of
> an entire test file.

Obviously you changed this as I asked, but this final sentence makes me think we're not on the same page with respect to die(). I don't think any kind of mode matters here. When we call die(), whatever we have on the stack is _not_ a leak, by LSan's or valgrind's standards. Because we still have access to those bytes. And nor can we ever get rid of such cases. If we ever do:

  void foo(const char *str)
  {
	char *x = xstrdup(str);
	bar(x);
	free(x);
  }

  void bar(const char *x)
  {
	if (!strcmp(x, "foo"))
		die("whatever");
  }

Then "x" will always still be allocated when we die(). We cannot free it in bar(), where it is read-only. We cannot free it in foo() before we call bar(), because it is needed there. But control never returns to the free() statement.

So this code is perfectly fine and unavoidable. In the case you were touching it was foo() that was calling die() directly, so we could work around it with some conditionals. But from the leak-checker's perspective the two cases are the same.

-Peff