From: Phillip Wood Date: Mon, 03 Oct 2022 09:29:24 GMT Subject: Re: [PATCH] sequencer: detect author name errors in read_author_script() Message-ID: In-Reply-To: Hi Peff On 03/10/2022 09:45, Jeff King wrote: > On Sun, Oct 02, 2022 at 11:39:16PM -0700, Michael V. Scovetta wrote: > . >> In commit 2a7d63a2, sequencer.c:912 looks like: >> 912 if (name_i == -2) >> 913 error(_("missing 'GIT_AUTHOR_NAME'")); >> 914 if (email_i == -2) >> 915 error(_("missing 'GIT_AUTHOR_EMAIL'")); >> 916 if (date_i == -2) >> 917 error(_("missing 'GIT_AUTHOR_DATE'")); >> 918 if (date_i < 0 || email_i < 0 || date_i < 0 || err) <-- date_i >> is referenced here twice >> 919 goto finish; >> >> I'm fairly sure that line 918 should be: >> 918 if (name_i < 0 || email_i < 0 || date_i < 0 || err) > > Agreed, but +cc Phillip as the original author. Thanks, I must have got confused as I was typing. >> I haven't validated this, but I suspect that if >> `rebase-merge/author-script` contained two GIT_AUTHOR_NAME fields, >> then name_i would be set to -1 (by the error function), but control >> wouldn't flow to finish, but instead to line 920 ( *name = >> kv.items[name_i].util; ) where it would attempt to access memory >> slightly outside of items' memory space. > > Correct. It also happens if GIT_AUTHOR_NAME is missing. > >> I haven't been able to actually trigger the bug, but strongly suspect >> I'm just not familiar enough with how rebasing works under the covers. > > It's a little tricky, because we avoid writing and reading the > author-script file unless necessary. An easy way to need it is to break > with a conflict (which writes it), and then resume with "git rebase > --continue" (which reads it back while committing). > > Here's a patch to fix it. Thanks for your report! Yes thank you for reporting this Michael > -- >8 -- > Subject: sequencer: detect author name errors in read_author_script() > > As we parse the author-script file, we check for missing or duplicate > lines for GIT_AUTHOR_NAME, etc. But after reading the whole file, our > final error conditional checks "date_i" twice and "name_i" not at all. > This not only leads to us failing to abort, but we may do an > out-of-bounds read on the string_list array. > > The bug goes back to 442c36bd08 (am: improve author-script error > reporting, 2018-10-31), though the code was soon after moved to this > spot by bcd33ec25f (add read_author_script() to libgit, 2018-10-31). > It was presmably just a typo in 442c36bd08. s/presmbly/presumably/ > We'll add test coverage for all the error cases here, though only the > GIT_AUTHOR_NAME ones fail (even in a vanilla build they to segfault Do you mean "they ought to segfault"? Thanks for fixing this and going to the trouble of adding some tests. Phillip > consistently, but certainly with SANITIZE=address). > > Reported-by: Michael V. Scovetta > Signed-off-by: Jeff King > --- > The tests kind of feel like overkill, as this is such a specific > condition and I doubt we'd regress to have the same bug twice. But it > was nice at least to confirm the bug and the fix now. > > sequencer.c | 2 +- > t/t3438-rebase-broken-files.sh | 53 ++++++++++++++++++++++++++++++++++ > 2 files changed, 54 insertions(+), 1 deletion(-) > create mode 100755 t/t3438-rebase-broken-files.sh > > diff --git a/sequencer.c b/sequencer.c > index d26ede83c4..83e0425b04 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -915,7 +915,7 @@ int read_author_script(const char *path, char **name, char **email, char **date, > error(_("missing 'GIT_AUTHOR_EMAIL'")); > if (date_i == -2) > error(_("missing 'GIT_AUTHOR_DATE'")); > - if (date_i < 0 || email_i < 0 || date_i < 0 || err) > + if (name_i < 0 || email_i < 0 || date_i < 0 || err) > goto finish; > *name = kv.items[name_i].util; > *email = kv.items[email_i].util; > diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh > new file mode 100755 > index 0000000000..e68aac4b36 > --- /dev/null > +++ b/t/t3438-rebase-broken-files.sh > @@ -0,0 +1,53 @@ > +#!/bin/sh > + > +test_description='rebase behavior when on-disk files are broken' > +. ./test-lib.sh > + > +test_expect_success 'set up conflicting branches' ' > + test_commit base file && > + git checkout -b branch1 && > + test_commit one file && > + git checkout -b branch2 HEAD^ && > + test_commit two file > +' > + > +check_broken_author () { > + title=$1; shift > + script=$1; shift > + > + test_expect_success "$title" ' > + # create conflicted state > + test_when_finished "git rebase --abort" && > + git checkout -B tmp branch2 && > + test_must_fail git rebase branch1 && > + > + # break author-script > + '"$script"' && > + > + # resolving notices broken author-script > + echo resolved >file && > + git add file && > + test_must_fail git rebase --continue > + ' > +} > + > +for item in NAME EMAIL DATE > +do > + check_broken_author "detect missing GIT_AUTHOR_$item" ' > + grep -v $item .git/rebase-merge/author-script >tmp && > + mv tmp .git/rebase-merge/author-script' > +done > + > +for item in NAME EMAIL DATE > +do > + check_broken_author "detect duplicate GIT_AUTHOR_$item" ' > + grep -i $item .git/rebase-merge/author-script >tmp && > + cat tmp >>.git/rebase-merge/author-script' > +done > + > +check_broken_author 'unknown key in author-script' ' > + echo "GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}" \ > + >>.git/rebase-merge/author-script' > + > + > +test_done