Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 11, 2020, 15:28 UTC
Message-ID: <20200811152832.GC19871@syl.lan>
In-Reply-To: <20200811131313.3349582-1-detegr@rbx.email>

Hi Antti,

Thanks for working on this. I have a few thoughts below, but I think that this is on the right track.

On Tue, Aug 10, 2020 at 04:13:15PM +0300, Antti Keränen wrote:

> 'todo_list_write_to_file' may overwrite the static buffer, originating
> from 'find_unique_abbrev', that was used to store the short commit hash
> 'c' for "# Rebase a..b onto c" message in the todo editor.

It would be great to know the commit that regressed, or if this has always been the case. I'm not sure if you'll have a ton of luck bisecting, since you indicate that this overwrite *may* occur (that makes me think that it doesn't always happen, so your efforts to bisect the change may be noisy).

Show 6 quoted lines

> Fix by duplicating the string before usage, so subsequent calls to
> 'find_unique_abbrev' or other functions calling 'hash_to_hex_algop_r'
> can't overwrite the buffer.
>
> Found-by: Jussi Keränen <jussike@gmail.com>
> Signed-off-by: Antti Keränen <detegr@rbx.email>

Show 22 quoted lines

> ---
>  sequencer.c                   |  7 ++++---
>  t/t3404-rebase-interactive.sh | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index fd7701c88a..0679adb639 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5178,13 +5178,12 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  		    struct string_list *commands, unsigned autosquash,
>  		    struct todo_list *todo_list)
>  {
> -	const char *shortonto, *todo_file = rebase_path_todo();
> +	const char *todo_file = rebase_path_todo();
>  	struct todo_list new_todo = TODO_LIST_INIT;
>  	struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
>  	struct object_id oid = onto->object.oid;
>  	int res;
> -
> -	shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
> +	char *shortonto;

A minor nit is that you could probably move this line up to below the 'const char *' declaration (it makes sense why you have to drop the const qualifier, though).

Show 11 quoted lines

>
>  	if (buf->len == 0) {
>  		struct todo_item *item = append_new_todo(todo_list);
> @@ -5206,8 +5205,10 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  		return error(_("nothing to do"));
>  	}
>
> +	shortonto = xstrdup(find_unique_abbrev(&oid, DEFAULT_ABBREV));
>  	res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
>  			     shortonto, flags);
> +	free(shortonto);

OK. I think of two things here:

  1. Why are we calling 'find_unique_abbrev()' instead of
     'short_commit_name()'? We already have a commit pointer, so I don't
     see any reason that we should be reimplementing that function (even
     though it is a one-liner).

  2. If we should indeed be calling 'short_commit_name()', are there
     other callers that need to do the same duplication? In other words:
     could you say a little bit more about what makes
     'complete_action()' special in this regard?

Show 15 quoted lines

>  	if (res == -1)
>  		return -1;
>  	else if (res == -2) {
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 4a7d21f898..09af16753c 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1760,6 +1760,19 @@ test_expect_success 'correct error message for commit --amend after empty pick'
>  	test_i18ngrep "middle of a rebase -- cannot amend." err
>  '
>
> +test_expect_success 'todo has correct onto hash' '
> +	write_script dump-raw.sh <<-\EOF &&
> +		cat "$1"
> +	EOF

It's too bad that you have to write your own script here, since we already have 'set_cat_todo_editor()', but it makes sense since you want to retain the comments (something which set_cat_todo_editor explicitly does not do).

So, this makes sense.

Show 9 quoted lines

> +	git checkout branch1 &&
> +	(
> +		test_set_editor "$(pwd)/dump-raw.sh" &&
> +		git rebase -i HEAD~5 >actual
> +	) &&
> +	onto=$(git rev-parse --short HEAD~5) &&
> +	test_i18ngrep "^# Rebase ..* onto $onto .*" actual
> +'
> +

This all makes sense, too, and it leaves the below test as the last one in the file, which is the right thing to do.

Show 5 quoted lines

>  # This must be the last test in this file
>  test_expect_success '$EDITOR and friends are unchanged' '
>  	test_editor_unchanged
> --
> 2.28.0

Thanks, Taylor