Re: [PATCH 3/6] revert: Parse instruction sheet more cautiously
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Aug 11, 2011, 19:47 UTC
- Message-ID
- <20110811194708.GG2277@elie.gateway.2wire.net>
- In-Reply-To
- <1313088705-32222-4-git-send-email-artagnon@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?
Show 7 quoted lines
> 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.
Show 9 quoted lines
> --- 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.
Show 7 quoted lines
> --- 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.
Show 18 quoted lines
> + 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 actualChecking 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>