{"thread":{"id":"54019","subject":"[PATCH] rebase -i: Fix possibly wrong onto hash in todo","startedAt":"2020-08-11T14:10:44Z","lastAt":"2020-08-12T19:40:12Z","messageCount":13,"participants":["Antti Keränen","Taylor Blau","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"403355","messageId":"20200811131313.3349582-1-detegr@rbx.email","threadId":"54019","inReplyTo":null,"subject":"[PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Antti Keränen","fromEmail":"detegr@rbx.email","sentAt":"2020-08-11T13:13:15Z","receivedAt":"2020-08-11T14:10:44Z","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.\n\nFix by duplicating the string before usage, so subsequent calls to\n'find_unique_abbrev' or other functions calling 'hash_to_hex_algop_r'\ncan't overwrite the buffer.\n\nFound-by: Jussi Keränen <jussike@gmail.com>\nSigned-off-by: Antti Keränen <detegr@rbx.email>\n---\n sequencer.c                   |  7 ++++---\n t/t3404-rebase-interactive.sh | 13 +++++++++++++\n 2 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex fd7701c88a..0679adb639 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5178,13 +5178,12 @@ 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+\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+\tchar *shortonto;\n \n \tif (buf->len == 0) {\n \t\tstruct todo_item *item = append_new_todo(todo_list);\n@@ -5206,8 +5205,10 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn error(_(\"nothing to do\"));\n \t}\n \n+\tshortonto = xstrdup(find_unique_abbrev(&oid, DEFAULT_ABBREV));\n \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n \t\t\t     shortonto, flags);\n+\tfree(shortonto);\n \tif (res == -1)\n \t\treturn -1;\n \telse if (res == -2) {\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 4a7d21f898..09af16753c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1760,6 +1760,19 @@ 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+\twrite_script dump-raw.sh <<-\\EOF &&\n+\t\tcat \"$1\"\n+\tEOF\n+\tgit checkout branch1 &&\n+\t(\n+\t\ttest_set_editor \"$(pwd)/dump-raw.sh\" &&\n+\t\tgit rebase -i HEAD~5 >actual\n+\t) &&\n+\tonto=$(git rev-parse --short HEAD~5) &&\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.28.0\n\n"},{"id":"403357","messageId":"20200811152832.GC19871@syl.lan","threadId":"54019","inReplyTo":"20200811131313.3349582-1-detegr@rbx.email","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-11T15:28:32Z","receivedAt":"2020-08-11T15:28:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Antti,\n\nThanks for working on this. I have a few thoughts below, but I think\nthat this is on the right track.\n\nOn Tue, Aug 10, 2020 at 04:13:15PM +0300, Antti Keränen wrote:\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.\n\nIt would be great to know the commit that regressed, or if this has\nalways been the case. I'm not sure if you'll have a ton of luck\nbisecting, since you indicate that this overwrite *may* occur (that\nmakes me think that it doesn't always happen, so your efforts to bisect\nthe change may be noisy).\n\n> Fix by duplicating the string before usage, so subsequent calls to\n> 'find_unique_abbrev' or other functions calling 'hash_to_hex_algop_r'\n> can't overwrite the buffer.\n>\n> Found-by: Jussi Keränen <jussike@gmail.com>\n> Signed-off-by: Antti Keränen <detegr@rbx.email>\n\n> ---\n>  sequencer.c                   |  7 ++++---\n>  t/t3404-rebase-interactive.sh | 13 +++++++++++++\n>  2 files changed, 17 insertions(+), 3 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index fd7701c88a..0679adb639 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5178,13 +5178,12 @@ 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> +\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> +\tchar *shortonto;\n\nA minor nit is that you could probably move this line up to below the\n'const char *' declaration (it makes sense why you have to drop the\nconst qualifier, though).\n\n>\n>  \tif (buf->len == 0) {\n>  \t\tstruct todo_item *item = append_new_todo(todo_list);\n> @@ -5206,8 +5205,10 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>  \t\treturn error(_(\"nothing to do\"));\n>  \t}\n>\n> +\tshortonto = xstrdup(find_unique_abbrev(&oid, DEFAULT_ABBREV));\n>  \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n>  \t\t\t     shortonto, flags);\n> +\tfree(shortonto);\n\nOK. I think of two things here:\n\n  1. Why are we calling 'find_unique_abbrev()' instead of\n     'short_commit_name()'? We already have a commit pointer, so I don't\n     see any reason that we should be reimplementing that function (even\n     though it is a one-liner).\n\n  2. If we should indeed be calling 'short_commit_name()', are there\n     other callers that need to do the same duplication? In other words:\n     could you say a little bit more about what makes\n     'complete_action()' special in this regard?\n\n>  \tif (res == -1)\n>  \t\treturn -1;\n>  \telse if (res == -2) {\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 4a7d21f898..09af16753c 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1760,6 +1760,19 @@ 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> +\twrite_script dump-raw.sh <<-\\EOF &&\n> +\t\tcat \"$1\"\n> +\tEOF\n\nIt's too bad that you have to write your own script here, since we\nalready have 'set_cat_todo_editor()', but it makes sense since you want\nto retain the comments (something which set_cat_todo_editor explicitly\ndoes not do).\n\nSo, this makes sense.\n\n> +\tgit checkout branch1 &&\n> +\t(\n> +\t\ttest_set_editor \"$(pwd)/dump-raw.sh\" &&\n> +\t\tgit rebase -i HEAD~5 >actual\n> +\t) &&\n> +\tonto=$(git rev-parse --short HEAD~5) &&\n> +\ttest_i18ngrep \"^# Rebase ..* onto $onto .*\" actual\n> +'\n> +\n\nThis all makes sense, too, and it leaves the below test as the last one\nin the file, which is the right thing to do.\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> 2.28.0\n\nThanks,\nTaylor\n"},{"id":"403359","messageId":"ebdc0bc7-f48e-9f38-328d-b1181ac974d7@gmail.com","threadId":"54019","inReplyTo":"20200811131313.3349582-1-detegr@rbx.email","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-08-11T15:32:37Z","receivedAt":"2020-08-11T15:32:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Antti\n\nOn 11/08/2020 14:13, Antti Keränen wrote:\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.\n> Fix by duplicating the string before usage, so subsequent calls to\n> 'find_unique_abbrev' or other functions calling 'hash_to_hex_algop_r'\n> can't overwrite the buffer.\n> \n> Found-by: Jussi Keränen <jussike@gmail.com>\n> Signed-off-by: Antti Keränen <detegr@rbx.email>\n\nThanks for working on this\n\n> ---\n>   sequencer.c                   |  7 ++++---\n>   t/t3404-rebase-interactive.sh | 13 +++++++++++++\n>   2 files changed, 17 insertions(+), 3 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index fd7701c88a..0679adb639 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5178,13 +5178,12 @@ 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> +\tconst char *todo_file = rebase_path_todo();\n\nI'm not sure it's worth rearranging these lines. It probably does not \nmatter but we could do\n\n+\tchar shortonto[GIT_MAX_HEXSZ + 1];\n\nand then later call find_unique_abbrev_r() instead so we don't have to \nworry about freeing shortonto.\n\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> +\tchar *shortonto;\n>   \n>   \tif (buf->len == 0) {\n>   \t\tstruct todo_item *item = append_new_todo(todo_list);\n> @@ -5206,8 +5205,10 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>   \t\treturn error(_(\"nothing to do\"));\n>   \t}\n>   \n> +\tshortonto = xstrdup(find_unique_abbrev(&oid, DEFAULT_ABBREV));\n>   \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n>   \t\t\t     shortonto, flags);\n> +\tfree(shortonto);\n>   \tif (res == -1)\n>   \t\treturn -1;\n>   \telse if (res == -2) {\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 4a7d21f898..09af16753c 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1760,6 +1760,19 @@ 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> +\twrite_script dump-raw.sh <<-\\EOF &&\n> +\t\tcat \"$1\"\n> +\tEOF\n> +\tgit checkout branch1 &&\n> +\t(\n> +\t\ttest_set_editor \"$(pwd)/dump-raw.sh\" &&\n> +\t\tgit rebase -i HEAD~5 >actual\n> +\t) &&\n\nThanks for taking the trouble to add a test, I think all the lines above \ncould be simplified to\n\n\tGIT_SEQUENCE_EDITOR=cat git rebase -i HEAD~5 branch1 >actual\n\n> +\tonto=$(git rev-parse --short HEAD~5) &&\n> +\ttest_i18ngrep \"^# Rebase ..* onto $onto .*\" actual\n\nwe could lose the final .*\n\nMany Thanks and Best Wishes\n\nPhillip\n\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"},{"id":"403360","messageId":"20200811153621.GD19871@syl.lan","threadId":"54019","inReplyTo":"ebdc0bc7-f48e-9f38-328d-b1181ac974d7@gmail.com","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-11T15:36:21Z","receivedAt":"2020-08-11T15:36:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 11, 2020 at 04:32:37PM +0100, Phillip Wood wrote:\n> Hi Antti\n>\n> On 11/08/2020 14:13, Antti Keränen wrote:\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.\n> > Fix by duplicating the string before usage, so subsequent calls to\n> > 'find_unique_abbrev' or other functions calling 'hash_to_hex_algop_r'\n> > can't overwrite the buffer.\n> >\n> > Found-by: Jussi Keränen <jussike@gmail.com>\n> > Signed-off-by: Antti Keränen <detegr@rbx.email>\n>\n> Thanks for working on this\n>\n> > ---\n> >   sequencer.c                   |  7 ++++---\n> >   t/t3404-rebase-interactive.sh | 13 +++++++++++++\n> >   2 files changed, 17 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index fd7701c88a..0679adb639 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -5178,13 +5178,12 @@ 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> > +\tconst char *todo_file = rebase_path_todo();\n>\n> I'm not sure it's worth rearranging these lines. It probably does not matter\n> but we could do\n>\n> +\tchar shortonto[GIT_MAX_HEXSZ + 1];\n>\n> and then later call find_unique_abbrev_r() instead so we don't have to worry\n> about freeing shortonto.\n>\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> > +\tchar *shortonto;\n> >   \tif (buf->len == 0) {\n> >   \t\tstruct todo_item *item = append_new_todo(todo_list);\n> > @@ -5206,8 +5205,10 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> >   \t\treturn error(_(\"nothing to do\"));\n> >   \t}\n> > +\tshortonto = xstrdup(find_unique_abbrev(&oid, DEFAULT_ABBREV));\n> >   \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n> >   \t\t\t     shortonto, flags);\n> > +\tfree(shortonto);\n> >   \tif (res == -1)\n> >   \t\treturn -1;\n> >   \telse if (res == -2) {\n> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> > index 4a7d21f898..09af16753c 100755\n> > --- a/t/t3404-rebase-interactive.sh\n> > +++ b/t/t3404-rebase-interactive.sh\n> > @@ -1760,6 +1760,19 @@ test_expect_success 'correct error message for commit --amend after empty pick'\n> >   \ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n> >   '\n> > +test_expect_success 'todo has correct onto hash' '\n> > +\twrite_script dump-raw.sh <<-\\EOF &&\n> > +\t\tcat \"$1\"\n> > +\tEOF\n> > +\tgit checkout branch1 &&\n> > +\t(\n> > +\t\ttest_set_editor \"$(pwd)/dump-raw.sh\" &&\n> > +\t\tgit rebase -i HEAD~5 >actual\n> > +\t) &&\n>\n> Thanks for taking the trouble to add a test, I think all the lines above\n> could be simplified to\n>\n> \tGIT_SEQUENCE_EDITOR=cat git rebase -i HEAD~5 branch1 >actual\n\nGood suggestion.\n\n> > +\tonto=$(git rev-parse --short HEAD~5) &&\n> > +\ttest_i18ngrep \"^# Rebase ..* onto $onto .*\" actual\n>\n> we could lose the final .*\n\nAck, I noticed this too during my review, but apparently forgot to\ncomment on it. I'm puzzled by the first '..*'. If you're searching for\nany non-empty string, how about '.+' instead?\n\n> Many Thanks and Best Wishes\n>\n> Phillip\n>\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> >\nThanks,\nTaylor\n"},{"id":"403367","messageId":"20200811181537.qt7jkoxy7qn3k7mo@haukka.localdomain","threadId":"54019","inReplyTo":"20200811153621.GD19871@syl.lan","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Antti Keränen","fromEmail":"antti@keraset.fi","sentAt":"2020-08-11T18:15:37Z","receivedAt":"2020-08-11T18:16:05Z","isPatch":true,"sender":{"key":"antti@keraset.fi","avatar":null},"body":"Hi Phillip and Taylor.\n\nOn Tue, Aug 11, 2020 at 04:32:37PM +0100, Phillip Wood wrote:\n> > I'm not sure it's worth rearranging these lines. It probably does not matter\n> > but we could do\n> >\n> > +\tchar shortonto[GIT_MAX_HEXSZ + 1];\n> >\n> > and then later call find_unique_abbrev_r() instead so we don't have to worry\n> > about freeing shortonto.\n\nI like this. I'll change the code, if the rest of the patch is okay.\n\n> > Thanks for taking the trouble to add a test, I think all the lines above\n> > could be simplified to\n> >\n> > \tGIT_SEQUENCE_EDITOR=cat git rebase -i HEAD~5 branch1 >actual\n\nNice! :) There was a test called 'todo count' which I mimicked. This\nobviously is much cleaner solution, thanks.\n\n> > we could lose the final .*\n\nAck.\n\nOn Tue, Aug 11, 2020 at 11:36:21AM -0400, Taylor Blau wrote:\n> Ack, I noticed this too during my review, but apparently forgot to\n> comment on it. I'm puzzled by the first '..*'. If you're searching for\n> any non-empty string, how about '.+' instead?\n\nThat's true. Good point. I pretty much copy&pasted the 'todo count' test\nso I didn't give this much thought. I'll fix this.\n\n-- \nAntti\n"},{"id":"403368","messageId":"20200811181017.jjbryeiepcsutnki@haukka.localdomain","threadId":"54019","inReplyTo":"20200811152832.GC19871@syl.lan","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Antti Keränen","fromEmail":"antti@keraset.fi","sentAt":"2020-08-11T18:10:17Z","receivedAt":"2020-08-11T18:18:52Z","isPatch":true,"sender":{"key":"antti@keraset.fi","avatar":null},"body":"Hi Taylor,\n\nOn Tue, Aug 11, 2020 at 11:28:32AM -0400, Taylor Blau wrote:\n> Hi Antti,\n> \n> Thanks for working on this. I have a few thoughts below, but I think\n> that this is on the right track.\n> \n> On Tue, Aug 10, 2020 at 04:13:15PM +0300, Antti Keränen wrote:\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.\n> \n> It would be great to know the commit that regressed, or if this has\n> always been the case. I'm not sure if you'll have a ton of luck\n> bisecting, since you indicate that this overwrite *may* occur (that\n> makes me think that it doesn't always happen, so your efforts to bisect\n> the change may be noisy).\n\nThis was introduced when interactive rebase was refactored. The first\ncommit is 94bcad797966b6a3490bc6806d3ee3eed54da9d9. Would you like to\nhave this information in the commit message also?\n\nThe reason I stated it may occur is that the buffer find_unique_abbrev\nreturns is valid for 4 more calls. So the rebase must have 4 commits in\nit before this happens.\n\n> > Fix by duplicating the string before usage, so subsequent calls to\n> > 'find_unique_abbrev' or other functions calling 'hash_to_hex_algop_r'\n> > can't overwrite the buffer.\n> >\n> > Found-by: Jussi Keränen <jussike@gmail.com>\n> > Signed-off-by: Antti Keränen <detegr@rbx.email>\n> \n> > ---\n> >  sequencer.c                   |  7 ++++---\n> >  t/t3404-rebase-interactive.sh | 13 +++++++++++++\n> >  2 files changed, 17 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index fd7701c88a..0679adb639 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -5178,13 +5178,12 @@ 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> > +\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> > +\tchar *shortonto;\n> \n> A minor nit is that you could probably move this line up to below the\n> 'const char *' declaration (it makes sense why you have to drop the\n> const qualifier, though).\n\nAck.\n\n> > +\tshortonto = xstrdup(find_unique_abbrev(&oid, DEFAULT_ABBREV));\n> >  \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n> >  \t\t\t     shortonto, flags);\n> > +\tfree(shortonto);\n> \n> OK. I think of two things here:\n> \n>   1. Why are we calling 'find_unique_abbrev()' instead of\n>      'short_commit_name()'? We already have a commit pointer, so I don't\n>      see any reason that we should be reimplementing that function (even\n>      though it is a one-liner).\n> \n>   2. If we should indeed be calling 'short_commit_name()', are there\n>      other callers that need to do the same duplication? In other words:\n>      could you say a little bit more about what makes\n>      'complete_action()' special in this regard?\n\nGood question. The code used find_unique_abbrev and as I'm new to the\ncode base I did not notice that short_commit_name essentially does the\nsame thing.\n\nThe reason what makes this special is that this code first calls\nfind_unique_abbrev which, as we know, stores its information to a static\nbuffer. The pointer is stored in a variable and it is assumed it does\nnot change.\nAfterwards, it calls edit_todo_list that reuses the buffer before it\naccesses the shortonto string, meaning that if we have enough commits in\nthe rebase, the shortonto string is overwritten.\n\nActually, now that you asked, I noticed that orig_head in\ncomplete_action (actually comes from get_revision_ranges in\nbuiltin/rebase.c) may be affected as well. Though, I'm not sure whether\nthere are enough calls to find_unique_abbrev in the execution path when\nit is being used to actually cause a rewritten buffer. It wouldn't hurt\nto duplicate it too just in case.\n\n-- \nAntti\n"},{"id":"403369","messageId":"20200811182417.GA33504@syl.lan","threadId":"54019","inReplyTo":"20200811181017.jjbryeiepcsutnki@haukka.localdomain","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-11T18:24:17Z","receivedAt":"2020-08-11T18:24:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 11, 2020 at 09:10:17PM +0300, Antti Keränen wrote:\n> Hi Taylor,\n>\n> On Tue, Aug 11, 2020 at 11:28:32AM -0400, Taylor Blau wrote:\n> > Hi Antti,\n> >\n> > Thanks for working on this. I have a few thoughts below, but I think\n> > that this is on the right track.\n> >\n> > On Tue, Aug 10, 2020 at 04:13:15PM +0300, Antti Keränen wrote:\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.\n> >\n> > It would be great to know the commit that regressed, or if this has\n> > always been the case. I'm not sure if you'll have a ton of luck\n> > bisecting, since you indicate that this overwrite *may* occur (that\n> > makes me think that it doesn't always happen, so your efforts to bisect\n> > the change may be noisy).\n>\n> This was introduced when interactive rebase was refactored. The first\n> commit is 94bcad797966b6a3490bc6806d3ee3eed54da9d9. Would you like to\n> have this information in the commit message also?\n\nIf it's been broken since the start, I think having a reference to\n94bcad7979 isn't interesting, but \"this behavior has been broken since\nits introduction\" is.\n\n> The reason I stated it may occur is that the buffer find_unique_abbrev\n> returns is valid for 4 more calls. So the rebase must have 4 commits in\n> it before this happens.\n\n:-). Interesting detail, too, and probably worth noting in the commit\nmessage.\n\n>\n> > > Fix by duplicating the string before usage, so subsequent calls to\n> > > 'find_unique_abbrev' or other functions calling 'hash_to_hex_algop_r'\n> > > can't overwrite the buffer.\n> > >\n> > > Found-by: Jussi Keränen <jussike@gmail.com>\n> > > Signed-off-by: Antti Keränen <detegr@rbx.email>\n> >\n> > > ---\n> > >  sequencer.c                   |  7 ++++---\n> > >  t/t3404-rebase-interactive.sh | 13 +++++++++++++\n> > >  2 files changed, 17 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/sequencer.c b/sequencer.c\n> > > index fd7701c88a..0679adb639 100644\n> > > --- a/sequencer.c\n> > > +++ b/sequencer.c\n> > > @@ -5178,13 +5178,12 @@ 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> > > +\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> > > +\tchar *shortonto;\n> >\n> > A minor nit is that you could probably move this line up to below the\n> > 'const char *' declaration (it makes sense why you have to drop the\n> > const qualifier, though).\n>\n> Ack.\n\nFeel free to discard this suggestion in favor of what Phillip is\nproposing, though.\n\n> > > +\tshortonto = xstrdup(find_unique_abbrev(&oid, DEFAULT_ABBREV));\n> > >  \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n> > >  \t\t\t     shortonto, flags);\n> > > +\tfree(shortonto);\n> >\n> > OK. I think of two things here:\n> >\n> >   1. Why are we calling 'find_unique_abbrev()' instead of\n> >      'short_commit_name()'? We already have a commit pointer, so I don't\n> >      see any reason that we should be reimplementing that function (even\n> >      though it is a one-liner).\n> >\n> >   2. If we should indeed be calling 'short_commit_name()', are there\n> >      other callers that need to do the same duplication? In other words:\n> >      could you say a little bit more about what makes\n> >      'complete_action()' special in this regard?\n>\n> Good question. The code used find_unique_abbrev and as I'm new to the\n> code base I did not notice that short_commit_name essentially does the\n> same thing.\n>\n> The reason what makes this special is that this code first calls\n> find_unique_abbrev which, as we know, stores its information to a static\n> buffer. The pointer is stored in a variable and it is assumed it does\n> not change.\n> Afterwards, it calls edit_todo_list that reuses the buffer before it\n> accesses the shortonto string, meaning that if we have enough commits in\n> the rebase, the shortonto string is overwritten.\n\nThanks for the detailed analysis. Makes sense.\n\n> Actually, now that you asked, I noticed that orig_head in\n> complete_action (actually comes from get_revision_ranges in\n> builtin/rebase.c) may be affected as well. Though, I'm not sure whether\n> there are enough calls to find_unique_abbrev in the execution path when\n> it is being used to actually cause a rewritten buffer. It wouldn't hurt\n> to duplicate it too just in case.\n>\n> --\n> Antti\n\nThanks,\nTaylor\n"},{"id":"403375","messageId":"xmqqsgct9fi1.fsf@gitster.c.googlers.com","threadId":"54019","inReplyTo":"20200811181537.qt7jkoxy7qn3k7mo@haukka.localdomain","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-11T18:58:14Z","receivedAt":"2020-08-11T18:58:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antti Keränen <antti@keraset.fi> writes:\n\n> On Tue, Aug 11, 2020 at 11:36:21AM -0400, Taylor Blau wrote:\n>> Ack, I noticed this too during my review, but apparently forgot to\n>> comment on it. I'm puzzled by the first '..*'. If you're searching for\n>> any non-empty string, how about '.+' instead?\n>\n> That's true. Good point. I pretty much copy&pasted the 'todo count' test\n> so I didn't give this much thought. I'll fix this.\n\nPlease don't shorten ..* into .+ if you are writing a portable sed\nscript---stick to the BRE.\n\n"},{"id":"403379","messageId":"20200811190139.GB34058@syl.lan","threadId":"54019","inReplyTo":"xmqqsgct9fi1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-11T19:01:39Z","receivedAt":"2020-08-11T19:01:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 11, 2020 at 11:58:14AM -0700, Junio C Hamano wrote:\n> Antti Keränen <antti@keraset.fi> writes:\n>\n> > On Tue, Aug 11, 2020 at 11:36:21AM -0400, Taylor Blau wrote:\n> >> Ack, I noticed this too during my review, but apparently forgot to\n> >> comment on it. I'm puzzled by the first '..*'. If you're searching for\n> >> any non-empty string, how about '.+' instead?\n> >\n> > That's true. Good point. I pretty much copy&pasted the 'todo count' test\n> > so I didn't give this much thought. I'll fix this.\n>\n> Please don't shorten ..* into .+ if you are writing a portable sed\n> script---stick to the BRE.\n\nSure, and sorry -- I didn't know that we cared about the difference\nbetween BRE and ERE. Do you prefer ..* over .\\+? Both should be\nsupported in BRE, if I'm reading [1] correctly.\n\nThanks,\nTaylor\n\n[1]: https://www.gnu.org/software/sed/manual/html_node/BRE-vs-ERE.html#BRE-vs-ERE\n"},{"id":"403380","messageId":"xmqqft8t9f55.fsf@gitster.c.googlers.com","threadId":"54019","inReplyTo":"20200811190139.GB34058@syl.lan","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-11T19:05:58Z","receivedAt":"2020-08-11T19:06:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Tue, Aug 11, 2020 at 11:58:14AM -0700, Junio C Hamano wrote:\n>> Antti Keränen <antti@keraset.fi> writes:\n>>\n>> > On Tue, Aug 11, 2020 at 11:36:21AM -0400, Taylor Blau wrote:\n>> >> Ack, I noticed this too during my review, but apparently forgot to\n>> >> comment on it. I'm puzzled by the first '..*'. If you're searching for\n>> >> any non-empty string, how about '.+' instead?\n>> >\n>> > That's true. Good point. I pretty much copy&pasted the 'todo count' test\n>> > so I didn't give this much thought. I'll fix this.\n>>\n>> Please don't shorten ..* into .+ if you are writing a portable sed\n>> script---stick to the BRE.\n>\n> Sure, and sorry -- I didn't know that we cared about the difference\n> between BRE and ERE. Do you prefer ..* over .\\+? Both should be\n> supported in BRE, if I'm reading [1] correctly.\n\nI thought that BRE only commands taking backslash-quoted ERE was GNU\nextension?\n\nLook for \"stick to a subset of BRE\" in Documention/CodingGuidelines;\nwe may need to update the document to raise the baseline to match\nthe reality of year 2020, though.\n\n"},{"id":"403454","messageId":"eb3ecbc0-84aa-70fa-1893-3b16b21989a8@gmail.com","threadId":"54019","inReplyTo":"20200811181537.qt7jkoxy7qn3k7mo@haukka.localdomain","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-08-12T13:59:48Z","receivedAt":"2020-08-12T13:59:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Antti\n\nOn 11/08/2020 19:15, Antti Keränen wrote:\n> Hi Phillip and Taylor.\n> \n> On Tue, Aug 11, 2020 at 04:32:37PM +0100, Phillip Wood wrote:\n>>> I'm not sure it's worth rearranging these lines. It probably does not matter\n>>> but we could do\n>>>\n>>> +\tchar shortonto[GIT_MAX_HEXSZ + 1];\n>>>\n>>> and then later call find_unique_abbrev_r() instead so we don't have to worry\n>>> about freeing shortonto.\n> \n> I like this. I'll change the code, if the rest of the patch is okay.\n\nThanks great\n\nThanks\n\nPhillip\n\n> \n>>> Thanks for taking the trouble to add a test, I think all the lines above\n>>> could be simplified to\n>>>\n>>> \tGIT_SEQUENCE_EDITOR=cat git rebase -i HEAD~5 branch1 >actual\n> \n> Nice! :) There was a test called 'todo count' which I mimicked. This\n> obviously is much cleaner solution, thanks.\n> \n>>> we could lose the final .*\n> \n> Ack.\n> \n> On Tue, Aug 11, 2020 at 11:36:21AM -0400, Taylor Blau wrote:\n>> Ack, I noticed this too during my review, but apparently forgot to\n>> comment on it. I'm puzzled by the first '..*'. If you're searching for\n>> any non-empty string, how about '.+' instead?\n> \n> That's true. Good point. I pretty much copy&pasted the 'todo count' test\n> so I didn't give this much thought. I'll fix this.\n> \n"},{"id":"403455","messageId":"20200812140352.GC74542@syl.lan","threadId":"54019","inReplyTo":"xmqqft8t9f55.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-12T14:03:52Z","receivedAt":"2020-08-12T14:03:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 11, 2020 at 12:05:58PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > On Tue, Aug 11, 2020 at 11:58:14AM -0700, Junio C Hamano wrote:\n> >> Antti Keränen <antti@keraset.fi> writes:\n> >>\n> >> > On Tue, Aug 11, 2020 at 11:36:21AM -0400, Taylor Blau wrote:\n> >> >> Ack, I noticed this too during my review, but apparently forgot to\n> >> >> comment on it. I'm puzzled by the first '..*'. If you're searching for\n> >> >> any non-empty string, how about '.+' instead?\n> >> >\n> >> > That's true. Good point. I pretty much copy&pasted the 'todo count' test\n> >> > so I didn't give this much thought. I'll fix this.\n> >>\n> >> Please don't shorten ..* into .+ if you are writing a portable sed\n> >> script---stick to the BRE.\n> >\n> > Sure, and sorry -- I didn't know that we cared about the difference\n> > between BRE and ERE. Do you prefer ..* over .\\+? Both should be\n> > supported in BRE, if I'm reading [1] correctly.\n>\n> I thought that BRE only commands taking backslash-quoted ERE was GNU\n> extension?\n\n*Grumble*, it's not anywhere in POSIX:\n\n  https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html\n\n...making this a GNU-ism.\n\n> Look for \"stick to a subset of BRE\" in Documention/CodingGuidelines;\n> we may need to update the document to raise the baseline to match\n> the reality of year 2020, though.\n\nSo, I think the \"reality of year 2020\" is that we still write '..*'\ninstead of '.\\+'.\n\nThanks,\nTaylor\n"},{"id":"403493","messageId":"xmqqpn7v64bx.fsf@gitster.c.googlers.com","threadId":"54019","inReplyTo":"20200812140352.GC74542@syl.lan","subject":"Re: [PATCH] rebase -i: Fix possibly wrong onto hash in todo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T19:40:02Z","receivedAt":"2020-08-12T19:40:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> *Grumble*, it's not anywhere in POSIX:\n>\n>   https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html\n>\n> ...making this a GNU-ism.\n>\n>> Look for \"stick to a subset of BRE\" in Documention/CodingGuidelines;\n>> we may need to update the document to raise the baseline to match\n>> the reality of year 2020, though.\n>\n> So, I think the \"reality of year 2020\" is that we still write '..*'\n> instead of '.\\+'.\n\nAnother interesting thing I found is this piece in \"man sed\" on\nLinux (hence GNU):\n\n    REGULAR EXPRESSIONS\n           ...\n           The -E option switches to using extended regular  expressions  instead;\n           it  has  been  supported  for  years by GNU sed, and is now included in\n           POSIX.\n\nBut the \"sed\" documentation [*1*] that matches what you cited above (The\nOpen Group Base Specifications Issue 7, 2018 edition) does not\nmention \"-E\", \"-r\" nor \"--regexp-extended\".\n\n\n[Reference]\n\n*1*\nhttps://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html#tag_20_116_02\n"}]}