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

[Taylor Blau <me@ttaylorr.com>]

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).

> 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>

> ---
>  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).

>
>  	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?

>  	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.

> +	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.

>  # 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