Re: [PATCH] rebase --autosquash: fix a potential segfault

From: Junio C Hamano <gitster@pobox.com>
Date: May 4, 2020, 21:19 UTC
Message-ID: <xmqq4ksvflep.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.625.git.1588624804554.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 19 quoted lines

> diff --git a/sequencer.c b/sequencer.c
> index e528225e787..0d4d53d2a49 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5266,8 +5266,17 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>  				TODO_FIXUP : TODO_SQUASH;
>  			if (next[i2] < 0)
>  				next[i2] = i;
> -			else
> +			else if (tail[i2] >= 0)
>  				next[tail[i2]] = i;
> +			else {
> +				/*
> +				 * i2 refers to a fixup commit in the middle of
> +				 * a fixup chain
> +				 */
> +				next[i] = next[i2];
> +				next[i2] = i;
> +				continue;

OK, this would catch the case even when fixing up a fix-up of antoher fix-up, so we won't need further "else if" in the future ;-)

I suspect that this breakage is as old as 2.14, introduced by c44a4c65 (rebase -i: rearrange fixup/squash lines using the rebase--helper, 2017-07-14), but perhaps we won't need to backmerge the fix that far. We don't even backport security fixes beyond 2.17 (which is two years old).

Just in case I'll queue this immediately on top of f2a04904 (sequencer: refactor rearrange_squash() to work on a todo_list, 2019-03-05); that would give us a potential to cover as far back to 2.20 series.

Thanks.

Show 29 quoted lines

> +			}
>  			tail[i2] = i;
>  		} else if (!hashmap_get_from_hash(&subject2item,
>  						strhash(subject), subject)) {
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 093de9005b7..ca135349346 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -424,4 +424,18 @@ test_expect_success 'abort last squash' '
>  	! grep first actual
>  '
>  
> +test_expect_success 'fixup a fixup' '
> +	echo 0to-fixup >file0 &&
> +	test_tick &&
> +	git commit -m "to-fixup" file0 &&
> +	test_tick &&
> +	git commit --squash HEAD -m X --allow-empty &&
> +	test_tick &&
> +	git commit --squash HEAD^ -m Y --allow-empty &&
> +	test_tick &&
> +	git commit -m "squash! $(git rev-parse HEAD^)" -m Z --allow-empty &&
> +	git rebase -ki --autosquash HEAD~4 &&
> +	test XZY = $(git show | tr -cd X-Z)
> +'
> +
>  test_done
>
> base-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec