threads / patch / 34678

patch, 3 partsfix interactive rebase short SHA-1 collision bug

Subject: [PATCH 0/3] fix interactive rebase short SHA-1 collision bug

## tl;dr

8 messages between Aug 12, 2013 and Aug 12, 2013. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Eric Sunshine· Aug 12, 2013, 04:07 UTC · lore

This series addresses a bug [1][2] which can manifest during interactive rebase when the prefix of the new SHA-1 of an edited commit is shared with the abbreviated SHA-1 of a subsequent commit in the 'todo' list. When rebase attempts to process the subsequent command, it dies with a "short SHA1 badbeef is ambiguous" error.

patch 1: fix a problem in the interactive rebase test suite which can
    make subsequent tests fail
patch 2: add a test demonstrating the short SHA-1 collision bug
patch 3: fix the bug (this patch is from Junio [3] but augmented also to
    fix up "rebase --edit-todo")

[1]: http://thread.gmane.org/gmane.comp.version-control.git/229091 [2]: http://thread.gmane.org/gmane.comp.version-control.git/232012 [3]: http://thread.gmane.org/gmane.comp.version-control.git/229091/focus=229120

Eric Sunshine (2):
  t3404: restore specialized rebase-editor following commentchar test
  t3404: rebase: interactive: demonstrate short SHA-1 collision
Junio C Hamano (1):
  rebase: interactive: fix short SHA-1 collision
 git-rebase--interactive.sh    | 30 ++++++++++++++++++++++++++++++
 t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++
 2 files changed, 48 insertions(+)
-- 
1.8.4.rc2.460.ga591f4a
Eric Sunshine· Aug 12, 2013, 04:07 UTC · re: Eric Sunshine · lore

[PATCH 1/3] t3404: restore specialized rebase-editor following commentchar test

At start of script, t3404 installs a specialized test-editor ($EDITOR) upon which many of the interactive rebase tests depend. Late in t3404, test "rebase -i respects core.commentchar" installs its own custom editor but neglects to restore the specialized editor when finished. This oversight will cause later tests, which require the specialized editor, to fail. (There are no such tests presently, but a subsequent patch will introduce one.) Fix this problem.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t3404-rebase-interactive.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/t3404-rebase-interactive.sh +1 −0
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 49ccb38..af141be 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -949,6 +949,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	sed -e "2,\$s/^/\\\\/" "$1" >"$1.tmp" &&
 	mv "$1.tmp" "$1"
 	EOF
+	test_when_finished "set_fake_editor" &&
 	test_set_editor "$(pwd)/remove-all-but-first.sh" &&
 	git rebase -i B &&
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
-- 
1.8.4.rc2.460.ga591f4a
Junio C Hamano· Aug 12, 2013, 06:28 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/3] t3404: restore specialized rebase-editor following commentchar test

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 6 quoted lines
> At start of script, t3404 installs a specialized test-editor ($EDITOR)
> upon which many of the interactive rebase tests depend.  Late in t3404,
> test "rebase -i respects core.commentchar" installs its own custom
> editor but neglects to restore the specialized editor when finished.
> This oversight will cause later tests, which require the specialized
> editor, to fail. 

That is not oversight but was deliberately done knowing that it will be the last test (and new tests can be added before it).

I think the patch is one way to give _known_ status to later tests by declaring the editor installed by "set_fake_editor" the gold standard, but isn't a better alternative to make sure that any newly added tests after this point (or before the commentchar tests, for that matter) set a fake editor it wants to use explicitly?

Show 20 quoted lines
> (There are no such tests presently, but a subsequent
> patch will introduce one.)  Fix this problem.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/t3404-rebase-interactive.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 49ccb38..af141be 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -949,6 +949,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
>  	sed -e "2,\$s/^/\\\\/" "$1" >"$1.tmp" &&
>  	mv "$1.tmp" "$1"
>  	EOF
> +	test_when_finished "set_fake_editor" &&
>  	test_set_editor "$(pwd)/remove-all-but-first.sh" &&
>  	git rebase -i B &&
>  	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
Eric Sunshine· Aug 12, 2013, 07:29 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] t3404: restore specialized rebase-editor following commentchar test

On Mon, Aug 12, 2013 at 2:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> At start of script, t3404 installs a specialized test-editor ($EDITOR)
>> upon which many of the interactive rebase tests depend.  Late in t3404,
>> test "rebase -i respects core.commentchar" installs its own custom
>> editor but neglects to restore the specialized editor when finished.
>> This oversight will cause later tests, which require the specialized
>> editor, to fail.
>
> That is not oversight but was deliberately done knowing that it will
> be the last test (and new tests can be added before it).

There is no mention of this being deliberate either in the mailing list discussion [1] or the commit message (180bad3d1), and other tests have been added following this one.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/216079
Show 5 quoted lines
> I think the patch is one way to give _known_ status to later tests
> by declaring the editor installed by "set_fake_editor" the gold
> standard, but isn't a better alternative to make sure that any newly
> added tests after this point (or before the commentchar tests, for
> that matter) set a fake editor it wants to use explicitly?

set_fake_editor is the very first thing done by t3404, and many tests depend upon this state. It would have been inconsistent for this one new test to be the exception by having to invoke set_fake_editor itself, however, I don't mind making the new test more self-contained, despite the inconsistency. (A later cleanup patch can do the same for other existing tests.)

Eric Sunshine· Aug 12, 2013, 04:07 UTC · re: Eric Sunshine · lore

[PATCH 2/3] t3404: rebase: interactive: demonstrate short SHA-1 collision

The 'todo' sheet for interactive rebase shows abbreviated SHA-1's and then performs its operations upon those shortened values. This can lead to an abort if the SHA-1 of a reworded or edited commit is no longer unique within the abbreviated SHA-1 space and a subsequent SHA-1 in the todo list has the same abbreviated value.

For example:
  edit f00dfad first
  pick badbeef second

If, after editing, the new SHA-1 of "first" is now badbeef5ba78983324dff5265c80c4490d5a809a, then the subsequent 'pick badbeef second' will fail since badbeef is no longer a unique SHA-1 abbreviation:

  error: short SHA1 badbeef is ambiguous.
  fatal: Needed a single revision
  Invalid commit name: badbeef

Demonstrate this problem with a couple of specially crafted commits which initially have distinct abbreviated SHA-1's, but for which the abbreviated SHA-1's collide after a simple rewording of the first commit's message.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t3404-rebase-interactive.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Show changes to t/t3404-rebase-interactive.sh +17 −0
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index af141be..e5ebec6 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -977,4 +977,21 @@ test_expect_success 'rebase -i with --strategy and -X' '
 	test $(cat file1) = Z
 '
 
+test_expect_success 'short SHA-1 setup' '
+	test_when_finished "git checkout master" &&
+	git checkout --orphan collide &&
+	git rm -rf . &&
+	unset test_tick &&
+	test_commit collide1 collide &&
+	test_commit --notick collide2 collide &&
+	test_commit --notick "collide3 115158b5" collide collide3 collide3
+'
+
+test_expect_failure 'short SHA-1 collide' '
+	test_when_finished "reset_rebase && git checkout master" &&
+	git checkout collide &&
+	FAKE_COMMIT_MESSAGE="collide2 815200e" \
+	FAKE_LINES="reword 1 2" git rebase -i HEAD~2
+'
+
 test_done
-- 
1.8.4.rc2.460.ga591f4a
Junio C Hamano· Aug 12, 2013, 06:31 UTC · re: Eric Sunshine · lore

Re: [PATCH 2/3] t3404: rebase: interactive: demonstrate short SHA-1 collision

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 43 quoted lines
> The 'todo' sheet for interactive rebase shows abbreviated SHA-1's and
> then performs its operations upon those shortened values. This can lead
> to an abort if the SHA-1 of a reworded or edited commit is no longer
> unique within the abbreviated SHA-1 space and a subsequent SHA-1 in the
> todo list has the same abbreviated value.
>
> For example:
>
>   edit f00dfad first
>   pick badbeef second
>
> If, after editing, the new SHA-1 of "first" is now
> badbeef5ba78983324dff5265c80c4490d5a809a, then the subsequent 'pick
> badbeef second' will fail since badbeef is no longer a unique SHA-1
> abbreviation:
>
>   error: short SHA1 badbeef is ambiguous.
>   fatal: Needed a single revision
>   Invalid commit name: badbeef
>
> Demonstrate this problem with a couple of specially crafted commits
> which initially have distinct abbreviated SHA-1's, but for which the
> abbreviated SHA-1's collide after a simple rewording of the first
> commit's message.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/t3404-rebase-interactive.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index af141be..e5ebec6 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -977,4 +977,21 @@ test_expect_success 'rebase -i with --strategy and -X' '
>  	test $(cat file1) = Z
>  '
>  
> +test_expect_success 'short SHA-1 setup' '
> +	test_when_finished "git checkout master" &&
> +	git checkout --orphan collide &&
> +	git rm -rf . &&
> +	unset test_tick &&

This will inconvenience tests added later to these two in the future. Oversight, or deliberate act knowing that these two are the last tests in this script ;-)?

One bad thing is that "unset" loses information so that such future tests cannot resurrect test_tick and continue on from where the last test-tick left off.

Show 9 quoted lines
> +	test_commit collide1 collide &&
> +	test_commit --notick collide2 collide &&
> +	test_commit --notick "collide3 115158b5" collide collide3 collide3
> +'
>
> +test_expect_failure 'short SHA-1 collide' '
> +	test_when_finished "reset_rebase && git checkout master" &&
> +	git checkout collide &&
> +	FAKE_COMMIT_MESSAGE="collide2 815200e" \
;-)
> +	FAKE_LINES="reword 1 2" git rebase -i HEAD~2
> +'
> +
>  test_done
Eric Sunshine· Aug 12, 2013, 07:40 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] t3404: rebase: interactive: demonstrate short SHA-1 collision

On Mon, Aug 12, 2013 at 2:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 53 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> The 'todo' sheet for interactive rebase shows abbreviated SHA-1's and
>> then performs its operations upon those shortened values. This can lead
>> to an abort if the SHA-1 of a reworded or edited commit is no longer
>> unique within the abbreviated SHA-1 space and a subsequent SHA-1 in the
>> todo list has the same abbreviated value.
>>
>> For example:
>>
>>   edit f00dfad first
>>   pick badbeef second
>>
>> If, after editing, the new SHA-1 of "first" is now
>> badbeef5ba78983324dff5265c80c4490d5a809a, then the subsequent 'pick
>> badbeef second' will fail since badbeef is no longer a unique SHA-1
>> abbreviation:
>>
>>   error: short SHA1 badbeef is ambiguous.
>>   fatal: Needed a single revision
>>   Invalid commit name: badbeef
>>
>> Demonstrate this problem with a couple of specially crafted commits
>> which initially have distinct abbreviated SHA-1's, but for which the
>> abbreviated SHA-1's collide after a simple rewording of the first
>> commit's message.
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>>  t/t3404-rebase-interactive.sh | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index af141be..e5ebec6 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -977,4 +977,21 @@ test_expect_success 'rebase -i with --strategy and -X' '
>>       test $(cat file1) = Z
>>  '
>>
>> +test_expect_success 'short SHA-1 setup' '
>> +     test_when_finished "git checkout master" &&
>> +     git checkout --orphan collide &&
>> +     git rm -rf . &&
>> +     unset test_tick &&
>
> This will inconvenience tests added later to these two in the
> future.  Oversight, or deliberate act knowing that these two are the
> last tests in this script ;-)?
>
> One bad thing is that "unset" loses information so that such future
> tests cannot resurrect test_tick and continue on from where the last
> test-tick left off.

Oversight. The commits were quite deliberately crafted with very specific commit times, commit messages, trees, and blobs in order to achieve a conflicting short SHA-1 ("badbeef") at rebase time, so it was necessary to have test_tick at a known state.

It shouldn't be too much effort to save its value and restore it at the end of the test.

Show 16 quoted lines
>> +     test_commit collide1 collide &&
>> +     test_commit --notick collide2 collide &&
>> +     test_commit --notick "collide3 115158b5" collide collide3 collide3
>> +'
>>
>> +test_expect_failure 'short SHA-1 collide' '
>> +     test_when_finished "reset_rebase && git checkout master" &&
>> +     git checkout collide &&
>> +     FAKE_COMMIT_MESSAGE="collide2 815200e" \
>
> ;-)
>
>> +     FAKE_LINES="reword 1 2" git rebase -i HEAD~2
>> +'
>> +
>>  test_done
Eric Sunshine· Aug 12, 2013, 04:07 UTC · re: Eric Sunshine · lore

[PATCH 3/3] rebase: interactive: fix short SHA-1 collision

From: Junio C Hamano <gitster@pobox.com>

The 'todo' sheet for interactive rebase shows abbreviated SHA-1's and then performs its operations upon those shortened values. This can lead to an abort if the SHA-1 of a reworded or edited commit is no longer unique within the abbreviated SHA-1 space and a subsequent SHA-1 in the todo list has the same abbreviated value.

For example:
  edit f00dfad first
  pick badbeef second

If, after editing, the new SHA-1 of "first" is now badbeef5ba78983324dff5265c80c4490d5a809a, then the subsequent 'pick badbeef second' will fail since badbeef is no longer a unique SHA-1 abbreviation:

  error: short SHA1 badbeef is ambiguous.
  fatal: Needed a single revision
  Invalid commit name: badbeef

Fix this problem by expanding the SHA-1's in the todo list before performing the operations.

[es: also collapse & expand SHA-1's for --edit-todo]
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 git-rebase--interactive.sh    | 30 ++++++++++++++++++++++++++++++
 t/t3404-rebase-interactive.sh |  2 +-
 2 files changed, 31 insertions(+), 1 deletion(-)
Show changes to 2 files +31 −1

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 83d6d46..ea11e62 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -689,6 +689,32 @@ skip_unnecessary_picks () {
 	die "Could not skip unnecessary pick commands"
 }
 
+transform_todo_ids () {
+	while read -r command rest
+	do
+		case "$command" in
+		'#'* | exec)
+			# Be careful for oddball commands like 'exec'
+			# that do not have a SHA-1 at the beginning of $rest.
+			;;
+		*)
+			sha1=$(git rev-parse --verify --quiet "$@" ${rest%% *}) &&
+			rest="$sha1 ${rest#* }"
+			;;
+		esac
+		printf '%s\n' "$command${rest:+ }$rest"
+	done <"$todo" >"$todo.new" &&
+	mv -f "$todo.new" "$todo"
+}
+
+expand_todo_ids() {
+	transform_todo_ids
+}
+
+collapse_todo_ids() {
+	transform_todo_ids --short=7
+}
+
 # Rearrange the todo list that has both "pick sha1 msg" and
 # "pick sha1 fixup!/squash! msg" appears in it so that the latter
 # comes immediately after the former, and change "pick" to
@@ -841,6 +867,7 @@ skip)
 edit-todo)
 	git stripspace --strip-comments <"$todo" >"$todo".new
 	mv -f "$todo".new "$todo"
+	collapse_todo_ids
 	append_todo_help
 	git stripspace --comment-lines >>"$todo" <<\EOF
 
@@ -852,6 +879,7 @@ EOF
 
 	git_sequence_editor "$todo" ||
 		die "Could not execute editor"
+	expand_todo_ids
 
 	exit
 	;;
@@ -1008,6 +1036,8 @@ git_sequence_editor "$todo" ||
 has_action "$todo" ||
 	die_abort "Nothing to do"
 
+expand_todo_ids
+
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
 GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e5ebec6..db56db3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -987,7 +987,7 @@ test_expect_success 'short SHA-1 setup' '
 	test_commit --notick "collide3 115158b5" collide collide3 collide3
 '
 
-test_expect_failure 'short SHA-1 collide' '
+test_expect_success 'short SHA-1 collide' '
 	test_when_finished "reset_rebase && git checkout master" &&
 	git checkout collide &&
 	FAKE_COMMIT_MESSAGE="collide2 815200e" \
-- 
1.8.4.rc2.460.ga591f4a

← back to recent threads