* [PATCH] archive: fix segfault from too long --format parameter @ 2010-02-07 7:10 Jonathan Nieder 2010-02-07 10:03 ` René Scharfe 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Nieder @ 2010-02-07 7:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Rene Scharfe, Dmitry Potapov, git ‘git archive --format=<string of 25 characters or more>’ overflows a local buffer, producing a segfault here. The context: in commit 0f4b377 (git-archive: infer output format from filename when unspecified, 2009-09-14), the cmd_archive wrapper learned to produce a format argument for the local or remote archive machinery in a local buffer, but that code was missing a bounds check. So add the missing check. As a belt-and-suspenders measure, also use snprintf to make sure the copy afterwards does not overflow. Cc: Rene Scharfe <rene.scharfe@lsrfire.ath.cx> Cc: Dmitry Potapov <dpotapov@gmail.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- I noticed this while reading over the archive code. Thoughts? builtin-archive.c | 4 +++- t/t5000-tar-tree.sh | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletions(-) diff --git a/builtin-archive.c b/builtin-archive.c index 3fb4136..94db00d 100644 --- a/builtin-archive.c +++ b/builtin-archive.c @@ -107,7 +107,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix) } if (format) { - sprintf(fmt_opt, "--format=%s", format); + if (strlen(format) > sizeof(fmt_opt) - sizeof("--format=")) + die("git archive: format is too long: %.50s", format); + snprintf(fmt_opt, sizeof(fmt_opt), "--format=%s", format); /* * We have enough room in argv[] to muck it in place, * because either --format and/or --output must have diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh index 0037f63..cf114b2 100755 --- a/t/t5000-tar-tree.sh +++ b/t/t5000-tar-tree.sh @@ -174,6 +174,12 @@ test_expect_success \ ' test_expect_success \ + 'git archive --format=<long nonsense string>' \ + 'format=abacadabra && + format="${format}${format}${format}zip" && + test_must_fail git archive "--format=$format" HEAD' + +test_expect_success \ 'git archive --format=zip' \ 'git archive --format=zip HEAD >d.zip' -- 1.7.0.rc1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] archive: fix segfault from too long --format parameter 2010-02-07 7:10 [PATCH] archive: fix segfault from too long --format parameter Jonathan Nieder @ 2010-02-07 10:03 ` René Scharfe 2010-02-07 23:30 ` [PATCH] archive: simplify archive format guessing René Scharfe 0 siblings, 1 reply; 4+ messages in thread From: René Scharfe @ 2010-02-07 10:03 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, Dmitry Potapov, git Am 07.02.2010 08:10, schrieb Jonathan Nieder: > ‘git archive --format=<string of 25 characters or more>’ overflows a > local buffer, producing a segfault here. > > The context: in commit 0f4b377 (git-archive: infer output format from > filename when unspecified, 2009-09-14), the cmd_archive wrapper > learned to produce a format argument for the local or remote archive > machinery in a local buffer, but that code was missing a bounds check. > > So add the missing check. As a belt-and-suspenders measure, also use > snprintf to make sure the copy afterwards does not overflow. > > Cc: Rene Scharfe <rene.scharfe@lsrfire.ath.cx> > Cc: Dmitry Potapov <dpotapov@gmail.com> > Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> > --- > I noticed this while reading over the archive code. Thoughts? > > builtin-archive.c | 4 +++- > t/t5000-tar-tree.sh | 6 ++++++ > 2 files changed, 9 insertions(+), 1 deletions(-) > > diff --git a/builtin-archive.c b/builtin-archive.c > index 3fb4136..94db00d 100644 > --- a/builtin-archive.c > +++ b/builtin-archive.c > @@ -107,7 +107,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix) > } > > if (format) { > - sprintf(fmt_opt, "--format=%s", format); > + if (strlen(format) > sizeof(fmt_opt) - sizeof("--format=")) > + die("git archive: format is too long: %.50s", format); > + snprintf(fmt_opt, sizeof(fmt_opt), "--format=%s", format); > /* > * We have enough room in argv[] to muck it in place, > * because either --format and/or --output must have Thanks. I think this should go into 1.7.0. I'd use the same format string for the error message as archive.c, i.e. "Unknown archive format '%s'". Later I'd rather do this: builtin-archive.c | 25 ++++++++++++------------- 1 files changed, 12 insertions(+), 13 deletions(-) diff --git a/builtin-archive.c b/builtin-archive.c index 3fb4136..ffe4f4a 100644 --- a/builtin-archive.c +++ b/builtin-archive.c @@ -70,7 +70,7 @@ static const char *format_from_name(const char *filename) return NULL; ext++; if (!strcasecmp(ext, "zip")) - return "zip"; + return "--format=zip"; return NULL; } @@ -92,33 +92,32 @@ int cmd_archive(int argc, const char **argv, const char *prefix) "retrieve the archive from remote repository <repo>"), OPT_STRING(0, "exec", &exec, "cmd", "path to the remote git-upload-archive command"), - OPT_STRING(0, "format", &format, "fmt", "archive format"), OPT_END() }; - char fmt_opt[32]; argc = parse_options(argc, argv, prefix, local_opts, NULL, PARSE_OPT_KEEP_ALL); if (output) { create_output_file(output); - if (!format) - format = format_from_name(output); + format = format_from_name(output); } if (format) { - sprintf(fmt_opt, "--format=%s", format); /* * We have enough room in argv[] to muck it in place, - * because either --format and/or --output must have - * been given on the original command line if we get - * to this point, and parse_options() must have eaten - * it, i.e. we can add back one element to the array. - * But argv[] may contain "--"; we should make it the - * first option. + * because --output must have been given on the + * original command line if we get to this point, and + * parse_options() must have eaten it, i.e. we can add + * back one element to the array. We add a fake + * --format option at the beginning with the hint + * derived from our output filename. This way explicit + * --format options can override it, and the fake + * option is inserted before any "--" that might have + * been given. */ memmove(argv + 2, argv + 1, sizeof(*argv) * argc); - argv[1] = fmt_opt; + argv[1] = format; argv[++argc] = NULL; } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] archive: simplify archive format guessing 2010-02-07 10:03 ` René Scharfe @ 2010-02-07 23:30 ` René Scharfe 2010-02-08 0:45 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: René Scharfe @ 2010-02-07 23:30 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, Dmitry Potapov, git Here's my patch again, with a reformatted comment, a renamed variable and two simple tests. -- >8 -- The code to guess an output archive's format consumed any --format options and built a new one. Jonathan noticed that it does so in an unsafe way, risking to overflow the static buffer fmt_opt. Change the code to keep the existing --format options intact and to only add a new one if a format could be guessed based on the output file name. The new option is added as the first one, allowing the existing ones to overrule it, i.e. explicit --format options given on the command line win over format guesses, as before. To simplify the code further, format_from_name() is changed to return the full --format option, thus no potentially dangerous sprintf() calls are needed any more. Reported-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx> --- builtin-archive.c | 34 ++++++++++++++++------------------ t/t5000-tar-tree.sh | 10 ++++++++++ 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/builtin-archive.c b/builtin-archive.c index 3fb4136..6a887f5 100644 --- a/builtin-archive.c +++ b/builtin-archive.c @@ -70,7 +70,7 @@ static const char *format_from_name(const char *filename) return NULL; ext++; if (!strcasecmp(ext, "zip")) - return "zip"; + return "--format=zip"; return NULL; } @@ -84,7 +84,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix) const char *exec = "git-upload-archive"; const char *output = NULL; const char *remote = NULL; - const char *format = NULL; + const char *format_option = NULL; struct option local_opts[] = { OPT_STRING('o', "output", &output, "file", "write the archive to this file"), @@ -92,33 +92,31 @@ int cmd_archive(int argc, const char **argv, const char *prefix) "retrieve the archive from remote repository <repo>"), OPT_STRING(0, "exec", &exec, "cmd", "path to the remote git-upload-archive command"), - OPT_STRING(0, "format", &format, "fmt", "archive format"), OPT_END() }; - char fmt_opt[32]; argc = parse_options(argc, argv, prefix, local_opts, NULL, PARSE_OPT_KEEP_ALL); if (output) { create_output_file(output); - if (!format) - format = format_from_name(output); + format_option = format_from_name(output); } - if (format) { - sprintf(fmt_opt, "--format=%s", format); - /* - * We have enough room in argv[] to muck it in place, - * because either --format and/or --output must have - * been given on the original command line if we get - * to this point, and parse_options() must have eaten - * it, i.e. we can add back one element to the array. - * But argv[] may contain "--"; we should make it the - * first option. - */ + /* + * We have enough room in argv[] to muck it in place, because + * --output must have been given on the original command line + * if we get to this point, and parse_options() must have eaten + * it, i.e. we can add back one element to the array. + * + * We add a fake --format option at the beginning, with the + * format inferred from our output filename. This way explicit + * --format options can override it, and the fake option is + * inserted before any "--" that might have been given. + */ + if (format_option) { memmove(argv + 2, argv + 1, sizeof(*argv) * argc); - argv[1] = fmt_opt; + argv[1] = format_option; argv[++argc] = NULL; } diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh index 0037f63..27bfba5 100755 --- a/t/t5000-tar-tree.sh +++ b/t/t5000-tar-tree.sh @@ -189,6 +189,16 @@ test_expect_success 'git archive --format=zip with --output' \ 'git archive --format=zip --output=d2.zip HEAD && test_cmp d.zip d2.zip' +test_expect_success 'git archive with --output, inferring format' ' + git archive --output=d3.zip HEAD && + test_cmp d.zip d3.zip +' + +test_expect_success 'git archive with --output, override inferred format' ' + git archive --format=tar --output=d4.zip HEAD && + test_cmp b.tar d4.zip +' + $UNZIP -v >/dev/null 2>&1 if [ $? -eq 127 ]; then say "Skipping ZIP tests, because unzip was not found" -- 1.7.0.rc1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] archive: simplify archive format guessing 2010-02-07 23:30 ` [PATCH] archive: simplify archive format guessing René Scharfe @ 2010-02-08 0:45 ` Junio C Hamano 0 siblings, 0 replies; 4+ messages in thread From: Junio C Hamano @ 2010-02-08 0:45 UTC (permalink / raw) To: René Scharfe; +Cc: Jonathan Nieder, Dmitry Potapov, git René Scharfe <rene.scharfe@lsrfire.ath.cx> writes: > Here's my patch again, with a reformatted comment, a renamed variable and > two simple tests. Thanks; will apply to maint and merge upwards. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-02-08 0:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-07 7:10 [PATCH] archive: fix segfault from too long --format parameter Jonathan Nieder 2010-02-07 10:03 ` René Scharfe 2010-02-07 23:30 ` [PATCH] archive: simplify archive format guessing René Scharfe 2010-02-08 0:45 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).