Re: [PATCH] setup_revisions(): do not access outside argv

From: Junio C Hamano <gitster@pobox.com>
Date: May 21, 2009, 01:58 UTC
Message-ID: <7v7i0btdwu.fsf@alter.siamese.dyndns.org>
In-Reply-To: <fcaeb9bf0905200123r3649a7e5vc40ece402379e701@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Show 30 quoted lines

> 2009/5/20 Johannes Sixt <j.sixt@viscovery.net>:
>> Nguyễn Thái Ngọc Duy schrieb:
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>>  revision.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/revision.c b/revision.c
>>> index 18b7ebb..be1e307 100644
>>> --- a/revision.c
>>> +++ b/revision.c
>>> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
>>>               if (strcmp(arg, "--"))
>>>                       continue;
>>>               argv[i] = NULL;
>>> -             argc = i;
>>> -             if (argv[i + 1])
>>> +             if (i + 1 < argc && argv[i + 1])
>>>                       revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);
>>> +             argc = i;
>>>               seen_dashdash = 1;
>>>               break;
>>>       }
>>
>> Why is this necessary? I'd expect that argv arrays have NULL at the end.
>
> I have no idea. I hit this "bug" in my builtin-rebase.c and had that
> question too. But I grepped through and saw that
> at least verify_bundle() does not terminate argv with NULL. So I
> assume that setup_revisions() does not expect NULL at the end.

If a function takes (int ac, char **av), then people should be able to depend on the usual convention of

 (1) for any i < ac, av[i] is not NULL; and
 (2) av[ac] is NULL.

With your patch, a broken caller's wish is simply discarded and nobody will notice. Without your patch, at least you will know that the caller passed an inconsistent pair of ac and av to this function by seeing a coalmine canary segfault.

I would not mind a patch that adds an assertion that protects this function from broken callers, so that we can find them, but your patch makes me feel very uneasy.