Re: [PATCH 09/11] commit-graph: avoid using stale stack addresses
- From
Jeff King <peff@peff.net>
- Date
- May 15, 2025, 20:19 UTC
- Message-ID
- <20250515201943.GJ3320240@coredump.intra.peff.net>
- In-Reply-To
- <077bcab206f5bfc9fc10a28ad7b726a6ec16c2bb.1747314709.git.gitgitgadget@gmail.com>
On Thu, May 15, 2025 at 01:11:47PM +0000, Johannes Schindelin via GitGitGadget wrote:
Show 7 quoted lines
> The code is a bit too hard to reason about to fully assess whether the > `fill_commit_graph_info()` function is called at all after > `write_commit_graph()` returns (and hence the stack variable > `topo_levels` goes out of context). > > Let's simply make sure that the stack address is no longer used at that > stage, thereby making the code quite a bit easier to reason about.
Yep, I think this is a good practice in general. If the topo_levels member is never used outside of writing, I wonder if it could live in a separate data structure. But that is a much bigger refactor that I don't think we need to tackle here.
Show 16 quoted lines
> diff --git a/commit-graph.c b/commit-graph.c
> index 9f0115dac9b5..d052c1bf15c5 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2683,6 +2683,15 @@ cleanup:
> oid_array_clear(&ctx.oids);
> clear_topo_level_slab(&topo_levels);
>
> + if (ctx.r->objects->commit_graph) {
> + struct commit_graph *g = ctx.r->objects->commit_graph;
> +
> + while (g) {
> + g->topo_levels = NULL;
> + g = g->base_graph;
> + }
> + }This just clears the pointers to the local variable. Looks good.
-Peff