On Tue, Aug 11, 2020 at 04:32:37PM +0100, Phillip Wood wrote:
> Hi Antti
>
> On 11/08/2020 14:13, 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.
> > 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@xxxxxxxxx>
> > Signed-off-by: Antti Keränen <detegr@rbx.email>
>
> Thanks for working on this
>
> > ---
> > 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.
>
> > 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
Good suggestion.
> > + onto=$(git rev-parse --short HEAD~5) &&
> > + test_i18ngrep "^# Rebase ..* onto $onto .*" actual
>
> we could lose the final .*
Ack, I noticed this too during my review, but apparently forgot to
comment on it. I'm puzzled by the first '..*'. If you're searching for
any non-empty string, how about '.+' instead?
> Many Thanks and Best Wishes
>
> Phillip
>
> > +'
> > +
> > # This must be the last test in this file
> > test_expect_success '$EDITOR and friends are unchanged' '
> > test_editor_unchanged
> >
Thanks,
Taylor
