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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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@xxxxxxxxxxxxxx/
[2]: https://public-inbox.org/git/1f172fc1-4b51-cdf7-e841-5ca41e209be4@xxxxxxxxxxxx/

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@xxxxxxxxxxxxxx>
    +    Acked-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
     
     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@xxxxxxxxxxxxxx>
     
     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
-- 
2.18.0.267.gbc8be36ecb




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux