On Thu, May 15, 2025 at 01:11:40PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > 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. ;) > 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
