* [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).