{"thread":{"id":"54028","subject":"[PATCH v2] rebase -i: Fix possibly wrong onto hash in todo","startedAt":"2020-08-12T18:35:06Z","lastAt":"2020-08-13T14:38:21Z","messageCount":4,"participants":["Antti Keränen","Junio C Hamano","Alban Gruin","Phillip Wood"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"403484","messageId":"20200812183326.224782-1-detegr@rbx.email","threadId":"54028","inReplyTo":null,"subject":"[PATCH v2] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Antti Keränen","fromEmail":"detegr@rbx.email","sentAt":"2020-08-12T18:33:27Z","receivedAt":"2020-08-12T18:35:06Z","isPatch":true,"sender":{"key":"detegr@rbx.email","avatar":"https://avatars.githubusercontent.com/u/724433?v=4"},"body":"'todo_list_write_to_file' may overwrite the static buffer, originating\nfrom 'find_unique_abbrev', that was used to store the short commit hash\n'c' for \"# Rebase a..b onto c\" message in the todo editor. This is\nbecause the buffer that is returned from 'find_unique_abbrev' is valid\nuntil 4 more calls to `find_unique_abbrev` are made.\n\nAs 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased\ncommit, the hash for 'c' is overwritten if there are 4 or more commits\nin the rebase. This behavior has been broken since its introduction.\n\nFix by storing the short onto commit hash in a different buffer that\nremains valid, before calling 'todo_list_write_to_file'.\n\nFound-by: Jussi Keränen \nSigned-off-by: Antti Keränen \n---\n sequencer.c | 5 +++--\n t/t3404-rebase-interactive.sh | 6 ++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex fd7701c88a..e2007dbb8c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5178,13 +5178,14 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\t struct string_list *commands, unsigned autosquash,\n \t\t struct todo_list *todo_list)\n {\n-\tconst char *shortonto, *todo_file = rebase_path_todo();\n+\tchar shortonto[GIT_MAX_HEXSZ + 1];\n+\tconst char *todo_file = rebase_path_todo();\n \tstruct todo_list new_todo = TODO_LIST_INIT;\n \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n \tstruct object_id oid = onto->object.oid;\n \tint res;\n \n-\tshortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);\n+\tfind_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);\n \n \tif (buf->len == 0) {\n \t\tstruct todo_item *item = append_new_todo(todo_list);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 4a7d21f898..1b4fa0843e 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1760,6 +1760,12 @@ test_expect_success 'correct error message for commit --amend after empty pick'\n \ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n '\n \n+test_expect_success 'todo has correct onto hash' '\n+\tGIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual &&\n+\tonto=$(git rev-parse --short HEAD~4) &&\n+\ttest_i18ngrep \"^# Rebase ..* onto $onto\" actual\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\n-- \n2.27.0.395.g84249cff14\n\n"},{"id":"403505","messageId":"xmqq8sej618z.fsf@gitster.c.googlers.com","threadId":"54028","inReplyTo":"20200812183326.224782-1-detegr@rbx.email","subject":"Re: [PATCH v2] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T20:46:36Z","receivedAt":"2020-08-12T20:46:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antti Keränen writes:\n\n> 'todo_list_write_to_file' may overwrite the static buffer, originating\n> from 'find_unique_abbrev', that was used to store the short commit hash\n> 'c' for \"# Rebase a..b onto c\" message in the todo editor. This is\n> because the buffer that is returned from 'find_unique_abbrev' is valid\n> until 4 more calls to `find_unique_abbrev` are made.\n>\n> As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased\n> commit, the hash for 'c' is overwritten if there are 4 or more commits\n> in the rebase. This behavior has been broken since its introduction.\n>\n> Fix by storing the short onto commit hash in a different buffer that\n> remains valid, before calling 'todo_list_write_to_file'.\n>\n> Found-by: Jussi Keränen \n> Signed-off-by: Antti Keränen \n> ---\n\nLooking good.\n\n> sequencer.c | 5 +++--\n> t/t3404-rebase-interactive.sh | 6 ++++++\n> 2 files changed, 9 insertions(+), 2 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index fd7701c88a..e2007dbb8c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5178,13 +5178,14 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\t struct string_list *commands, unsigned autosquash,\n> \t\t struct todo_list *todo_list)\n> {\n> -\tconst char *shortonto, *todo_file = rebase_path_todo();\n> +\tchar shortonto[GIT_MAX_HEXSZ + 1];\n> +\tconst char *todo_file = rebase_path_todo();\n> \tstruct todo_list new_todo = TODO_LIST_INIT;\n> \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n> \tstruct object_id oid = onto->object.oid;\n> \tint res;\n> \n> -\tshortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);\n> +\tfind_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);\n> \n> \tif (buf->len == 0) {\n> \t\tstruct todo_item *item = append_new_todo(todo_list);\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 4a7d21f898..1b4fa0843e 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1760,6 +1760,12 @@ test_expect_success 'correct error message for commit --amend after empty pick'\n> \ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n> '\n> \n> +test_expect_success 'todo has correct onto hash' '\n> +\tGIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual &&\n> +\tonto=$(git rev-parse --short HEAD~4) &&\n> +\ttest_i18ngrep \"^# Rebase ..* onto $onto\" actual\n> +'\n> +\n> # This must be the last test in this file\n> test_expect_success '$EDITOR and friends are unchanged' '\n> \ttest_editor_unchanged\n"},{"id":"403537","messageId":"3ba6c005-e71f-3a0d-46ba-a90a68e88d3e@gmail.com","threadId":"54028","inReplyTo":"20200812183326.224782-1-detegr@rbx.email","subject":"Re: [PATCH v2] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2020-08-13T10:41:31Z","receivedAt":"2020-08-13T10:41:42Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Antti,\n\nLe 12/08/2020 à 20:33, Antti Keränen a écrit :\n> 'todo_list_write_to_file' may overwrite the static buffer, originating\n> from 'find_unique_abbrev', that was used to store the short commit hash\n> 'c' for \"# Rebase a..b onto c\" message in the todo editor. This is\n> because the buffer that is returned from 'find_unique_abbrev' is valid\n> until 4 more calls to `find_unique_abbrev` are made.\n> \n> As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased\n> commit, the hash for 'c' is overwritten if there are 4 or more commits\n> in the rebase. This behavior has been broken since its introduction.\n> \n> Fix by storing the short onto commit hash in a different buffer that\n> remains valid, before calling 'todo_list_write_to_file'.\n> \n> Found-by: Jussi Keränen \n> Signed-off-by: Antti Keränen \n> ---\n> sequencer.c | 5 +++--\n> t/t3404-rebase-interactive.sh | 6 ++++++\n> 2 files changed, 9 insertions(+), 2 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index fd7701c88a..e2007dbb8c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5178,13 +5178,14 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\t struct string_list *commands, unsigned autosquash,\n> \t\t struct todo_list *todo_list)\n> {\n> -\tconst char *shortonto, *todo_file = rebase_path_todo();\n> +\tchar shortonto[GIT_MAX_HEXSZ + 1];\n> +\tconst char *todo_file = rebase_path_todo();\n> \tstruct todo_list new_todo = TODO_LIST_INIT;\n> \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n> \tstruct object_id oid = onto->object.oid;\n> \tint res;\n> \n> -\tshortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);\n> +\tfind_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);\n> \n> \tif (buf->len == 0) {\n> \t\tstruct todo_item *item = append_new_todo(todo_list);\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 4a7d21f898..1b4fa0843e 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1760,6 +1760,12 @@ test_expect_success 'correct error message for commit --amend after empty pick'\n> \ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n> '\n> \n> +test_expect_success 'todo has correct onto hash' '\n> +\tGIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual &&\n> +\tonto=$(git rev-parse --short HEAD~4) &&\n> +\ttest_i18ngrep \"^# Rebase ..* onto $onto\" actual\n> +'\n> +\n> # This must be the last test in this file\n> test_expect_success '$EDITOR and friends are unchanged' '\n> \ttest_editor_unchanged\n> \n\nLooks good to me.\n\n Acked-by: Alban Gruin \n\nThis makes me wonder if it's worth to do the same change in\ntodo_list_to_strbuf(). #leftoverbits, perhaps?\n\nCheers,\nAlban\n\n"},{"id":"403546","messageId":"51ba9326-26fb-103b-d04b-acf50c2b9104@gmail.com","threadId":"54028","inReplyTo":"3ba6c005-e71f-3a0d-46ba-a90a68e88d3e@gmail.com","subject":"Re: [PATCH v2] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-08-13T14:38:12Z","receivedAt":"2020-08-13T14:38:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Antti & Alban\n\nOn 13/08/2020 11:41, Alban Gruin wrote:\n> Hi Antti,\n> \n> Le 12/08/2020 à 20:33, Antti Keränen a écrit :\n>> 'todo_list_write_to_file' may overwrite the static buffer, originating\n>> from 'find_unique_abbrev', that was used to store the short commit hash\n>> 'c' for \"# Rebase a..b onto c\" message in the todo editor. This is\n>> because the buffer that is returned from 'find_unique_abbrev' is valid\n>> until 4 more calls to `find_unique_abbrev` are made.\n>>\n>> As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased\n>> commit, the hash for 'c' is overwritten if there are 4 or more commits\n>> in the rebase. This behavior has been broken since its introduction.\n>>\n>> Fix by storing the short onto commit hash in a different buffer that\n>> remains valid, before calling 'todo_list_write_to_file'.\n>>\n>> Found-by: Jussi Keränen \n>> Signed-off-by: Antti Keränen \n>> ---\n>> sequencer.c | 5 +++--\n>> t/t3404-rebase-interactive.sh | 6 ++++++\n>> 2 files changed, 9 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index fd7701c88a..e2007dbb8c 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -5178,13 +5178,14 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>> \t\t struct string_list *commands, unsigned autosquash,\n>> \t\t struct todo_list *todo_list)\n>> {\n>> -\tconst char *shortonto, *todo_file = rebase_path_todo();\n>> +\tchar shortonto[GIT_MAX_HEXSZ + 1];\n>> +\tconst char *todo_file = rebase_path_todo();\n>> \tstruct todo_list new_todo = TODO_LIST_INIT;\n>> \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n>> \tstruct object_id oid = onto->object.oid;\n>> \tint res;\n>> \n>> -\tshortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);\n>> +\tfind_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);\n>> \n>> \tif (buf->len == 0) {\n>> \t\tstruct todo_item *item = append_new_todo(todo_list);\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 4a7d21f898..1b4fa0843e 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -1760,6 +1760,12 @@ test_expect_success 'correct error message for commit --amend after empty pick'\n>> \ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n>> '\n>> \n>> +test_expect_success 'todo has correct onto hash' '\n>> +\tGIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual &&\n>> +\tonto=$(git rev-parse --short HEAD~4) &&\n>> +\ttest_i18ngrep \"^# Rebase ..* onto $onto\" actual\n>> +'\n>> +\n>> # This must be the last test in this file\n>> test_expect_success '$EDITOR and friends are unchanged' '\n>> \ttest_editor_unchanged\n>>\n> \n> Looks good to me.\n\nIt looks good to me too, thanks Antti\n\n> Acked-by: Alban Gruin \n> \n> This makes me wonder if it's worth to do the same change in\n> todo_list_to_strbuf(). #leftoverbits, perhaps?\n\nIn todo_list_to_strbuf() we append the short oid to an strbuf before we \ncall find_unique_abbrev() again so I don't think it should be a problem \nthere\n\nBest Wishes\n\nPhillip\n\n> Cheers,\n> Alban\n> \n"}]}