Re: [PATCH 02/11] fetch: carefully clear local variable's address after use

From: Jeff King <peff@peff.net>
Date: May 15, 2025, 19:40 UTC
Message-ID: <20250515194003.GB3320240@coredump.intra.peff.net>
In-Reply-To: <c66eaee64ad5484120f1905ce5501fd8b75b2266.1747314709.git.gitgitgadget@gmail.com>

On Thu, May 15, 2025 at 01:11:40PM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 11 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> As pointed out by CodeQL, it is a potentially dangerous practice to
> store local variables' addresses in non-local structs. Yet this is
> exactly what happens with the `acked_commits` attribute that is used in
> `cmd_fetch()`: The pointer to a local variable is assigned to it.
> 
> Now, it is Git's convention that `cmd_*()` functions are essentially
> only returning just before exiting the process, therefore there is
> little danger that this attribute is used after the code flow returns
> from that function.

I was going to say: the real sin here is using a global variable in the first place, without which gtransport would not survive outside of cmd_fetch(). But the issue is even worse than that. The acked_commits variable is inside a conditional block, so the address is stale for the rest of cmd_fetch(), too!

It doesn't look like we ever examine it after that, but it's hard to trace, since it's a global. ;)

Show 12 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index cda6eaf1fd6e..c1a1434c7096 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2560,6 +2560,7 @@ int cmd_fetch(int argc,
>  		if (server_options.nr)
>  			gtransport->server_options = &server_options;
>  		result = transport_fetch_refs(gtransport, NULL);
> +		gtransport->smart_options->acked_commits = NULL;
>  
>  		oidset_iter_init(&acked_commits, &iter);
>  		while ((oid = oidset_iter_next(&iter)))

Here you unset it within that conditional block, which is the right spot. Looks good.

-Peff