On Tue, Jan 28, 2014 at 1:43 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> David Sharp <dhsharp@xxxxxxxxxx> 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 <dhsharp@xxxxxxxxxx>
>> ---
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html