Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jul 29, 2022 at 02:03:16PM -0400, Jeff King wrote:

> On Fri, Jul 29, 2022 at 09:07:40AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > > In that case, we could replace your patch 5 in favor of just calling
> > > strvec_clear() at the end of bisect_rev_setup().
> > 
> > 5/6 is doing that, or rather at the end of check_ancestors() and
> > bisect_next_all(), but those call bisect_rev_setup() and pass it the
> > strvec, so in terms of memory management it amounts to the same thing.
> 
> Right, but my point is that we do not need to complicate the interface
> and ownership rules for bisect_rev_setup() by passing around the extra
> variable.

Just to be clear, what I'm proposing is to replace your 5/6 with the
patch below.

It passes the leak-checker, but I don't think that should be a surprise.
The end result should be the same as your patch, and anyway I don't
think your series actually marks any scripts as PASSING_SANITIZE_LEAK
as a result of fixing this (presumably because there are other leaks in
those scripts, too).

The more interesting question is whether it causes any use-after-free
bugs. I don't think it does, and certainly SANITIZE=address agrees.

-- >8 --
Subject: [PATCH] bisect: stop leaking strvec in bisect_rev_setup()

Back when 8a534b6124 (bisect: use argv_array API, 2011-09-13) was
written, it was not safe to free the argv we had passed to
setup_revisions() until the actual traversal was done.

But since then, we've had many cleanups that makes this safe; e.g.,
df835d3a0c (add_rev_cmdline(): make a copy of the name argument,
2013-05-25) and 31faeb2088 (object_array_entry: fix memory handling of
the name field, 2013-05-25). The comment here is now out-dated; we can
just clear the strvec rather than leaking.

Note that there's still a small leak because of the way
setup_revisions() handles removed elements internally. That will be
fixed in a subsequent patch.

Signed-off-by: Jeff King <peff@xxxxxxxx>
---
 bisect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bisect.c b/bisect.c
index 421470bfa5..9cce23e929 100644
--- a/bisect.c
+++ b/bisect.c
@@ -670,7 +670,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
 		read_bisect_paths(&rev_argv);
 
 	setup_revisions(rev_argv.nr, rev_argv.v, revs, NULL);
-	/* XXX leak rev_argv, as "revs" may still be pointing to it */
+	strvec_clear(&rev_argv);
 }
 
 static void bisect_common(struct rev_info *revs)
-- 
2.37.1.804.g1775fa20e0




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux