Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]
- From
- David Sharp <dhsharp@google.com>
- Date
- Jan 28, 2014, 22:02 UTC
- Message-ID
- <CAJL_ekuuu+AZH7n4fKzUXqou-nhf96QpcQ-utvMNYRjx4BrTUg@mail.gmail.com>
- In-Reply-To
- <xmqqmwiflr37.fsf@gitster.dls.corp.google.com>
On Tue, Jan 28, 2014 at 1:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> David Sharp <dhsharp@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.
Show 50 quoted lines
>
> Thanks.
>
>>
>> Signed-off-by: David Sharp <dhsharp@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;
>> }