| Message ID | 20200812183326.224782-1-detegr@rbx.email (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] rebase -i: Fix possibly wrong onto hash in todo | expand |
Antti Keränen <detegr@rbx.email> writes: > '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. This is > because the buffer that is returned from 'find_unique_abbrev' is valid > until 4 more calls to `find_unique_abbrev` are made. > > As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased > commit, the hash for 'c' is overwritten if there are 4 or more commits > in the rebase. This behavior has been broken since its introduction. > > Fix by storing the short onto commit hash in a different buffer that > remains valid, before calling 'todo_list_write_to_file'. > > Found-by: Jussi Keränen <jussike@gmail.com> > Signed-off-by: Antti Keränen <detegr@rbx.email> > --- Looking good. > sequencer.c | 5 +++-- > t/t3404-rebase-interactive.sh | 6 ++++++ > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index fd7701c88a..e2007dbb8c 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -5178,13 +5178,14 @@ 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(); > + char shortonto[GIT_MAX_HEXSZ + 1]; > + 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); > + find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV); > > if (buf->len == 0) { > struct todo_item *item = append_new_todo(todo_list); > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 4a7d21f898..1b4fa0843e 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1760,6 +1760,12 @@ 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' ' > + GIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual && > + onto=$(git rev-parse --short HEAD~4) && > + test_i18ngrep "^# Rebase ..* onto $onto" actual > +' > + > # This must be the last test in this file > test_expect_success '$EDITOR and friends are unchanged' ' > test_editor_unchanged
Hi Antti, Le 12/08/2020 à 20:33, Antti Keränen a écrit : > '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. This is > because the buffer that is returned from 'find_unique_abbrev' is valid > until 4 more calls to `find_unique_abbrev` are made. > > As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased > commit, the hash for 'c' is overwritten if there are 4 or more commits > in the rebase. This behavior has been broken since its introduction. > > Fix by storing the short onto commit hash in a different buffer that > remains valid, before calling 'todo_list_write_to_file'. > > Found-by: Jussi Keränen <jussike@gmail.com> > Signed-off-by: Antti Keränen <detegr@rbx.email> > --- > sequencer.c | 5 +++-- > t/t3404-rebase-interactive.sh | 6 ++++++ > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index fd7701c88a..e2007dbb8c 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -5178,13 +5178,14 @@ 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(); > + char shortonto[GIT_MAX_HEXSZ + 1]; > + 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); > + find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV); > > if (buf->len == 0) { > struct todo_item *item = append_new_todo(todo_list); > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 4a7d21f898..1b4fa0843e 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1760,6 +1760,12 @@ 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' ' > + GIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual && > + onto=$(git rev-parse --short HEAD~4) && > + test_i18ngrep "^# Rebase ..* onto $onto" actual > +' > + > # This must be the last test in this file > test_expect_success '$EDITOR and friends are unchanged' ' > test_editor_unchanged > Looks good to me. Acked-by: Alban Gruin <alban.gruin@gmail.com> This makes me wonder if it's worth to do the same change in todo_list_to_strbuf(). #leftoverbits, perhaps? Cheers, Alban
Hi Antti & Alban On 13/08/2020 11:41, Alban Gruin wrote: > Hi Antti, > > Le 12/08/2020 à 20:33, Antti Keränen a écrit : >> '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. This is >> because the buffer that is returned from 'find_unique_abbrev' is valid >> until 4 more calls to `find_unique_abbrev` are made. >> >> As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased >> commit, the hash for 'c' is overwritten if there are 4 or more commits >> in the rebase. This behavior has been broken since its introduction. >> >> Fix by storing the short onto commit hash in a different buffer that >> remains valid, before calling 'todo_list_write_to_file'. >> >> Found-by: Jussi Keränen <jussike@gmail.com> >> Signed-off-by: Antti Keränen <detegr@rbx.email> >> --- >> sequencer.c | 5 +++-- >> t/t3404-rebase-interactive.sh | 6 ++++++ >> 2 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index fd7701c88a..e2007dbb8c 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -5178,13 +5178,14 @@ 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(); >> + char shortonto[GIT_MAX_HEXSZ + 1]; >> + 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); >> + find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV); >> >> if (buf->len == 0) { >> struct todo_item *item = append_new_todo(todo_list); >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh >> index 4a7d21f898..1b4fa0843e 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> @@ -1760,6 +1760,12 @@ 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' ' >> + GIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual && >> + onto=$(git rev-parse --short HEAD~4) && >> + test_i18ngrep "^# Rebase ..* onto $onto" actual >> +' >> + >> # This must be the last test in this file >> test_expect_success '$EDITOR and friends are unchanged' ' >> test_editor_unchanged >> > > Looks good to me. It looks good to me too, thanks Antti > Acked-by: Alban Gruin <alban.gruin@gmail.com> > > This makes me wonder if it's worth to do the same change in > todo_list_to_strbuf(). #leftoverbits, perhaps? In todo_list_to_strbuf() we append the short oid to an strbuf before we call find_unique_abbrev() again so I don't think it should be a problem there Best Wishes Phillip > Cheers, > Alban >
diff --git a/sequencer.c b/sequencer.c index fd7701c88a..e2007dbb8c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5178,13 +5178,14 @@ 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(); + char shortonto[GIT_MAX_HEXSZ + 1]; + 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); + find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV); if (buf->len == 0) { struct todo_item *item = append_new_todo(todo_list); diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 4a7d21f898..1b4fa0843e 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1760,6 +1760,12 @@ 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' ' + GIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual && + onto=$(git rev-parse --short HEAD~4) && + test_i18ngrep "^# Rebase ..* onto $onto" actual +' + # This must be the last test in this file test_expect_success '$EDITOR and friends are unchanged' ' test_editor_unchanged
'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. This is because the buffer that is returned from 'find_unique_abbrev' is valid until 4 more calls to `find_unique_abbrev` are made. As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased commit, the hash for 'c' is overwritten if there are 4 or more commits in the rebase. This behavior has been broken since its introduction. Fix by storing the short onto commit hash in a different buffer that remains valid, before calling 'todo_list_write_to_file'. Found-by: Jussi Keränen <jussike@gmail.com> Signed-off-by: Antti Keränen <detegr@rbx.email> --- sequencer.c | 5 +++-- t/t3404-rebase-interactive.sh | 6 ++++++ 2 files changed, 9 insertions(+), 2 deletions(-)