diff mbox series

[2/3] rebase -i: re-fix short SHA-1 collision

Message ID ad50cd1b92e3e52309536f3a84064571a224a0da.1579209506.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Re-fix rebase -i with SHA-1 collisions | expand

Commit Message

Johannes Schindelin via GitGitGadget Jan. 16, 2020, 9:18 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 66ae9a57b88 (t3404: rebase -i: demonstrate short SHA-1 collision,
2013-08-23), we added a test case that demonstrated how it is possible
that a previously unambiguous short commit ID could become ambiguous
*during* a rebase.

In 75c69766554 (rebase -i: fix short SHA-1 collision, 2013-08-23), we
fixed that problem simply by writing out the todo list with expanded
commit IDs (except *right* before letting the user edit the todo list,
in which case we shorten them, but we expand them right after the file
was edited).

However, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:
directly call pick_commits() from complete_action(), 2019-11-24): as of
this commit, the sequencer no longer re-reads the todo list after
writing it out with expanded commit IDs.

The only redeeming factor is that the todo list is already parsed at
that stage, including all the commits corresponding to the commands,
therefore the sequencer can continue even if the internal todo list has
short commit IDs.

That does not prevent problems, though: the sequencer writes out the
`done` and `git-rebase-todo` files incrementally (i.e. overwriting the
todo list with a version that has _short_ commit IDs), and if a merge
conflict happens, or if an `edit` or a `break` command is encountered, a
subsequent `git rebase --continue` _will_ re-read the todo list, opening
an opportunity for the "short SHA-1 collision" bug again.

To avoid that, let's make sure that we do expand the commit IDs in the
todo list as soon as we have parsed it after letting the user edit it.

Additionally, we improve the 'short SHA-1 collide' test case in t3404 to
test specifically for the case where the rebase is resumed. We also
hard-code the expected colliding short SHA-1s, to document the
expectation (and to make it easier on future readers).

Note that we specifically test that the short commit ID is used in the
`git-rebase-todo.tmp` file: this file is created by the fake editor in
the test script and reflects the state that would have been presented to
the user to edit.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c                   | 14 +++++++++++++-
 t/t3404-rebase-interactive.sh | 15 +++++++++++++--
 2 files changed, 26 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Jan. 17, 2020, 6:30 p.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> However, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:
> directly call pick_commits() from complete_action(), 2019-11-24): as of
> this commit, the sequencer no longer re-reads the todo list after
> writing it out with expanded commit IDs.

Ouch.  The analysis above is quite understandable.

> @@ -5128,6 +5128,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  		return -1;
>  	}
>  
> +	/* Expand the commit IDs */
> +	todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
> +	strbuf_swap(&new_todo.buf, &buf2);
> +	strbuf_release(&buf2);
> +	new_todo.total_nr -= new_todo.nr;
> +	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) {
> +		fprintf(stderr, _(edit_todo_list_advice));
> +		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
> +		todo_list_release(&new_todo);
> +		return error(_("invalid todo list after expanding IDs"));
> +	}

The above happens after edit_todo_list() returns and then the
resulting data (i.e. new_todo) that came from the on-disk file has
been parsed with an existing call to todo_lsit_parse_insn_buffer()?

I am wondering when this if() statement would trigger, iow, under
what condition the result of "Expand the commit IDs" operation fails
to be parsed correctly, and what the user can do to remedy it.
Especially given that incoming new_todo has passed the existing
parse and check without hitting "return -1" we see above in the
context, I am not sure if there is anything, other than any
potential glitch in the added code above, that can cause this second
parse_insn_buffer() to fail.  Shouldn't the body of if() be simply a
BUG()?

Or am I somehow missing a hunk that removes an existing call to
parse&check?

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ae6e55ce79..1cc9f36bc7 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '
>  test_expect_success SHA1 'short SHA-1 collide' '
>  	test_when_finished "reset_rebase && git checkout master" &&
>  	git checkout collide &&
> +	colliding_sha1=6bcda37 &&
> +	test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
>  	(
>  		unset test_tick &&
>  		test_tick &&
>  		set_fake_editor &&
>  		FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \
> -		FAKE_LINES="reword 1 2" git rebase -i HEAD~2
> -	)
> +		FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 &&
> +		test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
> +		grep "^pick $colliding_sha1 " \
> +			.git/rebase-merge/git-rebase-todo.tmp &&
> +		grep "^pick [0-9a-f]\{40\}" \
> +			.git/rebase-merge/git-rebase-todo &&
> +		git rebase --continue
> +	) &&
> +	collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" &&
> +	collide3="$(git rev-parse collide3 | cut -c 1-4)" &&
> +	test "$collide2" = "$collide3"
>  '
>  
>  test_expect_success 'respect core.abbrev' '
Johannes Schindelin Jan. 17, 2020, 9:31 p.m. UTC | #2 
Hi Junio,

On Fri, 17 Jan 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +	/* Expand the commit IDs */
> > +	todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
> > +	strbuf_swap(&new_todo.buf, &buf2);
> > +	strbuf_release(&buf2);
> > +	new_todo.total_nr -= new_todo.nr;
> > +	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) {
> > +		fprintf(stderr, _(edit_todo_list_advice));
> > +		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
> > +		todo_list_release(&new_todo);
> > +		return error(_("invalid todo list after expanding IDs"));
> > +	}
>
> The above happens after edit_todo_list() returns and then the
> resulting data (i.e. new_todo) that came from the on-disk file has
> been parsed with an existing call to todo_lsit_parse_insn_buffer()?
>
> I am wondering when this if() statement would trigger, iow, under
> what condition the result of "Expand the commit IDs" operation fails
> to be parsed correctly, and what the user can do to remedy it.

You're right, this is defensive code to guard against bugs, but the error
message suggests that it might be a user error: it isn't.

> Especially given that incoming new_todo has passed the existing
> parse and check without hitting "return -1" we see above in the
> context, I am not sure if there is anything, other than any
> potential glitch in the added code above, that can cause this second
> parse_insn_buffer() to fail.  Shouldn't the body of if() be simply a
> BUG()?

It should.

Will fix,
Dscho

> Or am I somehow missing a hunk that removes an existing call to
> parse&check?
>
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index ae6e55ce79..1cc9f36bc7 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '
> >  test_expect_success SHA1 'short SHA-1 collide' '
> >  	test_when_finished "reset_rebase && git checkout master" &&
> >  	git checkout collide &&
> > +	colliding_sha1=6bcda37 &&
> > +	test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
> >  	(
> >  		unset test_tick &&
> >  		test_tick &&
> >  		set_fake_editor &&
> >  		FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \
> > -		FAKE_LINES="reword 1 2" git rebase -i HEAD~2
> > -	)
> > +		FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 &&
> > +		test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
> > +		grep "^pick $colliding_sha1 " \
> > +			.git/rebase-merge/git-rebase-todo.tmp &&
> > +		grep "^pick [0-9a-f]\{40\}" \
> > +			.git/rebase-merge/git-rebase-todo &&
> > +		git rebase --continue
> > +	) &&
> > +	collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" &&
> > +	collide3="$(git rev-parse collide3 | cut -c 1-4)" &&
> > +	test "$collide2" = "$collide3"
> >  '
> >
> >  test_expect_success 'respect core.abbrev' '
>
>
diff mbox series

Patch

diff --git a/sequencer.c b/sequencer.c
index 7c30dad59c..c2945c699d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5076,7 +5076,7 @@  int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 {
 	const char *shortonto, *todo_file = rebase_path_todo();
 	struct todo_list new_todo = TODO_LIST_INIT;
-	struct strbuf *buf = &todo_list->buf;
+	struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
 	struct object_id oid = onto->object.oid;
 	int res;
 
@@ -5128,6 +5128,18 @@  int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return -1;
 	}
 
+	/* Expand the commit IDs */
+	todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
+	strbuf_swap(&new_todo.buf, &buf2);
+	strbuf_release(&buf2);
+	new_todo.total_nr -= new_todo.nr;
+	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) {
+		fprintf(stderr, _(edit_todo_list_advice));
+		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
+		todo_list_release(&new_todo);
+		return error(_("invalid todo list after expanding IDs"));
+	}
+
 	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {
 		todo_list_release(&new_todo);
 		return error(_("could not skip unnecessary pick commands"));
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ae6e55ce79..1cc9f36bc7 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1264,13 +1264,24 @@  test_expect_success SHA1 'short SHA-1 setup' '
 test_expect_success SHA1 'short SHA-1 collide' '
 	test_when_finished "reset_rebase && git checkout master" &&
 	git checkout collide &&
+	colliding_sha1=6bcda37 &&
+	test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
 	(
 		unset test_tick &&
 		test_tick &&
 		set_fake_editor &&
 		FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \
-		FAKE_LINES="reword 1 2" git rebase -i HEAD~2
-	)
+		FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 &&
+		test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
+		grep "^pick $colliding_sha1 " \
+			.git/rebase-merge/git-rebase-todo.tmp &&
+		grep "^pick [0-9a-f]\{40\}" \
+			.git/rebase-merge/git-rebase-todo &&
+		git rebase --continue
+	) &&
+	collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" &&
+	collide3="$(git rev-parse collide3 | cut -c 1-4)" &&
+	test "$collide2" = "$collide3"
 '
 
 test_expect_success 'respect core.abbrev' '