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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2022, 18:52 UTC
Message-ID: <xmqqk07v69bh.fsf@gitster.g>
In-Reply-To: <YuQpBwUt8rAE9oym@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> The more interesting question is whether it causes any use-after-free
> bugs.

Thanks for mentioning this. All the "plug more leaks" patches make me worried for exactly that. Another is a potential subtle breakage hidden by use of FREE_AND_NULL() and friends, which the sanitizers would probably not see, but can appear as behaviour change.

> I don't think it does, and certainly SANITIZE=address agrees.

;-)

Show 35 quoted lines

> -- >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@peff.net>
> ---
>  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)