{"thread":{"id":"35744","subject":"[PATCH] rev-parse: Check argc before using argv[i+1]","startedAt":"2014-01-27T23:44:22Z","lastAt":"2014-01-28T22:03:20Z","messageCount":8,"participants":["David Sharp","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"233852","messageId":"1390866262-10188-1-git-send-email-dhsharp@google.com","threadId":"35744","inReplyTo":null,"subject":"[PATCH] rev-parse: Check argc before using argv[i+1]","fromName":"David Sharp","fromEmail":"dhsharp@google.com","sentAt":"2014-01-27T23:44:22Z","receivedAt":"2014-01-27T23:44:22Z","isPatch":true,"sender":{"key":"dhsharp@google.com","avatar":null},"body":"Without this patch, git-rev-parse --prefix, --default, or\n--resolve-git-dir, without a value argument, would result in a segfault.\nInstead, die() with a message.\n\nSigned-off-by: David Sharp \n---\n builtin/rev-parse.c | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex aaeb611..3bf65c5 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -547,15 +547,17 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--default\")) {\n-\t\t\t\tdef = argv[i+1];\n-\t\t\t\ti++;\n+\t\t\t\tif (++i >= argc)\n+\t\t\t\t\tdie(\"--default requires an argument\");\n+\t\t\t\tdef = argv[i];\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--prefix\")) {\n-\t\t\t\tprefix = argv[i+1];\n+\t\t\t\tif (++i >= argc)\n+\t\t\t\t\tdie(\"--prefix requires an argument\");\n+\t\t\t\tprefix = argv[i];\n \t\t\t\tstartup_info->prefix = prefix;\n \t\t\t\toutput_prefix = 1;\n-\t\t\t\ti++;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--revs-only\")) {\n@@ -738,9 +740,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n-\t\t\t\tconst char *gitdir = resolve_gitdir(argv[i+1]);\n+\t\t\t\tif (++i >= argc)\n+\t\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n+\t\t\t\tconst char *gitdir = resolve_gitdir(argv[i]);\n \t\t\t\tif (!gitdir)\n-\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i+1]);\n+\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n \t\t\t\tputs(gitdir);\n \t\t\t\tcontinue;\n \t\t\t}\n-- \n1.8.5.3\n"},{"id":"233869","messageId":"xmqqtxcnly39.fsf@gitster.dls.corp.google.com","threadId":"35744","inReplyTo":"1390866262-10188-1-git-send-email-dhsharp@google.com","subject":"Re: [PATCH] rev-parse: Check argc before using argv[i+1]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-28T19:12:42Z","receivedAt":"2014-01-28T19:12:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Sharp writes:\n\n> @@ -738,9 +740,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n> -\t\t\t\tconst char *gitdir = resolve_gitdir(argv[i+1]);\n> +\t\t\t\tif (++i >= argc)\n> +\t\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n> +\t\t\t\tconst char *gitdir = resolve_gitdir(argv[i]);\n> \t\t\t\tif (!gitdir)\n> -\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i+1]);\n> +\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n\nThis adds decl-after-statement. As referencing argv[argc] is not a\ncrime (you will grab a NULL), how about doing it this way to see if\nthe value is a NULL, instead of comparing the index and argc?\n\n\tconst char *gitdir;\n if (!argv[++i])\n \tdie(\"--resolve-git-dir requires an argument\");\n\tgitdir = resolve_gitdir(argv[i]);\n if (!gitdir)\n \tdie(\"not a gitdir '%s'\", argv[i]);\n\nSame comment may apply to other hunks.\n\nThanks.\n"},{"id":"233870","messageId":"CAJL_ekuMJByFDVLMCYD4fo9EQy3LKdohsw25WfHqJv9M=3S9Kw@mail.gmail.com","threadId":"35744","inReplyTo":"xmqqtxcnly39.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] rev-parse: Check argc before using argv[i+1]","fromName":"David Sharp","fromEmail":"dhsharp@google.com","sentAt":"2014-01-28T21:20:43Z","receivedAt":"2014-01-28T21:20:43Z","isPatch":true,"sender":{"key":"dhsharp@google.com","avatar":null},"body":"On Tue, Jan 28, 2014 at 11:12 AM, Junio C Hamano wrote:\n> David Sharp writes:\n>\n>> @@ -738,9 +740,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>> continue;\n>> }\n>> if (!strcmp(arg, \"--resolve-git-dir\")) {\n>> - const char *gitdir = resolve_gitdir(argv[i+1]);\n>> + if (++i >= argc)\n>> + die(\"--resolve-git-dir requires an argument\");\n>> + const char *gitdir = resolve_gitdir(argv[i]);\n>> if (!gitdir)\n>> - die(\"not a gitdir '%s'\", argv[i+1]);\n>> + die(\"not a gitdir '%s'\", argv[i]);\n>\n> This adds decl-after-statement.\n\nSorry, I wasn't aware that git is trying to conform to C90. (It's not\ncompiled with -std=c90, -ansi or -Wdeclaration-after-statement.)\n\n> As referencing argv[argc] is not a\n> crime (you will grab a NULL), how about doing it this way to see if\n> the value is a NULL, instead of comparing the index and argc?\n\nI'm not convinced this is any better, but alright. I did something\nslightly different based on that, though.\n\nv2 patch incoming...\n\n>\n> const char *gitdir;\n> if (!argv[++i])\n> die(\"--resolve-git-dir requires an argument\");\n> gitdir = resolve_gitdir(argv[i]);\n> if (!gitdir)\n> die(\"not a gitdir '%s'\", argv[i]);\n>\n> Same comment may apply to other hunks.\n>\n> Thanks.\n"},{"id":"233871","messageId":"1390944060-783-1-git-send-email-dhsharp@google.com","threadId":"35744","inReplyTo":"CAJL_ekuMJByFDVLMCYD4fo9EQy3LKdohsw25WfHqJv9M=3S9Kw@mail.gmail.com","subject":"[PATCH v2] rev-parse: Check argc before using argv[i+1]","fromName":"David Sharp","fromEmail":"dhsharp@google.com","sentAt":"2014-01-28T21:21:00Z","receivedAt":"2014-01-28T21:21:00Z","isPatch":true,"sender":{"key":"dhsharp@google.com","avatar":null},"body":"Without this patch, git-rev-parse --prefix, --default, or\n--resolve-git-dir, without a value argument, would result in a segfault.\nInstead, die() with a message.\n\nSigned-off-by: David Sharp \n---\n builtin/rev-parse.c | 17 +++++++++++------\n 1 file changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex aaeb611..45901df 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -547,15 +547,17 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--default\")) {\n-\t\t\t\tdef = argv[i+1];\n-\t\t\t\ti++;\n+\t\t\t\tdef = argv[++i];\n+\t\t\t\tif (!def)\n+\t\t\t\t\tdie(\"--default requires an argument\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--prefix\")) {\n-\t\t\t\tprefix = argv[i+1];\n+\t\t\t\tprefix = argv[++i];\n+\t\t\t\tif (!prefix)\n+\t\t\t\t\tdie(\"--prefix requires an argument\");\n \t\t\t\tstartup_info->prefix = prefix;\n \t\t\t\toutput_prefix = 1;\n-\t\t\t\ti++;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--revs-only\")) {\n@@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n-\t\t\t\tconst char *gitdir = resolve_gitdir(argv[i+1]);\n+\t\t\t\tconst char *gitdir = argv[++i];\n \t\t\t\tif (!gitdir)\n-\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i+1]);\n+\t\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n+\t\t\t\tgitdir = resolve_gitdir(gitdir);\n+\t\t\t\tif (!gitdir)\n+\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n \t\t\t\tputs(gitdir);\n \t\t\t\tcontinue;\n \t\t\t}\n-- \n1.8.5.3\n"},{"id":"233872","messageId":"xmqqmwiflr37.fsf@gitster.dls.corp.google.com","threadId":"35744","inReplyTo":"1390944060-783-1-git-send-email-dhsharp@google.com","subject":"Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-28T21:43:56Z","receivedAt":"2014-01-28T21:43:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Sharp writes:\n\n> Without this patch, git-rev-parse --prefix, --default, or\n> --resolve-git-dir, without a value argument, would result in a segfault.\n> Instead, die() with a message.\n\nWhen I sent the review message, I actually was on the fence between\nchecking i vs argc and checking the nullness myself. I realize,\nafter seeing the actual patch below, that we are protecting against\npicking up a NULL and blindly passing it on in the codepaths that\nfollow, so the updated code does look a lot better, at least to me.\n\nThanks.\n\n>\n> Signed-off-by: David Sharp \n> ---\n> builtin/rev-parse.c | 17 +++++++++++------\n> 1 file changed, 11 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index aaeb611..45901df 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -547,15 +547,17 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--default\")) {\n> -\t\t\t\tdef = argv[i+1];\n> -\t\t\t\ti++;\n> +\t\t\t\tdef = argv[++i];\n> +\t\t\t\tif (!def)\n> +\t\t\t\t\tdie(\"--default requires an argument\");\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--prefix\")) {\n> -\t\t\t\tprefix = argv[i+1];\n> +\t\t\t\tprefix = argv[++i];\n> +\t\t\t\tif (!prefix)\n> +\t\t\t\t\tdie(\"--prefix requires an argument\");\n> \t\t\t\tstartup_info->prefix = prefix;\n> \t\t\t\toutput_prefix = 1;\n> -\t\t\t\ti++;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--revs-only\")) {\n> @@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n> -\t\t\t\tconst char *gitdir = resolve_gitdir(argv[i+1]);\n> +\t\t\t\tconst char *gitdir = argv[++i];\n> \t\t\t\tif (!gitdir)\n> -\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i+1]);\n> +\t\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n> +\t\t\t\tgitdir = resolve_gitdir(gitdir);\n> +\t\t\t\tif (!gitdir)\n> +\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n> \t\t\t\tputs(gitdir);\n> \t\t\t\tcontinue;\n> \t\t\t}\n"},{"id":"233874","messageId":"52E828AD.2040609@kdbg.org","threadId":"35744","inReplyTo":"1390944060-783-1-git-send-email-dhsharp@google.com","subject":"Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-01-28T22:01:17Z","receivedAt":"2014-01-28T22:01:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 28.01.2014 22:21, schrieb David Sharp:\n> @@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n> -\t\t\t\tconst char *gitdir = resolve_gitdir(argv[i+1]);\n> +\t\t\t\tconst char *gitdir = argv[++i];\n> \t\t\t\tif (!gitdir)\n> -\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i+1]);\n> +\t\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n> +\t\t\t\tgitdir = resolve_gitdir(gitdir);\n> +\t\t\t\tif (!gitdir)\n> +\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n> \t\t\t\tputs(gitdir);\n> \t\t\t\tcontinue;\n> \t\t\t}\n\nIn this hunk, I don't see where the old code incremented i for the\nargument. Was this another bug lurking that is fixed now?\n\n-- Hannes\n"},{"id":"233875","messageId":"CAJL_ekuuu+AZH7n4fKzUXqou-nhf96QpcQ-utvMNYRjx4BrTUg@mail.gmail.com","threadId":"35744","inReplyTo":"xmqqmwiflr37.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]","fromName":"David Sharp","fromEmail":"dhsharp@google.com","sentAt":"2014-01-28T22:02:04Z","receivedAt":"2014-01-28T22:02:04Z","isPatch":true,"sender":{"key":"dhsharp@google.com","avatar":null},"body":"On Tue, Jan 28, 2014 at 1:43 PM, Junio C Hamano wrote:\n> David Sharp writes:\n>\n>> Without this patch, git-rev-parse --prefix, --default, or\n>> --resolve-git-dir, without a value argument, would result in a segfault.\n>> Instead, die() with a message.\n>\n> When I sent the review message, I actually was on the fence between\n> checking i vs argc and checking the nullness myself. I realize,\n> after seeing the actual patch below, that we are protecting against\n> picking up a NULL and blindly passing it on in the codepaths that\n> follow, so the updated code does look a lot better, at least to me.\n\nIt's a matter of perspective, I guess. When checking the index, to me,\nI saw that it is protecting against reading past the end of an array,\nwhich is at least as bad as sending NULL to codepaths that don't\naccept NULL. I do think that everybody knows that reading past the end\nof an array is bad, while not necessarily everybody knows that the\nargv array is null-terminated.\n\n>\n> Thanks.\n>\n>>\n>> Signed-off-by: David Sharp \n>> ---\n>> builtin/rev-parse.c | 17 +++++++++++------\n>> 1 file changed, 11 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n>> index aaeb611..45901df 100644\n>> --- a/builtin/rev-parse.c\n>> +++ b/builtin/rev-parse.c\n>> @@ -547,15 +547,17 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>> continue;\n>> }\n>> if (!strcmp(arg, \"--default\")) {\n>> - def = argv[i+1];\n>> - i++;\n>> + def = argv[++i];\n>> + if (!def)\n>> + die(\"--default requires an argument\");\n>> continue;\n>> }\n>> if (!strcmp(arg, \"--prefix\")) {\n>> - prefix = argv[i+1];\n>> + prefix = argv[++i];\n>> + if (!prefix)\n>> + die(\"--prefix requires an argument\");\n>> startup_info->prefix = prefix;\n>> output_prefix = 1;\n>> - i++;\n>> continue;\n>> }\n>> if (!strcmp(arg, \"--revs-only\")) {\n>> @@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>> continue;\n>> }\n>> if (!strcmp(arg, \"--resolve-git-dir\")) {\n>> - const char *gitdir = resolve_gitdir(argv[i+1]);\n>> + const char *gitdir = argv[++i];\n>> if (!gitdir)\n>> - die(\"not a gitdir '%s'\", argv[i+1]);\n>> + die(\"--resolve-git-dir requires an argument\");\n>> + gitdir = resolve_gitdir(gitdir);\n>> + if (!gitdir)\n>> + die(\"not a gitdir '%s'\", argv[i]);\n>> puts(gitdir);\n>> continue;\n>> }\n"},{"id":"233876","messageId":"CAJL_ekvepmKn6OmLYZWO==RMAbkkArAOLK1iASQG58ShKXU_4g@mail.gmail.com","threadId":"35744","inReplyTo":"52E828AD.2040609@kdbg.org","subject":"Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]","fromName":"David Sharp","fromEmail":"dhsharp@google.com","sentAt":"2014-01-28T22:03:20Z","receivedAt":"2014-01-28T22:03:20Z","isPatch":true,"sender":{"key":"dhsharp@google.com","avatar":null},"body":"On Tue, Jan 28, 2014 at 2:01 PM, Johannes Sixt wrote:\n> Am 28.01.2014 22:21, schrieb David Sharp:\n>> @@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>> continue;\n>> }\n>> if (!strcmp(arg, \"--resolve-git-dir\")) {\n>> - const char *gitdir = resolve_gitdir(argv[i+1]);\n>> + const char *gitdir = argv[++i];\n>> if (!gitdir)\n>> - die(\"not a gitdir '%s'\", argv[i+1]);\n>> + die(\"--resolve-git-dir requires an argument\");\n>> + gitdir = resolve_gitdir(gitdir);\n>> + if (!gitdir)\n>> + die(\"not a gitdir '%s'\", argv[i]);\n>> puts(gitdir);\n>> continue;\n>> }\n>\n> In this hunk, I don't see where the old code incremented i for the\n> argument. Was this another bug lurking that is fixed now?\n\nYup. I did notice that too. Should have mentioned it in the description.\n\n>\n> -- Hannes\n>\n"}]}