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