{"thread":{"id":"58541","subject":"Bug Report: Duplicate condition in read_author_script (sequencer.c)","startedAt":"2022-10-03T06:39:55Z","lastAt":"2022-10-03T18:07:48Z","messageCount":11,"participants":["Michael V. Scovetta","Jeff King","Phillip Wood","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"464059","messageId":"CADG3Mza_QU+ceTUsMYxJ3PzsEqi8M98oOBAzzz0GHRJ-F7vkpA@mail.gmail.com","threadId":"58541","inReplyTo":null,"subject":"Bug Report: Duplicate condition in read_author_script (sequencer.c)","fromName":"Michael V. Scovetta","fromEmail":"michael.scovetta@gmail.com","sentAt":"2022-10-03T06:39:16Z","receivedAt":"2022-10-03T06:39:55Z","isPatch":false,"sender":{"key":"michael.scovetta@gmail.com","avatar":null},"body":"Hello!\nI noticed a duplicate condition in the read_author_script function\ndefined in sequencer.c. I reported this initially to the security\nmailing list, but folks there thought it was unlikely to be\ninteresting from a security perspective, and advised me to report it\nhere.\n\nIn commit 2a7d63a2, sequencer.c:912 looks like:\n912  if (name_i == -2)\n913      error(_(\"missing 'GIT_AUTHOR_NAME'\"));\n914  if (email_i == -2)\n915      error(_(\"missing 'GIT_AUTHOR_EMAIL'\"));\n916  if (date_i == -2)\n917      error(_(\"missing 'GIT_AUTHOR_DATE'\"));\n918  if (date_i < 0 || email_i < 0 || date_i < 0 || err)    <-- date_i\nis referenced here twice\n919      goto finish;\n\nI'm fairly sure that line 918 should be:\n918  if (name_i < 0 || email_i < 0 || date_i < 0 || err)\n\nI haven't validated this, but I suspect that if\n`rebase-merge/author-script` contained two GIT_AUTHOR_NAME fields,\nthen name_i would be set to -1 (by the error function), but control\nwouldn't flow to finish, but instead to line 920 ( *name =\nkv.items[name_i].util; ) where it would attempt to access memory\nslightly outside of items' memory space.\n\nI haven't been able to actually trigger the bug, but strongly suspect\nI'm just not familiar enough with how rebasing works under the covers.\n\nI can dig into this deeper if you'd like, but it looks like an obvious\ntypo or copy/paste error. Is this something that a maintainer would be\nable to take from here?\n\nThanks so much!!\n\nMike\n"},{"id":"464061","messageId":"YzqhEcTDwMwa8dQX@coredump.intra.peff.net","threadId":"58541","inReplyTo":"CADG3Mza_QU+ceTUsMYxJ3PzsEqi8M98oOBAzzz0GHRJ-F7vkpA@mail.gmail.com","subject":"[PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-03T08:45:05Z","receivedAt":"2022-10-03T09:00:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 02, 2022 at 11:39:16PM -0700, Michael V. Scovetta wrote:\n\n> In commit 2a7d63a2, sequencer.c:912 looks like:\n> 912  if (name_i == -2)\n> 913      error(_(\"missing 'GIT_AUTHOR_NAME'\"));\n> 914  if (email_i == -2)\n> 915      error(_(\"missing 'GIT_AUTHOR_EMAIL'\"));\n> 916  if (date_i == -2)\n> 917      error(_(\"missing 'GIT_AUTHOR_DATE'\"));\n> 918  if (date_i < 0 || email_i < 0 || date_i < 0 || err)    <-- date_i\n> is referenced here twice\n> 919      goto finish;\n> \n> I'm fairly sure that line 918 should be:\n> 918  if (name_i < 0 || email_i < 0 || date_i < 0 || err)\n\nAgreed, but +cc Phillip as the original author.\n\n> I haven't validated this, but I suspect that if\n> `rebase-merge/author-script` contained two GIT_AUTHOR_NAME fields,\n> then name_i would be set to -1 (by the error function), but control\n> wouldn't flow to finish, but instead to line 920 ( *name =\n> kv.items[name_i].util; ) where it would attempt to access memory\n> slightly outside of items' memory space.\n\nCorrect. It also happens if GIT_AUTHOR_NAME is missing.\n\n> I haven't been able to actually trigger the bug, but strongly suspect\n> I'm just not familiar enough with how rebasing works under the covers.\n\nIt's a little tricky, because we avoid writing and reading the\nauthor-script file unless necessary. An easy way to need it is to break\nwith a conflict (which writes it), and then resume with \"git rebase\n--continue\" (which reads it back while committing).\n\nHere's a patch to fix it. Thanks for your report!\n\n-- >8 --\nSubject: sequencer: detect author name errors in read_author_script()\n\nAs we parse the author-script file, we check for missing or duplicate\nlines for GIT_AUTHOR_NAME, etc. But after reading the whole file, our\nfinal error conditional checks \"date_i\" twice and \"name_i\" not at all.\nThis not only leads to us failing to abort, but we may do an\nout-of-bounds read on the string_list array.\n\nThe bug goes back to 442c36bd08 (am: improve author-script error\nreporting, 2018-10-31), though the code was soon after moved to this\nspot by bcd33ec25f (add read_author_script() to libgit, 2018-10-31).\nIt was presmably just a typo in 442c36bd08.\n\nWe'll add test coverage for all the error cases here, though only the\nGIT_AUTHOR_NAME ones fail (even in a vanilla build they to segfault\nconsistently, but certainly with SANITIZE=address).\n\nReported-by: Michael V. Scovetta <michael.scovetta@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe tests kind of feel like overkill, as this is such a specific\ncondition and I doubt we'd regress to have the same bug twice. But it\nwas nice at least to confirm the bug and the fix now.\n\n sequencer.c                    |  2 +-\n t/t3438-rebase-broken-files.sh | 53 ++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 1 deletion(-)\n create mode 100755 t/t3438-rebase-broken-files.sh\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d26ede83c4..83e0425b04 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -915,7 +915,7 @@ int read_author_script(const char *path, char **name, char **email, char **date,\n \t\terror(_(\"missing 'GIT_AUTHOR_EMAIL'\"));\n \tif (date_i == -2)\n \t\terror(_(\"missing 'GIT_AUTHOR_DATE'\"));\n-\tif (date_i < 0 || email_i < 0 || date_i < 0 || err)\n+\tif (name_i < 0 || email_i < 0 || date_i < 0 || err)\n \t\tgoto finish;\n \t*name = kv.items[name_i].util;\n \t*email = kv.items[email_i].util;\ndiff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh\nnew file mode 100755\nindex 0000000000..e68aac4b36\n--- /dev/null\n+++ b/t/t3438-rebase-broken-files.sh\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+\n+test_description='rebase behavior when on-disk files are broken'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up conflicting branches' '\n+\ttest_commit base file &&\n+\tgit checkout -b branch1 &&\n+\ttest_commit one file &&\n+\tgit checkout -b branch2 HEAD^ &&\n+\ttest_commit two file\n+'\n+\n+check_broken_author () {\n+\ttitle=$1; shift\n+\tscript=$1; shift\n+\n+\ttest_expect_success \"$title\" '\n+\t\t# create conflicted state\n+\t\ttest_when_finished \"git rebase --abort\" &&\n+\t\tgit checkout -B tmp branch2 &&\n+\t\ttest_must_fail git rebase branch1 &&\n+\n+\t\t# break author-script\n+\t\t'\"$script\"' &&\n+\n+\t\t# resolving notices broken author-script\n+\t\techo resolved >file &&\n+\t\tgit add file &&\n+\t\ttest_must_fail git rebase --continue\n+\t'\n+}\n+\n+for item in NAME EMAIL DATE\n+do\n+\tcheck_broken_author \"detect missing GIT_AUTHOR_$item\" '\n+\t\tgrep -v $item .git/rebase-merge/author-script >tmp &&\n+\t\tmv tmp .git/rebase-merge/author-script'\n+done\n+\n+for item in NAME EMAIL DATE\n+do\n+\tcheck_broken_author \"detect duplicate GIT_AUTHOR_$item\" '\n+\t\tgrep -i $item .git/rebase-merge/author-script >tmp &&\n+\t\tcat tmp >>.git/rebase-merge/author-script'\n+done\n+\n+check_broken_author 'unknown key in author-script' '\n+\techo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\" \\\n+\t\t>>.git/rebase-merge/author-script'\n+\n+\n+test_done\n-- \n2.38.0.rc2.657.gc449b89570\n\n"},{"id":"464068","messageId":"f0a5abe5-761d-4c92-68df-cfd26a414c23@dunelm.org.uk","threadId":"58541","inReplyTo":"YzqhEcTDwMwa8dQX@coredump.intra.peff.net","subject":"Re: [PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-03T09:29:24Z","receivedAt":"2022-10-03T09:36:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 03/10/2022 09:45, Jeff King wrote:\n> On Sun, Oct 02, 2022 at 11:39:16PM -0700, Michael V. Scovetta wrote:\n> .\n>> In commit 2a7d63a2, sequencer.c:912 looks like:\n>> 912  if (name_i == -2)\n>> 913      error(_(\"missing 'GIT_AUTHOR_NAME'\"));\n>> 914  if (email_i == -2)\n>> 915      error(_(\"missing 'GIT_AUTHOR_EMAIL'\"));\n>> 916  if (date_i == -2)\n>> 917      error(_(\"missing 'GIT_AUTHOR_DATE'\"));\n>> 918  if (date_i < 0 || email_i < 0 || date_i < 0 || err)    <-- date_i\n>> is referenced here twice\n>> 919      goto finish;\n>>\n>> I'm fairly sure that line 918 should be:\n>> 918  if (name_i < 0 || email_i < 0 || date_i < 0 || err)\n> \n> Agreed, but +cc Phillip as the original author.\n\nThanks, I must have got confused as I was typing.\n\n>> I haven't validated this, but I suspect that if\n>> `rebase-merge/author-script` contained two GIT_AUTHOR_NAME fields,\n>> then name_i would be set to -1 (by the error function), but control\n>> wouldn't flow to finish, but instead to line 920 ( *name =\n>> kv.items[name_i].util; ) where it would attempt to access memory\n>> slightly outside of items' memory space.\n> \n> Correct. It also happens if GIT_AUTHOR_NAME is missing.\n> \n>> I haven't been able to actually trigger the bug, but strongly suspect\n>> I'm just not familiar enough with how rebasing works under the covers.\n> \n> It's a little tricky, because we avoid writing and reading the\n> author-script file unless necessary. An easy way to need it is to break\n> with a conflict (which writes it), and then resume with \"git rebase\n> --continue\" (which reads it back while committing).\n> \n> Here's a patch to fix it. Thanks for your report!\n\nYes thank you for reporting this Michael\n\n> -- >8 --\n> Subject: sequencer: detect author name errors in read_author_script()\n> \n> As we parse the author-script file, we check for missing or duplicate\n> lines for GIT_AUTHOR_NAME, etc. But after reading the whole file, our\n> final error conditional checks \"date_i\" twice and \"name_i\" not at all.\n> This not only leads to us failing to abort, but we may do an\n> out-of-bounds read on the string_list array.\n> \n> The bug goes back to 442c36bd08 (am: improve author-script error\n> reporting, 2018-10-31), though the code was soon after moved to this\n> spot by bcd33ec25f (add read_author_script() to libgit, 2018-10-31).\n> It was presmably just a typo in 442c36bd08.\n\ns/presmbly/presumably/\n\n> We'll add test coverage for all the error cases here, though only the\n> GIT_AUTHOR_NAME ones fail (even in a vanilla build they to segfault\n\nDo you mean \"they ought to segfault\"?\n\nThanks for fixing this and going to the trouble of adding some tests.\n\nPhillip\n\n> consistently, but certainly with SANITIZE=address).\n> \n> Reported-by: Michael V. Scovetta <michael.scovetta@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The tests kind of feel like overkill, as this is such a specific\n> condition and I doubt we'd regress to have the same bug twice. But it\n> was nice at least to confirm the bug and the fix now.\n> \n>   sequencer.c                    |  2 +-\n>   t/t3438-rebase-broken-files.sh | 53 ++++++++++++++++++++++++++++++++++\n>   2 files changed, 54 insertions(+), 1 deletion(-)\n>   create mode 100755 t/t3438-rebase-broken-files.sh\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index d26ede83c4..83e0425b04 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -915,7 +915,7 @@ int read_author_script(const char *path, char **name, char **email, char **date,\n>   \t\terror(_(\"missing 'GIT_AUTHOR_EMAIL'\"));\n>   \tif (date_i == -2)\n>   \t\terror(_(\"missing 'GIT_AUTHOR_DATE'\"));\n> -\tif (date_i < 0 || email_i < 0 || date_i < 0 || err)\n> +\tif (name_i < 0 || email_i < 0 || date_i < 0 || err)\n>   \t\tgoto finish;\n>   \t*name = kv.items[name_i].util;\n>   \t*email = kv.items[email_i].util;\n> diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh\n> new file mode 100755\n> index 0000000000..e68aac4b36\n> --- /dev/null\n> +++ b/t/t3438-rebase-broken-files.sh\n> @@ -0,0 +1,53 @@\n> +#!/bin/sh\n> +\n> +test_description='rebase behavior when on-disk files are broken'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'set up conflicting branches' '\n> +\ttest_commit base file &&\n> +\tgit checkout -b branch1 &&\n> +\ttest_commit one file &&\n> +\tgit checkout -b branch2 HEAD^ &&\n> +\ttest_commit two file\n> +'\n> +\n> +check_broken_author () {\n> +\ttitle=$1; shift\n> +\tscript=$1; shift\n> +\n> +\ttest_expect_success \"$title\" '\n> +\t\t# create conflicted state\n> +\t\ttest_when_finished \"git rebase --abort\" &&\n> +\t\tgit checkout -B tmp branch2 &&\n> +\t\ttest_must_fail git rebase branch1 &&\n> +\n> +\t\t# break author-script\n> +\t\t'\"$script\"' &&\n> +\n> +\t\t# resolving notices broken author-script\n> +\t\techo resolved >file &&\n> +\t\tgit add file &&\n> +\t\ttest_must_fail git rebase --continue\n> +\t'\n> +}\n> +\n> +for item in NAME EMAIL DATE\n> +do\n> +\tcheck_broken_author \"detect missing GIT_AUTHOR_$item\" '\n> +\t\tgrep -v $item .git/rebase-merge/author-script >tmp &&\n> +\t\tmv tmp .git/rebase-merge/author-script'\n> +done\n> +\n> +for item in NAME EMAIL DATE\n> +do\n> +\tcheck_broken_author \"detect duplicate GIT_AUTHOR_$item\" '\n> +\t\tgrep -i $item .git/rebase-merge/author-script >tmp &&\n> +\t\tcat tmp >>.git/rebase-merge/author-script'\n> +done\n> +\n> +check_broken_author 'unknown key in author-script' '\n> +\techo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\" \\\n> +\t\t>>.git/rebase-merge/author-script'\n> +\n> +\n> +test_done\n"},{"id":"464069","messageId":"221003.86k05htf84.gmgdl@evledraar.gmail.com","threadId":"58541","inReplyTo":"YzqhEcTDwMwa8dQX@coredump.intra.peff.net","subject":"Re: [PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-03T09:40:52Z","receivedAt":"2022-10-03T09:46:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 03 2022, Jeff King wrote:\n\n> On Sun, Oct 02, 2022 at 11:39:16PM -0700, Michael V. Scovetta wrote:\n>\n>> In commit 2a7d63a2, sequencer.c:912 looks like:\n>> 912  if (name_i == -2)\n>> 913      error(_(\"missing 'GIT_AUTHOR_NAME'\"));\n>> 914  if (email_i == -2)\n>> 915      error(_(\"missing 'GIT_AUTHOR_EMAIL'\"));\n>> 916  if (date_i == -2)\n>> 917      error(_(\"missing 'GIT_AUTHOR_DATE'\"));\n>> 918  if (date_i < 0 || email_i < 0 || date_i < 0 || err)    <-- date_i\n>> is referenced here twice\n>> 919      goto finish;\n>> \n>> I'm fairly sure that line 918 should be:\n>> 918  if (name_i < 0 || email_i < 0 || date_i < 0 || err)\n>\n> Agreed, but +cc Phillip as the original author.\n>\n>> I haven't validated this, but I suspect that if\n>> `rebase-merge/author-script` contained two GIT_AUTHOR_NAME fields,\n>> then name_i would be set to -1 (by the error function), but control\n>> wouldn't flow to finish, but instead to line 920 ( *name =\n>> kv.items[name_i].util; ) where it would attempt to access memory\n>> slightly outside of items' memory space.\n>\n> Correct. It also happens if GIT_AUTHOR_NAME is missing.\n>\n>> I haven't been able to actually trigger the bug, but strongly suspect\n>> I'm just not familiar enough with how rebasing works under the covers.\n>\n> It's a little tricky, because we avoid writing and reading the\n> author-script file unless necessary. An easy way to need it is to break\n> with a conflict (which writes it), and then resume with \"git rebase\n> --continue\" (which reads it back while committing).\n>\n> Here's a patch to fix it. Thanks for your report!\n>\n> -- >8 --\n> Subject: sequencer: detect author name errors in read_author_script()\n>\n> As we parse the author-script file, we check for missing or duplicate\n> lines for GIT_AUTHOR_NAME, etc. But after reading the whole file, our\n> final error conditional checks \"date_i\" twice and \"name_i\" not at all.\n> This not only leads to us failing to abort, but we may do an\n> out-of-bounds read on the string_list array.\n>\n> The bug goes back to 442c36bd08 (am: improve author-script error\n> reporting, 2018-10-31), though the code was soon after moved to this\n> spot by bcd33ec25f (add read_author_script() to libgit, 2018-10-31).\n> It was presmably just a typo in 442c36bd08.\n>\n> We'll add test coverage for all the error cases here, though only the\n> GIT_AUTHOR_NAME ones fail (even in a vanilla build they to segfault\n> consistently, but certainly with SANITIZE=address).\n>\n> Reported-by: Michael V. Scovetta <michael.scovetta@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The tests kind of feel like overkill, as this is such a specific\n> condition and I doubt we'd regress to have the same bug twice. But it\n> was nice at least to confirm the bug and the fix now.\n\nHaving a regression test never feels like overkill to me :)\n\n>\n>  sequencer.c                    |  2 +-\n>  t/t3438-rebase-broken-files.sh | 53 ++++++++++++++++++++++++++++++++++\n>  2 files changed, 54 insertions(+), 1 deletion(-)\n>  create mode 100755 t/t3438-rebase-broken-files.sh\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index d26ede83c4..83e0425b04 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -915,7 +915,7 @@ int read_author_script(const char *path, char **name, char **email, char **date,\n>  \t\terror(_(\"missing 'GIT_AUTHOR_EMAIL'\"));\n>  \tif (date_i == -2)\n>  \t\terror(_(\"missing 'GIT_AUTHOR_DATE'\"));\n> -\tif (date_i < 0 || email_i < 0 || date_i < 0 || err)\n> +\tif (name_i < 0 || email_i < 0 || date_i < 0 || err)\n>  \t\tgoto finish;\n>  \t*name = kv.items[name_i].util;\n>  \t*email = kv.items[email_i].util;\n> diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh\n> new file mode 100755\n> index 0000000000..e68aac4b36\n> --- /dev/null\n> +++ b/t/t3438-rebase-broken-files.sh\n> @@ -0,0 +1,53 @@\n> +#!/bin/sh\n> +\n> +test_description='rebase behavior when on-disk files are broken'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'set up conflicting branches' '\n> +\ttest_commit base file &&\n> +\tgit checkout -b branch1 &&\n> +\ttest_commit one file &&\n> +\tgit checkout -b branch2 HEAD^ &&\n> +\ttest_commit two file\n> +'\n> +\n> +check_broken_author () {\n> +\ttitle=$1; shift\n> +\tscript=$1; shift\n> +\n> +\ttest_expect_success \"$title\" '\n> +\t\t# create conflicted state\n> +\t\ttest_when_finished \"git rebase --abort\" &&\n> +\t\tgit checkout -B tmp branch2 &&\n> +\t\ttest_must_fail git rebase branch1 &&\n> +\n> +\t\t# break author-script\n> +\t\t'\"$script\"' &&\n> +\n> +\t\t# resolving notices broken author-script\n> +\t\techo resolved >file &&\n> +\t\tgit add file &&\n> +\t\ttest_must_fail git rebase --continue\n> +\t'\n> +}\n> +\n> +for item in NAME EMAIL DATE\n> +do\n> +\tcheck_broken_author \"detect missing GIT_AUTHOR_$item\" '\n> +\t\tgrep -v $item .git/rebase-merge/author-script >tmp &&\n> +\t\tmv tmp .git/rebase-merge/author-script'\n> +done\n> +\n> +for item in NAME EMAIL DATE\n> +do\n> +\tcheck_broken_author \"detect duplicate GIT_AUTHOR_$item\" '\n> +\t\tgrep -i $item .git/rebase-merge/author-script >tmp &&\n> +\t\tcat tmp >>.git/rebase-merge/author-script'\n> +done\n> +\n> +check_broken_author 'unknown key in author-script' '\n> +\techo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\" \\\n> +\t\t>>.git/rebase-merge/author-script'\n> +\n> +\n> +test_done\n\nMaybe I just have too much PTSD from dealing with shell quoting issues\nwith this '\"$script\"\" pattern when you need to pass arguments with\nspaces in it, or even quotes. Although it probably won't ever be an\nissue here.\n\nBut in this case just passing the \"script\" on stdin nicely avoids any\nfuture quoting issues, maybe this would be good to squash in?:\n\ndiff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh\nindex e68aac4b36d..6911e145f08 100755\n--- a/t/t3438-rebase-broken-files.sh\n+++ b/t/t3438-rebase-broken-files.sh\n@@ -12,17 +12,19 @@ test_expect_success 'set up conflicting branches' '\n '\n \n check_broken_author () {\n-\ttitle=$1; shift\n-\tscript=$1; shift\n+\ttitle=$1 && shift &&\n \n-\ttest_expect_success \"$title\" '\n+\t# Avoid quoting issues\n+\twrite_script script.sh &&\n+\n+\ttest_expect_success \"$title of '$script'\" '\n \t\t# create conflicted state\n \t\ttest_when_finished \"git rebase --abort\" &&\n \t\tgit checkout -B tmp branch2 &&\n \t\ttest_must_fail git rebase branch1 &&\n \n-\t\t# break author-script\n-\t\t'\"$script\"' &&\n+\t\t./script.sh >tmp &&\n+\t\tmv tmp .git/rebase-merge/author-script &&\n \n \t\t# resolving notices broken author-script\n \t\techo resolved >file &&\n@@ -33,21 +35,22 @@ check_broken_author () {\n \n for item in NAME EMAIL DATE\n do\n-\tcheck_broken_author \"detect missing GIT_AUTHOR_$item\" '\n-\t\tgrep -v $item .git/rebase-merge/author-script >tmp &&\n-\t\tmv tmp .git/rebase-merge/author-script'\n+\tcheck_broken_author \"detect missing GIT_AUTHOR_$item\" <<-EOF\n+\tgrep -v $item .git/rebase-merge/author-script\n+\tEOF\n done\n \n for item in NAME EMAIL DATE\n do\n-\tcheck_broken_author \"detect duplicate GIT_AUTHOR_$item\" '\n-\t\tgrep -i $item .git/rebase-merge/author-script >tmp &&\n-\t\tcat tmp >>.git/rebase-merge/author-script'\n+\tcheck_broken_author \"detect duplicate GIT_AUTHOR_$item\" <<-EOF\n+\tcat .git/rebase-merge/author-script &&\n+\tgrep -i $item .git/rebase-merge/author-script\n+\tEOF\n done\n \n-check_broken_author 'unknown key in author-script' '\n-\techo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\" \\\n-\t\t>>.git/rebase-merge/author-script'\n-\n+check_broken_author 'unknown key in author-script' <<-EOF\n+cat .git/rebase-merge/author-script &&\n+echo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\"\n+EOF\n \n test_done\n\n"},{"id":"464083","messageId":"xmqq8rlwx3vx.fsf@gitster.g","threadId":"58541","inReplyTo":"YzqhEcTDwMwa8dQX@coredump.intra.peff.net","subject":"Re: [PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-03T16:34:26Z","receivedAt":"2022-10-03T16:34:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Oct 02, 2022 at 11:39:16PM -0700, Michael V. Scovetta wrote:\n>\n>> In commit 2a7d63a2, sequencer.c:912 looks like:\n>> 912  if (name_i == -2)\n>> 913      error(_(\"missing 'GIT_AUTHOR_NAME'\"));\n>> 914  if (email_i == -2)\n>> 915      error(_(\"missing 'GIT_AUTHOR_EMAIL'\"));\n>> 916  if (date_i == -2)\n>> 917      error(_(\"missing 'GIT_AUTHOR_DATE'\"));\n>> 918  if (date_i < 0 || email_i < 0 || date_i < 0 || err)    <-- date_i\n>> is referenced here twice\n>> 919      goto finish;\n>> \n>> I'm fairly sure that line 918 should be:\n>> 918  if (name_i < 0 || email_i < 0 || date_i < 0 || err)\n>\n> Agreed, but +cc Phillip as the original author.\n>\n>> I haven't validated this, but I suspect that if\n>> `rebase-merge/author-script` contained two GIT_AUTHOR_NAME fields,\n>> then name_i would be set to -1 (by the error function), but control\n>> wouldn't flow to finish, but instead to line 920 ( *name =\n>> kv.items[name_i].util; ) where it would attempt to access memory\n>> slightly outside of items' memory space.\n>\n> Correct. It also happens if GIT_AUTHOR_NAME is missing.\n>\n>> I haven't been able to actually trigger the bug, but strongly suspect\n>> I'm just not familiar enough with how rebasing works under the covers.\n>\n> It's a little tricky, because we avoid writing and reading the\n> author-script file unless necessary. An easy way to need it is to break\n> with a conflict (which writes it), and then resume with \"git rebase\n> --continue\" (which reads it back while committing).\n>\n> Here's a patch to fix it. Thanks for your report!\n\nAnd thanks for your fix ;-)\n"},{"id":"464087","messageId":"YzsYtXr6QM7LpExV@coredump.intra.peff.net","threadId":"58541","inReplyTo":"f0a5abe5-761d-4c92-68df-cfd26a414c23@dunelm.org.uk","subject":"Re: [PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-03T17:15:33Z","receivedAt":"2022-10-03T17:16:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2022 at 10:29:24AM +0100, Phillip Wood wrote:\n\n> > The bug goes back to 442c36bd08 (am: improve author-script error\n> > reporting, 2018-10-31), though the code was soon after moved to this\n> > spot by bcd33ec25f (add read_author_script() to libgit, 2018-10-31).\n> > It was presmably just a typo in 442c36bd08.\n> \n> s/presmbly/presumably/\n\nYep.\n\n> > We'll add test coverage for all the error cases here, though only the\n> > GIT_AUTHOR_NAME ones fail (even in a vanilla build they to segfault\n> \n> Do you mean \"they ought to segfault\"?\n\nI think it's just s/to//. I.e., \"even in a vanilla build they segfault\nconsistently\".\n\nI rewrote that sentence a few times trying to make it as not-confusing\nas possible, but managed to leave in an editing error anyway. ;)\n\n-Peff\n"},{"id":"464091","messageId":"YzsbiRcCEaJUGwdp@coredump.intra.peff.net","threadId":"58541","inReplyTo":"221003.86k05htf84.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-03T17:27:37Z","receivedAt":"2022-10-03T17:28:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2022 at 11:40:52AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > +check_broken_author 'unknown key in author-script' '\n> > +\techo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\" \\\n> > +\t\t>>.git/rebase-merge/author-script'\n> > +\n> > +\n> > +test_done\n> \n> Maybe I just have too much PTSD from dealing with shell quoting issues\n> with this '\"$script\"\" pattern when you need to pass arguments with\n> spaces in it, or even quotes. Although it probably won't ever be an\n> issue here.\n\nQuoting-wise, it's the exact same as passing the snippet to\ntest_expect_success that we already do. Which, granted, is full of\nhorrors, but is well understood within our code base. The \"$script\" is\nevaluated when we pass the snippet to test_expect_success, which sees\nthe expansion along with the rest of the script. The double-quotes\naround it ensure that whitespace is retained.\n\nThere is one extra quirk here, which is that it must not end with a\nnewline, since we stick \"&&\" on it.\n\n> +\t# Avoid quoting issues\n> +\twrite_script script.sh &&\n> +\n> +\ttest_expect_success \"$title of '$script'\" '\n>  \t\t# create conflicted state\n>  \t\ttest_when_finished \"git rebase --abort\" &&\n>  \t\tgit checkout -B tmp branch2 &&\n>  \t\ttest_must_fail git rebase branch1 &&\n>  \n> -\t\t# break author-script\n> -\t\t'\"$script\"' &&\n> +\t\t./script.sh >tmp &&\n> +\t\tmv tmp .git/rebase-merge/author-script &&\n\nOne of my goals was that the script be expanded inline so that\nverbose-mode showed what the test was actually doing. But this hides it\nbehind a vague \"script.sh\". Maybe that's not important enough to merit\nthe admitted ugliness of the loop+function we have.\n\nIf we give that up, then a better option is probably to put the\nbefore/after parts in functions, like:\n\ndiff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh\nindex e68aac4b36..b92a3ce46b 100755\n--- a/t/t3438-rebase-broken-files.sh\n+++ b/t/t3438-rebase-broken-files.sh\n@@ -11,43 +11,49 @@ test_expect_success 'set up conflicting branches' '\n \ttest_commit two file\n '\n \n-check_broken_author () {\n-\ttitle=$1; shift\n-\tscript=$1; shift\n-\n-\ttest_expect_success \"$title\" '\n-\t\t# create conflicted state\n-\t\ttest_when_finished \"git rebase --abort\" &&\n-\t\tgit checkout -B tmp branch2 &&\n-\t\ttest_must_fail git rebase branch1 &&\n-\n-\t\t# break author-script\n-\t\t'\"$script\"' &&\n-\n-\t\t# resolving notices broken author-script\n-\t\techo resolved >file &&\n-\t\tgit add file &&\n-\t\ttest_must_fail git rebase --continue\n-\t'\n+create_conflict () {\n+\ttest_when_finished \"git rebase --abort\" &&\n+\tgit checkout -B tmp branch2 &&\n+\ttest_must_fail git rebase branch1\n+}\n+\n+check_resolve_fails () {\n+\techo resolved >file &&\n+\tgit add file &&\n+\ttest_must_fail git rebase --continue\n }\n \n for item in NAME EMAIL DATE\n do\n-\tcheck_broken_author \"detect missing GIT_AUTHOR_$item\" '\n+\ttest_expect_success \"detect missing GIT_AUTHOR_$item\" '\n+\t\tcreate_conflict &&\n+\n \t\tgrep -v $item .git/rebase-merge/author-script >tmp &&\n-\t\tmv tmp .git/rebase-merge/author-script'\n+\t\tmv tmp .git/rebase-merge/author-script &&\n+\n+\t\tcheck_resolve_fails\n+\t'\n done\n \n for item in NAME EMAIL DATE\n do\n-\tcheck_broken_author \"detect duplicate GIT_AUTHOR_$item\" '\n+\ttest_expect_success \"detect duplicate GIT_AUTHOR_$item\" '\n+\t\tcreate_conflict &&\n+\n \t\tgrep -i $item .git/rebase-merge/author-script >tmp &&\n-\t\tcat tmp >>.git/rebase-merge/author-script'\n+\t\tcat tmp >>.git/rebase-merge/author-script &&\n+\n+\t\tcheck_resolve_fails\n+\t'\n done\n \n-check_broken_author 'unknown key in author-script' '\n+test_expect_success 'unknown key in author-script' '\n+\tcreate_conflict &&\n+\n \techo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\" \\\n-\t\t>>.git/rebase-merge/author-script'\n+\t\t>>.git/rebase-merge/author-script &&\n \n+\tcheck_resolve_fails\n+'\n \n test_done\n\nThat makes the boilerplate shorter in the \"-v\" output but focuses on the\nactual modification that breaks the author-script.\n\n-Peff\n"},{"id":"464092","messageId":"YzsdRuD2CdJFdNVG@coredump.intra.peff.net","threadId":"58541","inReplyTo":"YzqhEcTDwMwa8dQX@coredump.intra.peff.net","subject":"Re: [PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-03T17:35:02Z","receivedAt":"2022-10-03T17:36:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2022 at 04:45:06AM -0400, Jeff King wrote:\n\n> > I haven't been able to actually trigger the bug, but strongly suspect\n> > I'm just not familiar enough with how rebasing works under the covers.\n> \n> It's a little tricky, because we avoid writing and reading the\n> author-script file unless necessary. An easy way to need it is to break\n> with a conflict (which writes it), and then resume with \"git rebase\n> --continue\" (which reads it back while committing).\n> \n> Here's a patch to fix it. Thanks for your report!\n\nAnd here's a v2 based on the feedback received. The one-liner fix is the\nsame, but it tries to be a little less clever when constructing the\ntests and has 200% more typo fixes in the commit message. Thanks Phillip\nand Ævar for review.\n\n-- >8 --\nSubject: [PATCH] sequencer: detect author name errors in read_author_script()\n\nAs we parse the author-script file, we check for missing or duplicate\nlines for GIT_AUTHOR_NAME, etc. But after reading the whole file, our\nfinal error conditional checks \"date_i\" twice and \"name_i\" not at all.\nThis not only leads to us failing to abort, but we may do an\nout-of-bounds read on the string_list array.\n\nThe bug goes back to 442c36bd08 (am: improve author-script error\nreporting, 2018-10-31), though the code was soon after moved to this\nspot by bcd33ec25f (add read_author_script() to libgit, 2018-10-31).\nIt was presumably just a typo in 442c36bd08.\n\nWe'll add test coverage for all the error cases here, though only the\nGIT_AUTHOR_NAME ones fail (even in a vanilla build they segfault\nconsistently, but certainly with SANITIZE=address).\n\nReported-by: Michael V. Scovetta <michael.scovetta@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sequencer.c                    |  2 +-\n t/t3438-rebase-broken-files.sh | 59 ++++++++++++++++++++++++++++++++++\n 2 files changed, 60 insertions(+), 1 deletion(-)\n create mode 100755 t/t3438-rebase-broken-files.sh\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d26ede83c4..83e0425b04 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -915,7 +915,7 @@ int read_author_script(const char *path, char **name, char **email, char **date,\n \t\terror(_(\"missing 'GIT_AUTHOR_EMAIL'\"));\n \tif (date_i == -2)\n \t\terror(_(\"missing 'GIT_AUTHOR_DATE'\"));\n-\tif (date_i < 0 || email_i < 0 || date_i < 0 || err)\n+\tif (name_i < 0 || email_i < 0 || date_i < 0 || err)\n \t\tgoto finish;\n \t*name = kv.items[name_i].util;\n \t*email = kv.items[email_i].util;\ndiff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh\nnew file mode 100755\nindex 0000000000..b92a3ce46b\n--- /dev/null\n+++ b/t/t3438-rebase-broken-files.sh\n@@ -0,0 +1,59 @@\n+#!/bin/sh\n+\n+test_description='rebase behavior when on-disk files are broken'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up conflicting branches' '\n+\ttest_commit base file &&\n+\tgit checkout -b branch1 &&\n+\ttest_commit one file &&\n+\tgit checkout -b branch2 HEAD^ &&\n+\ttest_commit two file\n+'\n+\n+create_conflict () {\n+\ttest_when_finished \"git rebase --abort\" &&\n+\tgit checkout -B tmp branch2 &&\n+\ttest_must_fail git rebase branch1\n+}\n+\n+check_resolve_fails () {\n+\techo resolved >file &&\n+\tgit add file &&\n+\ttest_must_fail git rebase --continue\n+}\n+\n+for item in NAME EMAIL DATE\n+do\n+\ttest_expect_success \"detect missing GIT_AUTHOR_$item\" '\n+\t\tcreate_conflict &&\n+\n+\t\tgrep -v $item .git/rebase-merge/author-script >tmp &&\n+\t\tmv tmp .git/rebase-merge/author-script &&\n+\n+\t\tcheck_resolve_fails\n+\t'\n+done\n+\n+for item in NAME EMAIL DATE\n+do\n+\ttest_expect_success \"detect duplicate GIT_AUTHOR_$item\" '\n+\t\tcreate_conflict &&\n+\n+\t\tgrep -i $item .git/rebase-merge/author-script >tmp &&\n+\t\tcat tmp >>.git/rebase-merge/author-script &&\n+\n+\t\tcheck_resolve_fails\n+\t'\n+done\n+\n+test_expect_success 'unknown key in author-script' '\n+\tcreate_conflict &&\n+\n+\techo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\" \\\n+\t\t>>.git/rebase-merge/author-script &&\n+\n+\tcheck_resolve_fails\n+'\n+\n+test_done\n-- \n2.38.0.rc2.659.g50b8df2659\n\n"},{"id":"464093","messageId":"YzseUy8bRT0LN2Wp@coredump.intra.peff.net","threadId":"58541","inReplyTo":"YzsbiRcCEaJUGwdp@coredump.intra.peff.net","subject":"Re: [PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-03T17:39:31Z","receivedAt":"2022-10-03T17:40:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2022 at 01:27:37PM -0400, Jeff King wrote:\n\n> -check_broken_author 'unknown key in author-script' '\n> +test_expect_success 'unknown key in author-script' '\n> +\tcreate_conflict &&\n> +\n>  \techo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\" \\\n> -\t\t>>.git/rebase-merge/author-script'\n> +\t\t>>.git/rebase-merge/author-script &&\n>  \n> +\tcheck_resolve_fails\n> +'\n>  \n>  test_done\n> \n> That makes the boilerplate shorter in the \"-v\" output but focuses on the\n> actual modification that breaks the author-script.\n\nNote that we do still keep the ${SQ} bits here. They're necessary for\nthe same reason: before and after a snippet is being passed through a\nvariable. Whereas in yours we'd use stdin. I _do_ like that approach in\ngeneral, but it is unlike the rest of the test suite. Maybe it's worth\nresurrecting:\n\n  https://lore.kernel.org/git/YHDUg6ZR5vu93kGm@coredump.intra.peff.net/\n\n?\n\n-Peff\n"},{"id":"464094","messageId":"xmqq7d1gu6jy.fsf@gitster.g","threadId":"58541","inReplyTo":"YzseUy8bRT0LN2Wp@coredump.intra.peff.net","subject":"Re: [PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-03T18:05:05Z","receivedAt":"2022-10-03T18:05:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Oct 03, 2022 at 01:27:37PM -0400, Jeff King wrote:\n>\n>> -check_broken_author 'unknown key in author-script' '\n>> +test_expect_success 'unknown key in author-script' '\n>> +\tcreate_conflict &&\n>> +\n>>  \techo \"GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}\" \\\n>> -\t\t>>.git/rebase-merge/author-script'\n>> +\t\t>>.git/rebase-merge/author-script &&\n>>  \n>> +\tcheck_resolve_fails\n>> +'\n>>  \n>>  test_done\n>> \n>> That makes the boilerplate shorter in the \"-v\" output but focuses on the\n>> actual modification that breaks the author-script.\n>\n> Note that we do still keep the ${SQ} bits here. They're necessary for\n> the same reason: before and after a snippet is being passed through a\n> variable. Whereas in yours we'd use stdin. I _do_ like that approach in\n> general, but it is unlike the rest of the test suite. Maybe it's worth\n> resurrecting:\n>\n>   https://lore.kernel.org/git/YHDUg6ZR5vu93kGm@coredump.intra.peff.net/\n>\n> ?\n\nYeah, it is indeed quite tempting.\n"},{"id":"464095","messageId":"xmqq35c4u6fp.fsf@gitster.g","threadId":"58541","inReplyTo":"YzsdRuD2CdJFdNVG@coredump.intra.peff.net","subject":"Re: [PATCH] sequencer: detect author name errors in read_author_script()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-03T18:07:38Z","receivedAt":"2022-10-03T18:07:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\nJeff King <peff@peff.net> writes:\n\n> On Mon, Oct 03, 2022 at 04:45:06AM -0400, Jeff King wrote:\n>\n>> > I haven't been able to actually trigger the bug, but strongly suspect\n>> > I'm just not familiar enough with how rebasing works under the covers.\n>> \n>> It's a little tricky, because we avoid writing and reading the\n>> author-script file unless necessary. An easy way to need it is to break\n>> with a conflict (which writes it), and then resume with \"git rebase\n>> --continue\" (which reads it back while committing).\n>> \n>> Here's a patch to fix it. Thanks for your report!\n>\n> And here's a v2 based on the feedback received. The one-liner fix is the\n> same, but it tries to be a little less clever when constructing the\n> tests and has 200% more typo fixes in the commit message. Thanks Phillip\n> and Ævar for review.\n\nThe create-conflict & try-resolve pair does make it easier to see\nwhat goes on in these tests.\n\nWill queue.\n\nThanks.  \n"}]}