Re: [PATCH 3/6] revert: Parse instruction sheet more cautiously

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> Fix a buffer overflow bug by checking that parsed SHA-1 hex will fit
> in the buffer we've created for it.

Nit: it seems best to either describe the behavior or the code change.
So, for example:

	Do not overflow a buffer when the second word in a "pick"
	or "revert" line in .git/sequencer/todo is very long.

Or:

	Check that the commit name argument to a "pick" or "revert"
	action is not too long, to avoid overflowing an on-stack
	buffer.

It would also be comforting to readers to mention when the bug was
introduced, so they don't start worrying about protecting their
installations against privilege escalation:

	This fixes a regression introduced by <...>, which has not
	escaped into the wild yet, luckily.

Could we have a test for this?

> Also change the instruction sheet format

How is this "Also" part related to the earlier bit?  Does one make the
other easier, or is it more of a "While we're changing this code" kind
of thing?

> subtly so that a description of the commit after the object
> name is optional.  So now, an instruction sheet like this is perfectly
> valid:
>
>   pick 35b0426
>   pick fbd5bbcbc2e
>   pick 7362160f

Sounds convenient. :)  Thanks.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -697,26 +697,24 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
>  	unsigned char commit_sha1[20];
>  	char sha1_abbrev[40];
>  	enum replay_action action;
> -	int insn_len = 0;
> -	char *p, *q;
> +	char *p = start, *q, *end = strchrnul(start, '\n');

By the way, why are these non-const?  (Not about this patch.)

I don't know why, but I would be more comfortable reading something
like this:

	const char *p, *q;

	p = start;
	if (!prefixcmp(p, "pick ")) {
		action = CHERRY_PICK;
		p += strlen("pick ");
	} else if (...) {
		...
	} ...

	q = p + strcspn(p, " \n");
	if (q - p + 1 > sizeof(...))
		...
	...

Maybe because I can imagine how the pointers move, always forward and
never past the end of the line.

> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -211,4 +211,33 @@ test_expect_success 'malformed instruction sheet 2' '
>  	test_must_fail git cherry-pick --continue
>  '
>  
> +test_expect_success 'missing commit descriptions in instruction sheet' '

What assertion does this test check?  "commit descriptions in insn
sheet are optional"?

> +	pristine_detach initial &&
> +	test_must_fail git cherry-pick base..anotherpick &&

A failed cherry-pick.

> +	echo "c" >foo &&
> +	git add foo &&
> +	git commit &&

Resolving the conflict.

> +	cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
> +	cp new_sheet .git/sequencer/todo &&

Mucking about with the insn sheet.

> +	git cherry-pick --continue &&

Continuing.

> +	test_path_is_missing .git/sequencer &&
> +	{
> +		git rev-list HEAD |
> +		git diff-tree --root --stdin |
> +		sed "s/$_x40/OBJID/g"
> +	} >actual &&
> +	cat >expect <<-\EOF &&
> +	OBJID
> +	:100644 100644 OBJID OBJID M	foo
> +	OBJID
> +	:100644 100644 OBJID OBJID M	foo
> +	OBJID
> +	:100644 100644 OBJID OBJID M	unrelated
> +	OBJID
> +	:000000 100644 OBJID OBJID A	foo
> +	:000000 100644 OBJID OBJID A	unrelated
> +	EOF
> +	test_cmp expect actual

Checking that the cherry-pick succeeded and the resulting list of
commits.  How is this expected to potentially fail?  Maybe something
like

	git rev-list HEAD >commits &&
	test_line_count = 4 commits

or

	git diff --exit-code <something>

would make what this is intended to check clearer.  As hinted above,
some blank lines or comments might make the earlier part easier to
read.

Thanks, this patch does two good things.  For what it's worth, with
the changes hinted at above and a split into two patches if that seems
sensible,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>