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

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

[Antti Keränen]

'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
-- 
2.27.0.395.g84249cff14

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

[Junio C Hamano]

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

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

[Alban Gruin]

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

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

[Phillip Wood]

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
> 

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

[Antti Keränen]

'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>
Acked-by: Alban Gruin <alban.gruin@gmail.com>
---
 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
-- 
2.27.0.395.g84249cff14