git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rev-parse: Check argc before using argv[i+1]
@ 2014-01-27 23:44 David Sharp
  2014-01-28 19:12 ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: David Sharp @ 2014-01-27 23:44 UTC (permalink / raw)
  To: git; +Cc: David Sharp

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(-)

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] rev-parse: Check argc before using argv[i+1]
  2014-01-27 23:44 [PATCH] rev-parse: Check argc before using argv[i+1] David Sharp
@ 2014-01-28 19:12 ` Junio C Hamano
  2014-01-28 21:20   ` David Sharp
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2014-01-28 19:12 UTC (permalink / raw)
  To: David Sharp; +Cc: git

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.  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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] rev-parse: Check argc before using argv[i+1]
  2014-01-28 19:12 ` Junio C Hamano
@ 2014-01-28 21:20   ` David Sharp
  2014-01-28 21:21     ` [PATCH v2] " David Sharp
  0 siblings, 1 reply; 8+ messages in thread
From: David Sharp @ 2014-01-28 21:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Tue, Jan 28, 2014 at 11:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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...

>
>         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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] rev-parse: Check argc before using argv[i+1]
  2014-01-28 21:20   ` David Sharp
@ 2014-01-28 21:21     ` David Sharp
  2014-01-28 21:43       ` Junio C Hamano
  2014-01-28 22:01       ` Johannes Sixt
  0 siblings, 2 replies; 8+ messages in thread
From: David Sharp @ 2014-01-28 21:21 UTC (permalink / raw)
  To: git; +Cc: David Sharp

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(-)

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]
  2014-01-28 21:21     ` [PATCH v2] " David Sharp
@ 2014-01-28 21:43       ` Junio C Hamano
  2014-01-28 22:02         ` David Sharp
  2014-01-28 22:01       ` Johannes Sixt
  1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2014-01-28 21:43 UTC (permalink / raw)
  To: David Sharp; +Cc: git

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.

>
> 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;
>  			}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]
  2014-01-28 21:21     ` [PATCH v2] " David Sharp
  2014-01-28 21:43       ` Junio C Hamano
@ 2014-01-28 22:01       ` Johannes Sixt
  2014-01-28 22:03         ` David Sharp
  1 sibling, 1 reply; 8+ messages in thread
From: Johannes Sixt @ 2014-01-28 22:01 UTC (permalink / raw)
  To: David Sharp, git

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?

-- Hannes

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]
  2014-01-28 21:43       ` Junio C Hamano
@ 2014-01-28 22:02         ` David Sharp
  0 siblings, 0 replies; 8+ messages in thread
From: David Sharp @ 2014-01-28 22:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Tue, Jan 28, 2014 at 1:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>
> 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;
>>                       }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]
  2014-01-28 22:01       ` Johannes Sixt
@ 2014-01-28 22:03         ` David Sharp
  0 siblings, 0 replies; 8+ messages in thread
From: David Sharp @ 2014-01-28 22:03 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git

On Tue, Jan 28, 2014 at 2:01 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> 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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-01-28 22:03 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-27 23:44 [PATCH] rev-parse: Check argc before using argv[i+1] David Sharp
2014-01-28 19:12 ` Junio C Hamano
2014-01-28 21:20   ` David Sharp
2014-01-28 21:21     ` [PATCH v2] " David Sharp
2014-01-28 21:43       ` Junio C Hamano
2014-01-28 22:02         ` David Sharp
2014-01-28 22:01       ` Johannes Sixt
2014-01-28 22:03         ` David Sharp

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).