{"thread":{"id":"7205","subject":"[PATCH] git-grep: don't use sscanf","startedAt":"2007-03-12T12:11:29Z","lastAt":"2007-03-12T12:11:29Z","messageCount":1,"participants":["Jim Meyering"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"36900","messageId":"87lki2r79q.fsf@rho.meyering.net","threadId":"7205","inReplyTo":null,"subject":"[PATCH] git-grep: don't use sscanf","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-03-12T12:11:29Z","receivedAt":"2007-03-12T12:11:29Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"If you use scanf or sscanf to parse integers, your code probably\naccepts bogus inputs.  For example, builtin-grep (aka git-grep) uses\nsscanf(scan, \"%u\", &num) to parse the integer argument to -A, -B, -C.\nCurrently, \"-C 1,000\" and \"-C 4294967297\" are both treated just like\n\"-C 1\":\n\n    $ git-grep -h -C 4294967297 juggle\n    out and you may find it easier to switch back and forth if you\n    juggle multiple lines of development simultaneously. Of\n    course, you will pay the price of more disk usage to hold\n\nThe obvious fix is to use strtoul instead.  But using a bare strtoul is\ntoo messy, at least when done properly, so I've added a wrapper function.\n\nThe new function in the patch below belongs elsewhere if it would be\nuseful in replacing any of the four remaining uses of sscanf.\n\nOne final note:  With this change, I get a slightly different\ndiagnostic depending on the context size:\n\n  $ ./git-grep -h -C 4294967296 juggle\n  fatal: 4294967296: invalid context length argument\n  [Exit 128]\n  $ ./git-grep -h -C 4294967295 juggle\n  grep: 4294967295: invalid context length argument\n\n  [Exit 1]\n\nA common convention that makes it easy to identify the source\nof a diagnostic is to include the program name before the first \":\".\nWhether that should be \"git\" or \"git-grep\" is another question.\nUsing \"grep\" or \"fatal\" is misleading.\n\nSigned-off-by: Jim Meyering <jim@meyering.net>\n---\n builtin-grep.c |   14 +++++++++++++-\n 1 files changed, 13 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 694da5b..9a151bf 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -431,6 +431,18 @@ static const char emsg_missing_context_len[] =\n static const char emsg_missing_argument[] =\n \"option requires an argument -%s\";\n\n+static int\n+strtoul_ui (char const *s, unsigned int *result)\n+{\n+\tchar *p;\n+\terrno = 0;\n+\tunsigned long ul = strtoul (s, &p, 10);\n+\tif (errno || *p || p == s || (unsigned int) ul != ul)\n+\t\treturn 0;\n+\t*result = ul;\n+\treturn 1;\n+}\n+\n int cmd_grep(int argc, const char **argv, const char *prefix)\n {\n \tint hit = 0;\n@@ -553,7 +565,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t\tscan = arg + 1;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tif (sscanf(scan, \"%u\", &num) != 1)\n+\t\t\tif (!strtoul_ui (scan, &num))\n \t\t\t\tdie(emsg_invalid_context_len, scan);\n \t\t\tswitch (arg[1]) {\n \t\t\tcase 'A':\n--\n1.5.0.3.353.g8a3f\n"}]}