# [PATCH] rev-parse: Check argc before using argv[i+1] 8 messages from 2014-01-27 to 2014-01-28. Participants: David Sharp, Junio C Hamano, Johannes Sixt. Thread: https://gitlist.dev/t/35744 ## David Sharp, 2014-01-27 23:44 Subject: [PATCH] rev-parse: Check argc before using argv[i+1] Message-ID: <1390866262-10188-1-git-send-email-dhsharp@google.com> URL: https://gitlist.dev/e/1390866262-10188-1-git-send-email-dhsharp%40google.com ``` 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 ``` ## Junio C Hamano, 2014-01-28 19:12 Subject: Re: [PATCH] rev-parse: Check argc before using argv[i+1] Message-ID: URL: https://gitlist.dev/e/xmqqtxcnly39.fsf%40gitster.dls.corp.google.com In-Reply-To: <1390866262-10188-1-git-send-email-dhsharp@google.com> ``` 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. ``` ## David Sharp, 2014-01-28 21:20 Subject: Re: [PATCH] rev-parse: Check argc before using argv[i+1] Message-ID: URL: https://gitlist.dev/e/CAJL_ekuMJByFDVLMCYD4fo9EQy3LKdohsw25WfHqJv9M%3D3S9Kw%40mail.gmail.com In-Reply-To: ``` 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. ``` ## David Sharp, 2014-01-28 21:21 Subject: [PATCH v2] rev-parse: Check argc before using argv[i+1] Message-ID: <1390944060-783-1-git-send-email-dhsharp@google.com> URL: https://gitlist.dev/e/1390944060-783-1-git-send-email-dhsharp%40google.com In-Reply-To: ``` 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 | 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 ``` ## Junio C Hamano, 2014-01-28 21:43 Subject: Re: [PATCH v2] rev-parse: Check argc before using argv[i+1] Message-ID: URL: https://gitlist.dev/e/xmqqmwiflr37.fsf%40gitster.dls.corp.google.com In-Reply-To: <1390944060-783-1-git-send-email-dhsharp@google.com> ``` David Sharp 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 > --- > 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, 2014-01-28 22:01 Subject: Re: [PATCH v2] rev-parse: Check argc before using argv[i+1] Message-ID: <52E828AD.2040609@kdbg.org> URL: https://gitlist.dev/e/52E828AD.2040609%40kdbg.org In-Reply-To: <1390944060-783-1-git-send-email-dhsharp@google.com> ``` 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 ``` ## David Sharp, 2014-01-28 22:02 Subject: Re: [PATCH v2] rev-parse: Check argc before using argv[i+1] Message-ID: URL: https://gitlist.dev/e/CAJL_ekuuu%2BAZH7n4fKzUXqou-nhf96QpcQ-utvMNYRjx4BrTUg%40mail.gmail.com In-Reply-To: ``` On Tue, Jan 28, 2014 at 1:43 PM, Junio C Hamano wrote: > David Sharp 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 >> --- >> 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, 2014-01-28 22:03 Subject: Re: [PATCH v2] rev-parse: Check argc before using argv[i+1] Message-ID: URL: https://gitlist.dev/e/CAJL_ekvepmKn6OmLYZWO%3D%3DRMAbkkArAOLK1iASQG58ShKXU_4g%40mail.gmail.com In-Reply-To: <52E828AD.2040609@kdbg.org> ``` On Tue, Jan 28, 2014 at 2:01 PM, Johannes Sixt 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 > ```