On Tue, Jan 28, 2014 at 1:43 PM, Junio C Hamano <gits...@pobox.com> wrote:
> David Sharp <dhsh...@google.com> writes:
>
>> Without this patch, git-rev-parse --prefix, --default, or
>> --resolve-git-dir, without a value argument, would result in a segfault.
>> Instead, die() with a message.
>
> When I sent the review message, I actually was on the fence between
> checking i vs argc and checking the nullness myself.  I realize,
> after seeing the actual patch below, that we are protecting against
> picking up a NULL and blindly passing it on in the codepaths that
> follow, so the updated code does look a lot better, at least to me.

It's a matter of perspective, I guess. When checking the index, to me,
I saw that it is protecting against reading past the end of an array,
which is at least as bad as sending NULL to codepaths that don't
accept NULL. I do think that everybody knows that reading past the end
of an array is bad, while not necessarily everybody knows that the
argv array is null-terminated.

>
> Thanks.
>
>>
>> Signed-off-by: David Sharp <dhsh...@google.com>
>> ---
>>  builtin/rev-parse.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
>> index aaeb611..45901df 100644
>> --- a/builtin/rev-parse.c
>> +++ b/builtin/rev-parse.c
>> @@ -547,15 +547,17 @@ int cmd_rev_parse(int argc, const char **argv, const 
>> char *prefix)
>>                               continue;
>>                       }
>>                       if (!strcmp(arg, "--default")) {
>> -                             def = argv[i+1];
>> -                             i++;
>> +                             def = argv[++i];
>> +                             if (!def)
>> +                                     die("--default requires an argument");
>>                               continue;
>>                       }
>>                       if (!strcmp(arg, "--prefix")) {
>> -                             prefix = argv[i+1];
>> +                             prefix = argv[++i];
>> +                             if (!prefix)
>> +                                     die("--prefix requires an argument");
>>                               startup_info->prefix = prefix;
>>                               output_prefix = 1;
>> -                             i++;
>>                               continue;
>>                       }
>>                       if (!strcmp(arg, "--revs-only")) {
>> @@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const 
>> char *prefix)
>>                               continue;
>>                       }
>>                       if (!strcmp(arg, "--resolve-git-dir")) {
>> -                             const char *gitdir = resolve_gitdir(argv[i+1]);
>> +                             const char *gitdir = argv[++i];
>>                               if (!gitdir)
>> -                                     die("not a gitdir '%s'", argv[i+1]);
>> +                                     die("--resolve-git-dir requires an 
>> argument");
>> +                             gitdir = resolve_gitdir(gitdir);
>> +                             if (!gitdir)
>> +                                     die("not a gitdir '%s'", argv[i]);
>>                               puts(gitdir);
>>                               continue;
>>                       }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to