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

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

On Thu, May 21, 2009 at 11:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> 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.

OK. I will send another patch for verify_bundle() then :-) just to
make sure no one goes down the same way.
-- 
Duy