{"thread":{"id":"60595","subject":"[BUG] rev-list doesn't validate arguments to -n option","startedAt":"2023-12-07T22:12:15Z","lastAt":"2023-12-12T22:05:07Z","messageCount":6,"participants":["Britton Kerin","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"485456","messageId":"CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com","threadId":"60595","inReplyTo":null,"subject":"[BUG] rev-list doesn't validate arguments to -n option","fromName":"Britton Kerin","fromEmail":"britton.kerin@gmail.com","sentAt":"2023-12-07T22:12:01Z","receivedAt":"2023-12-07T22:12:15Z","isPatch":false,"sender":{"key":"britton.kerin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7434164?v=4"},"body":"It tolerates non-numeric arguments and garbage after a number:\n\nFor example:\n\n$ # -n 1 means same as -n 0:\n$ git rev-list -n q newest_commit\n$ git rev-list -n 0 newest_commit\n$ # Garbage after number is tolerated:\n$ git rev-list -n 1q newest_commit\n3be33f83695088d968cf084a1a08bdcde25a8d7a\n$ git rev-list -n 2q newest_commit\n3be33f83695088d968cf084a1a08bdcde25a8d7a\n286e62e1b68e39334978e6222cbff187ecc17bcf\n"},{"id":"485478","messageId":"xmqqy1e41lf5.fsf@gitster.g","threadId":"60595","inReplyTo":"CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com","subject":"Re: [BUG] rev-list doesn't validate arguments to -n option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-08T20:36:30Z","receivedAt":"2023-12-08T20:36:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Britton Kerin <britton.kerin@gmail.com> writes:\n\n> It tolerates non-numeric arguments and garbage after a number:\n>\n> For example:\n>\n> $ # -n 1 means same as -n 0:\n> $ git rev-list -n q newest_commit\n> $ git rev-list -n 0 newest_commit\n> $ # Garbage after number is tolerated:\n> $ git rev-list -n 1q newest_commit\n> 3be33f83695088d968cf084a1a08bdcde25a8d7a\n> $ git rev-list -n 2q newest_commit\n> 3be33f83695088d968cf084a1a08bdcde25a8d7a\n> 286e62e1b68e39334978e6222cbff187ecc17bcf\n\nIndeed, unlike the option parsing for most commands, \"rev-list\" and\n\"log\" family of commands in the oldest part of the system still use\nhand-crafted option parsing and most notably use atoi() instead of a\nmore robust strtol() family of functions.  The same issue is present\nfor parsing things like \"--max-count=1q\" and \"--skip=1q\".\n\nPerhaps a change along this line, plus a few new tests, would\nsuffice.  There are others (like \"--max-age\" we see in the post\ncontext of the patch) that use atoi() but they probably should not\nshare the same machinery (most importantly, the type of the value)\nas \"--max-count\", so I didn't touch it in the below.\n\n revision.c | 21 ++++++++++++++-------\n 1 file changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git c/revision.c w/revision.c\nindex 00d5c29bfc..99e838bbd1 100644\n--- c/revision.c\n+++ w/revision.c\n@@ -2223,6 +2223,15 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)\n \tadd_grep(revs, pattern, GREP_PATTERN_BODY);\n }\n \n+static int parse_count(const char *arg)\n+{\n+\tint count;\n+\n+\tif (strtol_i(arg, 10, &count) < 0 || count < 0)\n+\t\tdie(\"'%s': not a non-negative integer\", arg);\n+\treturn count;\n+}\n+\n static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n \t\t\t       int *unkc, const char **unkv,\n \t\t\t       const struct setup_revision_opt* opt)\n@@ -2249,26 +2258,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t}\n \n \tif ((argcount = parse_long_opt(\"max-count\", argv, &optarg))) {\n-\t\trevs->max_count = atoi(optarg);\n+\t\trevs->max_count = parse_count(optarg);\n \t\trevs->no_walk = 0;\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"skip\", argv, &optarg))) {\n-\t\trevs->skip_count = atoi(optarg);\n+\t\trevs->skip_count = parse_count(optarg);\n \t\treturn argcount;\n \t} else if ((*arg == '-') && isdigit(arg[1])) {\n \t\t/* accept -<digit>, like traditional \"head\" */\n-\t\tif (strtol_i(arg + 1, 10, &revs->max_count) < 0 ||\n-\t\t    revs->max_count < 0)\n-\t\t\tdie(\"'%s': not a non-negative integer\", arg + 1);\n+\t\trevs->max_count = parse_count(arg + 1);\n \t\trevs->no_walk = 0;\n \t} else if (!strcmp(arg, \"-n\")) {\n \t\tif (argc <= 1)\n \t\t\treturn error(\"-n requires an argument\");\n-\t\trevs->max_count = atoi(argv[1]);\n+\t\trevs->max_count = parse_count(argv[1]);\n \t\trevs->no_walk = 0;\n \t\treturn 2;\n \t} else if (skip_prefix(arg, \"-n\", &optarg)) {\n-\t\trevs->max_count = atoi(optarg);\n+\t\trevs->max_count = parse_count(optarg);\n \t\trevs->no_walk = 0;\n \t} else if ((argcount = parse_long_opt(\"max-age\", argv, &optarg))) {\n \t\trevs->max_age = atoi(optarg);\n"},{"id":"485490","messageId":"xmqq5y181fx0.fsf_-_@gitster.g","threadId":"60595","inReplyTo":"xmqqy1e41lf5.fsf@gitster.g","subject":"[PATCH] revision: parse integer arguments to --max-count, --skip, etc., more carefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-08T22:35:23Z","receivedAt":"2023-12-08T22:35:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"rev-list\" and other commands in the \"log\" family, being the\noldest part of the system, use their own custom argument parsers,\nand integer values of some options are parsed with atoi(), which\nallows a non-digit after the number (e.g., \"1q\") to be silently\nignored.  As a natural consequence, an argument that does not begin\nwith a digit (e.g., \"q\") silently becomes zero, too.\n\nSwitch to use strtol_i() and parse_timestamp() appropriately to\ncatch bogus input.\n\nNote that one may naïvely expect that --max-count, --skip, etc., to\nonly take non-negative values, but we must allow them to also take\nnegative values, as an escape hatch to countermand a limit set by an\nearlier option on the command line; the underlying variables are\ninitialized to (-1) and \"--max-count=-1\", for example, is a\nlegitimate way to reinitialize the limit.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n revision.c                 | 41 ++++++++++++++++++++++++++++----------\n t/t6005-rev-list-count.sh  | 15 +++++++++++++-\n t/t6009-rev-list-parent.sh | 11 ++++++++++\n 3 files changed, 55 insertions(+), 12 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 0ae1c76db3..8cda6e43d4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2214,6 +2214,27 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)\n \tadd_grep(revs, pattern, GREP_PATTERN_BODY);\n }\n \n+static int parse_count(const char *arg)\n+{\n+\tint count;\n+\n+\tif (strtol_i(arg, 10, &count) < 0)\n+\t\tdie(\"'%s': not an integer\", arg);\n+\treturn count;\n+}\n+\n+static timestamp_t parse_age(const char *arg)\n+{\n+\ttimestamp_t num;\n+\tchar *p;\n+\n+\terrno = 0;\n+\tnum = parse_timestamp(arg, &p, 10);\n+\tif (errno || *p || p == arg)\n+\t\tdie(\"'%s': not a number of seconds since epoch\", arg);\n+\treturn num;\n+}\n+\n static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n \t\t\t       int *unkc, const char **unkv,\n \t\t\t       const struct setup_revision_opt* opt)\n@@ -2240,29 +2261,27 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t}\n \n \tif ((argcount = parse_long_opt(\"max-count\", argv, &optarg))) {\n-\t\trevs->max_count = atoi(optarg);\n+\t\trevs->max_count = parse_count(optarg);\n \t\trevs->no_walk = 0;\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"skip\", argv, &optarg))) {\n-\t\trevs->skip_count = atoi(optarg);\n+\t\trevs->skip_count = parse_count(optarg);\n \t\treturn argcount;\n \t} else if ((*arg == '-') && isdigit(arg[1])) {\n \t\t/* accept -<digit>, like traditional \"head\" */\n-\t\tif (strtol_i(arg + 1, 10, &revs->max_count) < 0 ||\n-\t\t    revs->max_count < 0)\n-\t\t\tdie(\"'%s': not a non-negative integer\", arg + 1);\n+\t\trevs->max_count = parse_count(arg + 1);\n \t\trevs->no_walk = 0;\n \t} else if (!strcmp(arg, \"-n\")) {\n \t\tif (argc <= 1)\n \t\t\treturn error(\"-n requires an argument\");\n-\t\trevs->max_count = atoi(argv[1]);\n+\t\trevs->max_count = parse_count(argv[1]);\n \t\trevs->no_walk = 0;\n \t\treturn 2;\n \t} else if (skip_prefix(arg, \"-n\", &optarg)) {\n-\t\trevs->max_count = atoi(optarg);\n+\t\trevs->max_count = parse_count(optarg);\n \t\trevs->no_walk = 0;\n \t} else if ((argcount = parse_long_opt(\"max-age\", argv, &optarg))) {\n-\t\trevs->max_age = atoi(optarg);\n+\t\trevs->max_age = parse_age(optarg);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"since\", argv, &optarg))) {\n \t\trevs->max_age = approxidate(optarg);\n@@ -2274,7 +2293,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->max_age = approxidate(optarg);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"min-age\", argv, &optarg))) {\n-\t\trevs->min_age = atoi(optarg);\n+\t\trevs->min_age = parse_age(optarg);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"before\", argv, &optarg))) {\n \t\trevs->min_age = approxidate(optarg);\n@@ -2362,11 +2381,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n \t\trevs->max_parents = 1;\n \t} else if (skip_prefix(arg, \"--min-parents=\", &optarg)) {\n-\t\trevs->min_parents = atoi(optarg);\n+\t\trevs->min_parents = parse_count(optarg);\n \t} else if (!strcmp(arg, \"--no-min-parents\")) {\n \t\trevs->min_parents = 0;\n \t} else if (skip_prefix(arg, \"--max-parents=\", &optarg)) {\n-\t\trevs->max_parents = atoi(optarg);\n+\t\trevs->max_parents = parse_count(optarg);\n \t} else if (!strcmp(arg, \"--no-max-parents\")) {\n \t\trevs->max_parents = -1;\n \t} else if (!strcmp(arg, \"--boundary\")) {\ndiff --git a/t/t6005-rev-list-count.sh b/t/t6005-rev-list-count.sh\nindex 0729f800c3..62538f5a02 100755\n--- a/t/t6005-rev-list-count.sh\n+++ b/t/t6005-rev-list-count.sh\n@@ -18,13 +18,23 @@ test_expect_success 'no options' '\n '\n \n test_expect_success '--max-count' '\n+\ttest_must_fail git rev-list --max-count=1q HEAD 2>error &&\n+\tgrep \"not an integer\" error &&\n+\n \ttest_stdout_line_count = 0 git rev-list HEAD --max-count=0 &&\n \ttest_stdout_line_count = 3 git rev-list HEAD --max-count=3 &&\n \ttest_stdout_line_count = 5 git rev-list HEAD --max-count=5 &&\n-\ttest_stdout_line_count = 5 git rev-list HEAD --max-count=10\n+\ttest_stdout_line_count = 5 git rev-list HEAD --max-count=10 &&\n+\ttest_stdout_line_count = 5 git rev-list HEAD --max-count=-1\n '\n \n test_expect_success '--max-count all forms' '\n+\ttest_must_fail git rev-list -1q HEAD 2>error &&\n+\tgrep \"not an integer\" error &&\n+\ttest_must_fail git rev-list --1 HEAD &&\n+\ttest_must_fail git rev-list -n 1q HEAD 2>error &&\n+\tgrep \"not an integer\" error &&\n+\n \ttest_stdout_line_count = 1 git rev-list HEAD --max-count=1 &&\n \ttest_stdout_line_count = 1 git rev-list HEAD -1 &&\n \ttest_stdout_line_count = 1 git rev-list HEAD -n1 &&\n@@ -32,6 +42,9 @@ test_expect_success '--max-count all forms' '\n '\n \n test_expect_success '--skip' '\n+\ttest_must_fail git rev-list --skip 1q HEAD 2>error &&\n+\tgrep \"not an integer\" error &&\n+\n \ttest_stdout_line_count = 5 git rev-list HEAD --skip=0 &&\n \ttest_stdout_line_count = 2 git rev-list HEAD --skip=3 &&\n \ttest_stdout_line_count = 0 git rev-list HEAD --skip=5 &&\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 5a67bbc760..9c9a8459af 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -62,6 +62,17 @@ test_expect_success 'setup roots, merges and octopuses' '\n \tgit checkout main\n '\n \n+test_expect_success 'parse --max-parents & --min-parents' '\n+\ttest_must_fail git rev-list --max-parents=1q HEAD 2>error &&\n+\tgrep \"not an integer\" error &&\n+\n+\ttest_must_fail git rev-list --min-parents=1q HEAD 2>error &&\n+\tgrep \"not an integer\" error &&\n+\n+\tgit rev-list --max-parents=1 --min-parents=1 HEAD &&\n+\tgit rev-list --max-parents=-1 --min-parents=-1 HEAD\n+'\n+\n test_expect_success 'rev-list roots' '\n \n \tcheck_revlist \"--max-parents=0\" one five\n-- \n2.43.0\n\n"},{"id":"485552","messageId":"20231212013045.GE376323@coredump.intra.peff.net","threadId":"60595","inReplyTo":"xmqq5y181fx0.fsf_-_@gitster.g","subject":"Re: [PATCH] revision: parse integer arguments to --max-count, --skip, etc., more carefully","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T01:30:45Z","receivedAt":"2023-12-12T01:30:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 09, 2023 at 07:35:23AM +0900, Junio C Hamano wrote:\n\n> The \"rev-list\" and other commands in the \"log\" family, being the\n> oldest part of the system, use their own custom argument parsers,\n> and integer values of some options are parsed with atoi(), which\n> allows a non-digit after the number (e.g., \"1q\") to be silently\n> ignored.  As a natural consequence, an argument that does not begin\n> with a digit (e.g., \"q\") silently becomes zero, too.\n> \n> Switch to use strtol_i() and parse_timestamp() appropriately to\n> catch bogus input.\n> \n> Note that one may naïvely expect that --max-count, --skip, etc., to\n> only take non-negative values, but we must allow them to also take\n> negative values, as an escape hatch to countermand a limit set by an\n> earlier option on the command line; the underlying variables are\n> initialized to (-1) and \"--max-count=-1\", for example, is a\n> legitimate way to reinitialize the limit.\n\nThis all looks pretty reasonable to me.\n\nI couldn't help but think, though, that surely we have some helpers for\nthis already? But the closest seems to be git_parse_int(), which also\nallows unit factors. I'm not sure if allowing \"-n 1k\" would be a feature\nor a bug. ;)\n\nI guess \"strtol_i()\" maybe is that helper already, though I did not even\nknow it existed. Looks like it goes back to 2007, and is seldom used. I\nwonder if there are more spots that could benefit.\n\nI don't think there is any such helper for timestamps, but the checks in\nyour parser look good (strtol_i() checks for overflow as we cast to int,\nbut I don't think we need to do the same here since timestamp_t and\nparse_timestamp() should be matched).\n\n-Peff\n"},{"id":"485579","messageId":"xmqqjzpjsbjl.fsf@gitster.g","threadId":"60595","inReplyTo":"20231212013045.GE376323@coredump.intra.peff.net","subject":"Re: [PATCH] revision: parse integer arguments to --max-count, --skip, etc., more carefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-12T15:09:02Z","receivedAt":"2023-12-12T15:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This all looks pretty reasonable to me.\n>\n> I couldn't help but think, though, that surely we have some helpers for\n> this already? But the closest seems to be git_parse_int(), which also\n> allows unit factors. I'm not sure if allowing \"-n 1k\" would be a feature\n> or a bug. ;)\n\nThe change in question is meant to be a pure fix to replace a careless\nuse of atoi().  I do not mind to see a separate patch to add such a\nfeature later on top.\n\n> I guess \"strtol_i()\" maybe is that helper already, though I did not even\n> know it existed. Looks like it goes back to 2007, and is seldom used.\n\nI didn't know about it, either ;-) I used it only because there is\none use of it, among places that used atoi() I wanted to tighten.\n\n> I wonder if there are more spots that could benefit.\n\n\"git grep -e 'atoi('\" would give somebody motivated a decent set of\n#microproject ideas, but many hits are not suited for strtol_i(),\nwhich is designed to parse an integer at the end of a string.  Some\nplaces use atoi() immediately followed by strspn() to skip over\ndigits, which means they are parsing an integer and want to continue\nreading after the integer, which is incompatible with what\nstrtol_i() wants to do.  They need either a separate helper or an\nupdated strtol_i() that optionally allows you to parse the prefix\nand report where the integer ended, e.g., something like:\n\n#define strtol_i(s,b,r) strtol_i2((s), (b), (r), NULL)\nstatic inline int strtol_i2(char const *s, int base, int *result, char **endp)\n{\n\tlong ul;\n        char *dummy = NULL;\n\n        if (!endp)\n\t\tendp = &dummy;\n\terrno = 0;\n\tul = strtol(s, &endp, base);\n\tif (errno ||\n\t    /*\n\t     * if we are told to parse to the end of the string by\n\t     * passing NULL to endp, it is an error to have any\n\t     * remaining character after the digits.\n\t     */\n            (dummy && *dummy) ||\n\t    endp == s || (int) ul != ul)\n\t\treturn -1;\n\t*result = ul;\n\treturn 0;\n}\n\nperhaps.\n"},{"id":"485593","messageId":"20231212220505.GA1220605@coredump.intra.peff.net","threadId":"60595","inReplyTo":"xmqqjzpjsbjl.fsf@gitster.g","subject":"Re: [PATCH] revision: parse integer arguments to --max-count, --skip, etc., more carefully","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T22:05:05Z","receivedAt":"2023-12-12T22:05:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 12, 2023 at 07:09:02AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This all looks pretty reasonable to me.\n> >\n> > I couldn't help but think, though, that surely we have some helpers for\n> > this already? But the closest seems to be git_parse_int(), which also\n> > allows unit factors. I'm not sure if allowing \"-n 1k\" would be a feature\n> > or a bug. ;)\n> \n> The change in question is meant to be a pure fix to replace a careless\n> use of atoi().  I do not mind to see a separate patch to add such a\n> feature later on top.\n\nOh, I mostly meant that I would have turned to git_parse_int() as that\nalready-existing helper, but it is not suitable because of the extra\nunit-handling. I think your patch draws the line in the right place.\n\n> > I wonder if there are more spots that could benefit.\n> \n> \"git grep -e 'atoi('\" would give somebody motivated a decent set of\n> #microproject ideas, but many hits are not suited for strtol_i(),\n> which is designed to parse an integer at the end of a string.  Some\n> places use atoi() immediately followed by strspn() to skip over\n> digits, which means they are parsing an integer and want to continue\n> reading after the integer, which is incompatible with what\n> strtol_i() wants to do.  They need either a separate helper or an\n> updated strtol_i() that optionally allows you to parse the prefix\n> and report where the integer ended, e.g., something like:\n\nYeah, I agree this might be a good microproject (or leftoverbits) area,\nand the semantics for the helper you propose make sense to me.\n\n-Peff\n"}]}