threads / patch / 35744

patchrev-parse: Check argc before using argv[i+1]

Subject: [PATCH] rev-parse: Check argc before using argv[i+1]

## tl;dr

8 messages between Jan 27, 2014 and Jan 28, 2014. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

David Sharp· Jan 27, 2014, 23:44 UTC · lore

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.

Signed-off-by: David Sharp <dhsharp@google.com>
---
 builtin/rev-parse.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
Show changes to builtin/rev-parse.c +10 −6
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index aaeb611..3bf65c5 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++;
+				if (++i >= argc)
+					die("--default requires an argument");
+				def = argv[i];
 				continue;
 			}
 			if (!strcmp(arg, "--prefix")) {
-				prefix = argv[i+1];
+				if (++i >= argc)
+					die("--prefix requires an argument");
+				prefix = argv[i];
 				startup_info->prefix = prefix;
 				output_prefix = 1;
-				i++;
 				continue;
 			}
 			if (!strcmp(arg, "--revs-only")) {
@@ -738,9 +740,11 @@ 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]);
+				if (++i >= argc)
+					die("--resolve-git-dir requires an argument");
+				const char *gitdir = resolve_gitdir(argv[i]);
 				if (!gitdir)
-					die("not a gitdir '%s'", argv[i+1]);
+					die("not a gitdir '%s'", argv[i]);
 				puts(gitdir);
 				continue;
 			}

-- 
1.8.5.3
Junio C Hamano· Jan 28, 2014, 19:12 UTC · re: David Sharp · lore

Re: [PATCH] rev-parse: Check argc before using argv[i+1]

David Sharp <dhsharp@google.com> writes:
Show 11 quoted lines
> @@ -738,9 +740,11 @@ 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]);
> +				if (++i >= argc)
> +					die("--resolve-git-dir requires an argument");
> +				const char *gitdir = resolve_gitdir(argv[i]);
>  				if (!gitdir)
> -					die("not a gitdir '%s'", argv[i+1]);
> +					die("not a gitdir '%s'", argv[i]);

This adds decl-after-statement. As referencing argv[argc] is not a crime (you will grab a NULL), how about doing it this way to see if the value is a NULL, instead of comparing the index and argc?

	const char *gitdir;
        if (!argv[++i])
        	die("--resolve-git-dir requires an argument");
	gitdir = resolve_gitdir(argv[i]);
        if (!gitdir)
        	die("not a gitdir '%s'", argv[i]);
Same comment may apply to other hunks.
Thanks.
David Sharp· Jan 28, 2014, 21:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-parse: Check argc before using argv[i+1]

On Tue, Jan 28, 2014 at 11:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> David Sharp <dhsharp@google.com> writes:
>
>> @@ -738,9 +740,11 @@ 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]);
>> +                             if (++i >= argc)
>> +                                     die("--resolve-git-dir requires an argument");
>> +                             const char *gitdir = resolve_gitdir(argv[i]);
>>                               if (!gitdir)
>> -                                     die("not a gitdir '%s'", argv[i+1]);
>> +                                     die("not a gitdir '%s'", argv[i]);
>
> This adds decl-after-statement.

Sorry, I wasn't aware that git is trying to conform to C90. (It's not compiled with -std=c90, -ansi or -Wdeclaration-after-statement.)

> As referencing argv[argc] is not a
> crime (you will grab a NULL), how about doing it this way to see if
> the value is a NULL, instead of comparing the index and argc?

I'm not convinced this is any better, but alright. I did something slightly different based on that, though.

v2 patch incoming...
Show 11 quoted lines
>
>         const char *gitdir;
>         if (!argv[++i])
>                 die("--resolve-git-dir requires an argument");
>         gitdir = resolve_gitdir(argv[i]);
>         if (!gitdir)
>                 die("not a gitdir '%s'", argv[i]);
>
> Same comment may apply to other hunks.
>
> Thanks.
David Sharp· Jan 28, 2014, 21:21 UTC · re: David Sharp · lore

[PATCH v2] rev-parse: Check argc before using argv[i+1]

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.

Signed-off-by: David Sharp <dhsharp@google.com>
---
 builtin/rev-parse.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
Show changes to builtin/rev-parse.c +11 −6
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;
 			}

-- 
1.8.5.3
Junio C Hamano· Jan 28, 2014, 21:43 UTC · re: David Sharp · lore

Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]

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.

Thanks.
Show 47 quoted lines
>
> 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;
>  			}
David Sharp· Jan 28, 2014, 22:02 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]

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;
>>                       }
Johannes Sixt· Jan 28, 2014, 22:01 UTC · re: David Sharp · lore

Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]

Am 28.01.2014 22:21, schrieb David Sharp:
Show 15 quoted lines
> @@ -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;
>  			}

In this hunk, I don't see where the old code incremented i for the argument. Was this another bug lurking that is fixed now?

-- Hannes
David Sharp· Jan 28, 2014, 22:03 UTC · re: Johannes Sixt · lore

Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]

On Tue, Jan 28, 2014 at 2:01 PM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 19 quoted lines
> Am 28.01.2014 22:21, schrieb David Sharp:
>> @@ -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;
>>                       }
>
> In this hunk, I don't see where the old code incremented i for the
> argument. Was this another bug lurking that is fixed now?
Yup. I did notice that too. Should have mentioned it in the description.
>
> -- Hannes
>

← back to recent threads