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@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html