{"thread":{"id":"22555","subject":"[PATCH] archive: fix segfault from too long --format parameter","startedAt":"2010-02-07T07:10:46Z","lastAt":"2010-02-08T00:45:17Z","messageCount":4,"participants":["Jonathan Nieder","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"133857","messageId":"20100207070811.GA26338@progeny.tock","threadId":"22555","inReplyTo":null,"subject":"[PATCH] archive: fix segfault from too long --format parameter","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-07T07:10:46Z","receivedAt":"2010-02-07T07:10:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"‘git archive --format=<string of 25 characters or more>’ overflows a\nlocal buffer, producing a segfault here.\n\nThe context: in commit 0f4b377 (git-archive: infer output format from\nfilename when unspecified, 2009-09-14), the cmd_archive wrapper\nlearned to produce a format argument for the local or remote archive\nmachinery in a local buffer, but that code was missing a bounds check.\n\nSo add the missing check.  As a belt-and-suspenders measure, also use\nsnprintf to make sure the copy afterwards does not overflow.\n\nCc: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\nCc: Dmitry Potapov <dpotapov@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nI noticed this while reading over the archive code.  Thoughts?\n\n builtin-archive.c   |    4 +++-\n t/t5000-tar-tree.sh |    6 ++++++\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 3fb4136..94db00d 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -107,7 +107,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (format) {\n-\t\tsprintf(fmt_opt, \"--format=%s\", format);\n+\t\tif (strlen(format) > sizeof(fmt_opt) - sizeof(\"--format=\"))\n+\t\t\tdie(\"git archive: format is too long: %.50s\", format);\n+\t\tsnprintf(fmt_opt, sizeof(fmt_opt), \"--format=%s\", format);\n \t\t/*\n \t\t * We have enough room in argv[] to muck it in place,\n \t\t * because either --format and/or --output must have\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 0037f63..cf114b2 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -174,6 +174,12 @@ test_expect_success \\\n '\n \n test_expect_success \\\n+    'git archive --format=<long nonsense string>' \\\n+    'format=abacadabra &&\n+    format=\"${format}${format}${format}zip\" &&\n+    test_must_fail git archive \"--format=$format\" HEAD'\n+\n+test_expect_success \\\n     'git archive --format=zip' \\\n     'git archive --format=zip HEAD >d.zip'\n \n-- \n1.7.0.rc1\n"},{"id":"133862","messageId":"4B6E8FD9.7060905@lsrfire.ath.cx","threadId":"22555","inReplyTo":"20100207070811.GA26338@progeny.tock","subject":"Re: [PATCH] archive: fix segfault from too long --format parameter","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-07T10:03:05Z","receivedAt":"2010-02-07T10:03:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.02.2010 08:10, schrieb Jonathan Nieder:\n> ‘git archive --format=<string of 25 characters or more>’ overflows a\n> local buffer, producing a segfault here.\n> \n> The context: in commit 0f4b377 (git-archive: infer output format from\n> filename when unspecified, 2009-09-14), the cmd_archive wrapper\n> learned to produce a format argument for the local or remote archive\n> machinery in a local buffer, but that code was missing a bounds check.\n> \n> So add the missing check.  As a belt-and-suspenders measure, also use\n> snprintf to make sure the copy afterwards does not overflow.\n> \n> Cc: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n> Cc: Dmitry Potapov <dpotapov@gmail.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> I noticed this while reading over the archive code.  Thoughts?\n> \n>  builtin-archive.c   |    4 +++-\n>  t/t5000-tar-tree.sh |    6 ++++++\n>  2 files changed, 9 insertions(+), 1 deletions(-)\n> \n> diff --git a/builtin-archive.c b/builtin-archive.c\n> index 3fb4136..94db00d 100644\n> --- a/builtin-archive.c\n> +++ b/builtin-archive.c\n> @@ -107,7 +107,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tif (format) {\n> -\t\tsprintf(fmt_opt, \"--format=%s\", format);\n> +\t\tif (strlen(format) > sizeof(fmt_opt) - sizeof(\"--format=\"))\n> +\t\t\tdie(\"git archive: format is too long: %.50s\", format);\n> +\t\tsnprintf(fmt_opt, sizeof(fmt_opt), \"--format=%s\", format);\n>  \t\t/*\n>  \t\t * We have enough room in argv[] to muck it in place,\n>  \t\t * because either --format and/or --output must have\n\nThanks.  I think this should go into 1.7.0.  I'd use the same format\nstring for the error message as archive.c, i.e. \"Unknown archive format\n'%s'\".  Later I'd rather do this:\n\n\n builtin-archive.c |   25 ++++++++++++-------------\n 1 files changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 3fb4136..ffe4f4a 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -70,7 +70,7 @@ static const char *format_from_name(const char *filename)\n \t\treturn NULL;\n \text++;\n \tif (!strcasecmp(ext, \"zip\"))\n-\t\treturn \"zip\";\n+\t\treturn \"--format=zip\";\n \treturn NULL;\n }\n \n@@ -92,33 +92,32 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \t\t\t\"retrieve the archive from remote repository <repo>\"),\n \t\tOPT_STRING(0, \"exec\", &exec, \"cmd\",\n \t\t\t\"path to the remote git-upload-archive command\"),\n-\t\tOPT_STRING(0, \"format\", &format, \"fmt\", \"archive format\"),\n \t\tOPT_END()\n \t};\n-\tchar fmt_opt[32];\n \n \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n \t\t\t     PARSE_OPT_KEEP_ALL);\n \n \tif (output) {\n \t\tcreate_output_file(output);\n-\t\tif (!format)\n-\t\t\tformat = format_from_name(output);\n+\t\tformat = format_from_name(output);\n \t}\n \n \tif (format) {\n-\t\tsprintf(fmt_opt, \"--format=%s\", format);\n \t\t/*\n \t\t * We have enough room in argv[] to muck it in place,\n-\t\t * because either --format and/or --output must have\n-\t\t * been given on the original command line if we get\n-\t\t * to this point, and parse_options() must have eaten\n-\t\t * it, i.e. we can add back one element to the array.\n-\t\t * But argv[] may contain \"--\"; we should make it the\n-\t\t * first option.\n+\t\t * because --output must have been given on the\n+\t\t * original command line if we get to this point, and\n+\t\t * parse_options() must have eaten it, i.e. we can add\n+\t\t * back one element to the array.  We add a fake\n+\t\t * --format option at the beginning with the hint\n+\t\t * derived from our output filename.  This way explicit\n+\t\t * --format options can override it, and the fake\n+\t\t * option is inserted before any \"--\" that might have\n+\t\t * been given.\n \t\t */\n \t\tmemmove(argv + 2, argv + 1, sizeof(*argv) * argc);\n-\t\targv[1] = fmt_opt;\n+\t\targv[1] = format;\n \t\targv[++argc] = NULL;\n \t}\n \n"},{"id":"133898","messageId":"4B6F4D0C.2010107@lsrfire.ath.cx","threadId":"22555","inReplyTo":"4B6E8FD9.7060905@lsrfire.ath.cx","subject":"[PATCH] archive: simplify archive format guessing","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-07T23:30:20Z","receivedAt":"2010-02-07T23:30:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Here's my patch again, with a reformatted comment, a renamed variable and\ntwo simple tests.\n-- >8 --\nThe code to guess an output archive's format consumed any --format\noptions and built a new one.  Jonathan noticed that it does so in an\nunsafe way, risking to overflow the static buffer fmt_opt.\n\nChange the code to keep the existing --format options intact and to only\nadd a new one if a format could be guessed based on the output file name.\nThe new option is added as the first one, allowing the existing ones to\noverrule it, i.e. explicit --format options given on the command line win\nover format guesses, as before.\n\nTo simplify the code further, format_from_name() is changed to return the\nfull --format option, thus no potentially dangerous sprintf() calls are\nneeded any more.\n\nReported-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n builtin-archive.c   |   34 ++++++++++++++++------------------\n t/t5000-tar-tree.sh |   10 ++++++++++\n 2 files changed, 26 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 3fb4136..6a887f5 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -70,7 +70,7 @@ static const char *format_from_name(const char *filename)\n \t\treturn NULL;\n \text++;\n \tif (!strcasecmp(ext, \"zip\"))\n-\t\treturn \"zip\";\n+\t\treturn \"--format=zip\";\n \treturn NULL;\n }\n \n@@ -84,7 +84,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \tconst char *exec = \"git-upload-archive\";\n \tconst char *output = NULL;\n \tconst char *remote = NULL;\n-\tconst char *format = NULL;\n+\tconst char *format_option = NULL;\n \tstruct option local_opts[] = {\n \t\tOPT_STRING('o', \"output\", &output, \"file\",\n \t\t\t\"write the archive to this file\"),\n@@ -92,33 +92,31 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \t\t\t\"retrieve the archive from remote repository <repo>\"),\n \t\tOPT_STRING(0, \"exec\", &exec, \"cmd\",\n \t\t\t\"path to the remote git-upload-archive command\"),\n-\t\tOPT_STRING(0, \"format\", &format, \"fmt\", \"archive format\"),\n \t\tOPT_END()\n \t};\n-\tchar fmt_opt[32];\n \n \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n \t\t\t     PARSE_OPT_KEEP_ALL);\n \n \tif (output) {\n \t\tcreate_output_file(output);\n-\t\tif (!format)\n-\t\t\tformat = format_from_name(output);\n+\t\tformat_option = format_from_name(output);\n \t}\n \n-\tif (format) {\n-\t\tsprintf(fmt_opt, \"--format=%s\", format);\n-\t\t/*\n-\t\t * We have enough room in argv[] to muck it in place,\n-\t\t * because either --format and/or --output must have\n-\t\t * been given on the original command line if we get\n-\t\t * to this point, and parse_options() must have eaten\n-\t\t * it, i.e. we can add back one element to the array.\n-\t\t * But argv[] may contain \"--\"; we should make it the\n-\t\t * first option.\n-\t\t */\n+\t/*\n+\t * We have enough room in argv[] to muck it in place, because\n+\t * --output must have been given on the original command line\n+\t * if we get to this point, and parse_options() must have eaten\n+\t * it, i.e. we can add back one element to the array.\n+\t *\n+\t * We add a fake --format option at the beginning, with the\n+\t * format inferred from our output filename.  This way explicit\n+\t * --format options can override it, and the fake option is\n+\t * inserted before any \"--\" that might have been given.\n+\t */\n+\tif (format_option) {\n \t\tmemmove(argv + 2, argv + 1, sizeof(*argv) * argc);\n-\t\targv[1] = fmt_opt;\n+\t\targv[1] = format_option;\n \t\targv[++argc] = NULL;\n \t}\n \ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 0037f63..27bfba5 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -189,6 +189,16 @@ test_expect_success 'git archive --format=zip with --output' \\\n     'git archive --format=zip --output=d2.zip HEAD &&\n     test_cmp d.zip d2.zip'\n \n+test_expect_success 'git archive with --output, inferring format' '\n+\tgit archive --output=d3.zip HEAD &&\n+\ttest_cmp d.zip d3.zip\n+'\n+\n+test_expect_success 'git archive with --output, override inferred format' '\n+\tgit archive --format=tar --output=d4.zip HEAD &&\n+\ttest_cmp b.tar d4.zip\n+'\n+\n $UNZIP -v >/dev/null 2>&1\n if [ $? -eq 127 ]; then\n \tsay \"Skipping ZIP tests, because unzip was not found\"\n-- \n1.7.0.rc1\n"},{"id":"133902","messageId":"7vfx5c368y.fsf@alter.siamese.dyndns.org","threadId":"22555","inReplyTo":"4B6F4D0C.2010107@lsrfire.ath.cx","subject":"Re: [PATCH] archive: simplify archive format guessing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-08T00:45:17Z","receivedAt":"2010-02-08T00:45:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Here's my patch again, with a reformatted comment, a renamed variable and\n> two simple tests.\n\nThanks; will apply to maint and merge upwards.\n"}]}