On Fri, Sep 14, 2007 at 07:31:00AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@xxxxxxxxxx> writes:
>
> > Also, as we "spare" one argument from the batch before the last
> > (supposedly a full batch) we must be sure this batch with one element less
> > has at least 2 elements. Meaning that batches must have at least 3 available
> > slots for the algorithm to work. As it's a very unreasonable case (user
> > passing more than MAXARGS - 3 option flags !!!) die loudly.
>
> I did not quite like iterating over the index twice with
> pathspec_matches() which is not exactly "trivially cheap".
Agreed.
> So here is a counter-proposal. It looks far larger but that is
> due mostly for extra commenting on what is going on. I think
> this code is better structured and easier to read.
Except that you should *here* test if count > 3, because if you take 2
out of 3, you will then call grep with exactly 1 argument. This can only
happen if the user had sth like MAXARGS - 3 (or - 4, it does not really
matter for the explanation anyway) options, then he will have wrong
results. Though, as MAXARGS is _very_ long, that corresponds to insane
situations that we just want to die() for ;)
> + else if (kept) {
> + /*
> + * Called because we found many paths and haven't finished
> + * iterating over the cache yet. We keep two paths
> + * for the concluding call. argv[argc-2] and argv[argc-1]
> + * has the last two paths, so save the first one away,
> + * replace it with NULL while sending the list to grep,
> + * and recover them after we are done.
> + */
+ if (count < 4) {
+ /* we must have more than 3 paths to split, else
+ we end up with a singleton here which is what
+ we try to avoid */
+ die("insanely many options to grep");
+ }
...
Alternatively you could:
> static int external_grep(struct grep_opt *opt, const char **paths, int cached)
> {
> int i, nr, argc, hit, len, status;
> @@ -253,22 +324,12 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)
> push_arg(p->pattern);
> }
>
> - /*
> - * To make sure we get the header printed out when we want it,
> - * add /dev/null to the paths to grep. This is unnecessary
> - * (and wrong) with "-l" or "-L", which always print out the
> - * name anyway.
> - *
> - * GNU grep has "-H", but this is portable.
> - */
> - if (!opt->name_only && !opt->unmatch_name_only)
> - push_arg("/dev/null");
> -
> hit = 0;
> argc = nr;
+ if (MAXARGS - nr < 4) {
+ /* we must have more than 3 paths to split, else
+ we end up with a singleton here which is what
+ we try to avoid */
+ die("insanely many options to grep");
+ }
> for (i = 0; i < active_nr; i++) {
> struct cache_entry *ce = active_cache[i];
> char *name;
The test I propose are more aggressive than the one you proposes (in
the sense that If I'm correct you allow to call grep with 2 paths and
996 options, whereas my latter test disallow that), but I think that my
tests are clearer to read (I mean I had a hard time understanding how
your test worked, and what it really prevented) and the situations it
prevents are unlikely (and it's an euphemism) to happen.
So yes, this patch is 200% better than mine, and this is definitely
how I should have done it, it's so obvious that I feel stupid not to
have thought about that, but still, the complexity of your overflow test
is too hidden, too subtle, and in fact can yields incorrects results
(with 997 options and more than 3 paths).
--
·O· Pierre Habouzit
··O madcoder@xxxxxxxxxx
OOO http://www.madism.org
Attachment:
pgpZGUsLzcL7W.pgp
Description: PGP signature
