Re: [PATCH] rev-parse: Check argc before using argv[i+1]
On Tue, Jan 28, 2014 at 11:12 AM, Junio C Hamano wrote: > David Sharp 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse: Check argc before using argv[i+1]
David Sharp 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] 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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html