Re: [PATCH v2 0/4] fix "rebase -i --root" corrupting root commit

[Phillip Wood <phillip.wood@talktalk.net>]

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 <sunshine@sunshineco.com>
>      +    Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>       
>       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 <sunshine@sunshineco.com>
>       
>       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
>