On Tue, Oct 03, 2023 at 04:30:04PM -0400, Jeff King wrote:
> When adding a graph to a chain, we do some consistency checks and then
> if everything looks good, set g->base_graph to add a link to the chain.
> But when we added a new consistency check in 209250ef38 (commit-graph.c:
> prevent overflow in add_graph_to_chain(), 2023-07-12), it comes _after_
> we've already set g->base_graph. So we might return failure, even though
> we actually added to the chain.
>
> This hasn't caused a bug yet, because after failing to add to the chain,
> we discard the failed graph struct completely, leaking it. But in order
> to fix that, it's important that the struct be in a consistent and
> predictable state after the failure.
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> commit-graph.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 2f75ecd9ae..2c72a554c2 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -498,8 +498,6 @@ static int add_graph_to_chain(struct commit_graph *g,
> cur_g = cur_g->base_graph;
> }
>
> - g->base_graph = chain;
> -
> if (chain) {
> if (unsigned_add_overflows(chain->num_commits,
> chain->num_commits_in_base)) {
> @@ -510,6 +508,8 @@ static int add_graph_to_chain(struct commit_graph *g,
> g->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;
> }
>
> + g->base_graph = chain;
> +
Oops. That looks like my fault. Thanks for catching, this switch makes
sense to me.
Thanks,
Taylor
