Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Aug 11, 2020, 15:32 UTC
- Message-ID
- <ebdc0bc7-f48e-9f38-328d-b1181ac974d7@gmail.com>
- In-Reply-To
- <20200811131313.3349582-1-detegr@rbx.email>
Hi Antti
On 11/08/2020 14:13, Antti Keränen wrote:
Show 9 quoted lines
> '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. > 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>
Thanks for working on this
Show 15 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();I'm not sure it's worth rearranging these lines. It probably does not matter but we could do
+ char shortonto[GIT_MAX_HEXSZ + 1];
and then later call find_unique_abbrev_r() instead so we don't have to worry about freeing shortonto.
Show 38 quoted lines
> 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;
>
> 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);
> 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
> + git checkout branch1 &&
> + (
> + test_set_editor "$(pwd)/dump-raw.sh" &&
> + git rebase -i HEAD~5 >actual
> + ) &&Thanks for taking the trouble to add a test, I think all the lines above could be simplified to
GIT_SEQUENCE_EDITOR=cat git rebase -i HEAD~5 branch1 >actual
> + onto=$(git rev-parse --short HEAD~5) && > + test_i18ngrep "^# Rebase ..* onto $onto .*" actual
we could lose the final .*
Many Thanks and Best Wishes
Phillip
Show 6 quoted lines
> +' > + > # This must be the last test in this file > test_expect_success '$EDITOR and friends are unchanged' ' > test_editor_unchanged >