From: Phillip Wood Date: Tue, 31 Jul 2018 10:05:51 GMT Subject: Re: [PATCH v2 0/4] fix "rebase -i --root" corrupting root commit Message-ID: In-Reply-To: <20180731073331.40007-1-sunshine@sunshineco.com> Hi Eric On 31/07/18 08:33, Eric Sunshine wrote: > This is a re-roll of [1] which fixes sequencer bugs resulting in commit > object corruption when "rebase -i --root" swaps in a new commit as root. > Unfortunately, those bugs made it into v2.18.0 and have already > corrupted at least one repository (a local project of mine). Patches 3/4 > and 4/4 are new. > > v1 fixed these bugs: > > * trailing garbage on the commit's "author" header > > * extra trailing digit on "author" header's timezone (caused by two > separate bugs) > > v2 fixes those same bugs, plus: > > * eliminates a bogus "@" prepended to the "author" header timestamp > which renders the header corrupt > > * takes care to validate information coming from > "rebase-merge/author-script" before incorporating it into the "author" > header since that file may be hand-edited, and bogus hand-edited > values could corrupt the commit object. > > Patch 2/4 of this series conflicts with Akinori MUSHA's > 'am/sequencer-author-script-fix' which takes a stab at fixing one of the > four (or so) bugs fixed by this series (namely, adding a missing closing > quote to GIT_AUTHOR_DATE in "rebase-merge/author-script"). That patch > probably ought to be dropped (without prejudice) in favor of this series > for the following reasons: > > * It has the undesirable property of adding an unwanted extra blank line > to "rebase-merge/author-script"; this series doesn't make that > mistake. > > * The fix in this series (patch 2/4) is more complete, also ensuring > that the return value of sq_dequote() is checked. > > * And, most importantly, this series sells the change as a fix for a > genuine serious bug (commit object corruption), whereas that patch > talks only about adjusting the file content to make it parseable by > the shell. It's important for future readers of this change to > understand that the bug (missing closing quote) had much more dire > consequences than merely being syntactically invalid as a shell > script. > > The test added by Akinori MUSHA's patch may still have value, and it may > make sense to re-submit it, however, doing so need not hold up this > (higher priority) series. Yes I think it does, also the patch that Johannes and I have on top of it to fix the quoting of "'" in write_author_script() relies on fixing the missing trailing quote and handling of "'" at the same time, hopefully we can get that rebased on top of these asap. Best Wishes Phillip > Phillip's proposed[2] unification of parsers and other fixes and > cleanups can easily be built atop this series and, likewise, need not > prevent these (higher priority) patches from graduating independently. > > [1]: https://public-inbox.org/git/20180730092929.71114-1-sunshine@sunshineco.com/ > [2]: https://public-inbox.org/git/1f172fc1-4b51-cdf7-e841-5ca41e209be4@talktalk.net/ > > Eric Sunshine (4): > sequencer: fix "rebase -i --root" corrupting author header > sequencer: fix "rebase -i --root" corrupting author header timezone > sequencer: fix "rebase -i --root" corrupting author header timestamp > sequencer: don't die() on bogus user-edited timestamp > > sequencer.c | 39 +++++++++++++++++++++-------------- > t/t3404-rebase-interactive.sh | 10 ++++++++- > 2 files changed, 33 insertions(+), 16 deletions(-) > > Range-diff against v1: > 1: ba10ae4e5d ! 1: 8c47555bcf sequencer: fix "rebase -i --root" corrupting author header > @@ -11,8 +11,8 @@ > This is a result of read_author_ident() neglecting to NUL-terminate the > buffer into which it composes the "author" header. > > - (Note that the extra "0" in the timezone is a separate bug which will be > - fixed subsequently.) > + (Note that the "@" preceding the timestamp and the extra "0" in the > + timezone are separate bugs which will be fixed subsequently.) > > Security considerations: Construction of the "author" header by > read_author_ident() happens in-place and in parallel with parsing the > @@ -26,6 +26,7 @@ > inserted as part of the parsing process. > > Signed-off-by: Eric Sunshine > + Acked-by: Johannes Schindelin > > diff --git a/sequencer.c b/sequencer.c > --- a/sequencer.c > @@ -61,7 +62,7 @@ > + set_fake_editor && > + FAKE_LINES="2 1" git rebase -i --root && > + git cat-file commit HEAD^ >out && > -+ grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$" out > ++ grep "^author ..*> @[0-9][0-9]* [-+][0-9][0-9]*$" out > +' > + > test_done > 2: c0400cda85 ! 2: 1d4064147e sequencer: fix "rebase -i --root" corrupting author header timezone > @@ -22,6 +22,9 @@ > a NUL-terminator at the end of the shifted content, which explains the > duplicated last digit in the timezone. > > + (Note that the "@" preceding the timestamp is a separate bug which > + will be fixed subsequently.) > + > Signed-off-by: Eric Sunshine > > diff --git a/sequencer.c b/sequencer.c > @@ -56,8 +59,8 @@ > set_fake_editor && > FAKE_LINES="2 1" git rebase -i --root && > git cat-file commit HEAD^ >out && > -- grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$" out > -+ grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out > +- grep "^author ..*> @[0-9][0-9]* [-+][0-9][0-9]*$" out > ++ grep "^author ..*> @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out > ' > > test_done > -: ---------- > 3: cb65c7dd98 sequencer: fix "rebase -i --root" corrupting author header timestamp > -: ---------- > 4: 3a624da9f4 sequencer: don't die() on bogus user-edited timestamp >