* [PATCH] maint: check return of split_cmdline to avoid bad config strings @ 2008-09-24 6:10 Deskin Miller 2008-09-24 9:28 ` Miklos Vajna 0 siblings, 1 reply; 4+ messages in thread From: Deskin Miller @ 2008-09-24 6:10 UTC (permalink / raw) To: git; +Cc: gitster >From 7679d395856d17d7853eea0fc196435eab9be08f Mon Sep 17 00:00:00 2001 From: Deskin Miller <deskinm@umich.edu> Date: Mon, 22 Sep 2008 11:06:41 -0400 Subject: [PATCH] maint: check return of split_cmdline to avoid bad config strings Signed-off-by: Deskin Miller <deskinm@umich.edu> --- As the testcase demonstrates, it's possible to have split_cmdline return -1 and deallocate any memory it's allocated, if the config string is missing an end quote. In both the cases below, the return isn't checked, causing a pretty immediate segfault. builtin-merge.c | 2 ++ git.c | 2 ++ t/t1300-repo-config.sh | 10 ++++++++++ 3 files changed, 14 insertions(+), 0 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index b280444..dcaf368 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -442,6 +442,8 @@ static int git_merge_config(const char *k, const char *v, void *cb) buf = xstrdup(v); argc = split_cmdline(buf, &argv); + if (argc < 0) + die("Bad branch.%s.mergeoptions string", branch); argv = xrealloc(argv, sizeof(*argv) * (argc + 2)); memmove(argv + 1, argv, sizeof(*argv) * (argc + 1)); argc++; diff --git a/git.c b/git.c index fdb0f71..5582c51 100644 --- a/git.c +++ b/git.c @@ -162,6 +162,8 @@ static int handle_alias(int *argcp, const char ***argv) alias_string + 1, alias_command); } count = split_cmdline(alias_string, &new_argv); + if (count < 0) + die("Bad alias.%s string", alias_command); option_count = handle_options(&new_argv, &count, &envchanged); if (envchanged) die("alias '%s' changes environment variables\n" diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 64567fb..3794d23 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -741,4 +741,14 @@ test_expect_success 'symlinked configuration' ' ' +test_expect_success 'check split_cmdline return' " + git config alias.split-cmdline-fix 'echo \"' && + git split-cmdline-fix || test \$? = 128 && + echo foo > foo && + git add foo && + git commit -m 'initial commit' && + git config branch.master.mergeoptions 'echo \"' && + git merge master || test \$? = 128 + " + test_done -- 1.6.0.2.GIT ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] maint: check return of split_cmdline to avoid bad config strings 2008-09-24 6:10 [PATCH] maint: check return of split_cmdline to avoid bad config strings Deskin Miller @ 2008-09-24 9:28 ` Miklos Vajna 2008-09-24 14:50 ` [PATCH v2] " Deskin Miller 0 siblings, 1 reply; 4+ messages in thread From: Miklos Vajna @ 2008-09-24 9:28 UTC (permalink / raw) To: Deskin Miller; +Cc: git, gitster [-- Attachment #1: Type: text/plain, Size: 1541 bytes --] On Wed, Sep 24, 2008 at 02:10:28AM -0400, Deskin Miller <deskinm@umich.edu> wrote: > As the testcase demonstrates, it's possible to have split_cmdline return -1 and > deallocate any memory it's allocated, if the config string is missing an end > quote. In both the cases below, the return isn't checked, causing a pretty > immediate segfault. I think this could go to the commit message. > diff --git a/builtin-merge.c b/builtin-merge.c > index b280444..dcaf368 100644 > --- a/builtin-merge.c > +++ b/builtin-merge.c > @@ -442,6 +442,8 @@ static int git_merge_config(const char *k, const char *v, void *cb) > > buf = xstrdup(v); > argc = split_cmdline(buf, &argv); > + if (argc < 0) > + die("Bad branch.%s.mergeoptions string", branch); > argv = xrealloc(argv, sizeof(*argv) * (argc + 2)); > memmove(argv + 1, argv, sizeof(*argv) * (argc + 1)); > argc++; ACK. > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh > index 64567fb..3794d23 100755 > --- a/t/t1300-repo-config.sh > +++ b/t/t1300-repo-config.sh > @@ -741,4 +741,14 @@ test_expect_success 'symlinked configuration' ' > > ' > > +test_expect_success 'check split_cmdline return' " > + git config alias.split-cmdline-fix 'echo \"' && > + git split-cmdline-fix || test \$? = 128 && > + echo foo > foo && > + git add foo && > + git commit -m 'initial commit' && > + git config branch.master.mergeoptions 'echo \"' && > + git merge master || test \$? = 128 > + " Why don't you use test_must_fail here? [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] maint: check return of split_cmdline to avoid bad config strings 2008-09-24 9:28 ` Miklos Vajna @ 2008-09-24 14:50 ` Deskin Miller 2008-09-24 22:32 ` Miklos Vajna 0 siblings, 1 reply; 4+ messages in thread From: Deskin Miller @ 2008-09-24 14:50 UTC (permalink / raw) To: Miklos Vajna; +Cc: git, gitster >From 0d52afa86d757220fefa2440c78c81bd2c8b790a Mon Sep 17 00:00:00 2001 From: Deskin Miller <deskinm@umich.edu> Date: Mon, 22 Sep 2008 11:06:41 -0400 Subject: [PATCH] maint: check return of split_cmdline to avoid bad config strings As the testcase demonstrates, it's possible for split_cmdline to return -1 and deallocate any memory it's allocated, if the config string is missing an end quote. In both the cases below, which are the only calling sites, the return isn't checked, and using the pointer causes a pretty immediate segfault. Signed-off-by: Deskin Miller <deskinm@umich.edu> --- On Wed, Sep 24, 2008 at 11:28:47AM +0200, Miklos Vajna wrote: > On Wed, Sep 24, 2008 at 02:10:28AM -0400, Deskin Miller <deskinm@umich.edu> wrote: > > As the testcase demonstrates, it's possible to have split_cmdline return -1 and > > deallocate any memory it's allocated, if the config string is missing an end > > quote. In both the cases below, the return isn't checked, causing a pretty > > immediate segfault. > > I think this could go to the commit message. Done in v2. > > + git merge master || test \$? = 128 > > + " > > Why don't you use test_must_fail here? Didn't know about it. Fixed in v2. Thanks for the advice. builtin-merge.c | 2 ++ git.c | 2 ++ t/t1300-repo-config.sh | 10 ++++++++++ 3 files changed, 14 insertions(+), 0 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index b280444..dcaf368 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -442,6 +442,8 @@ static int git_merge_config(const char *k, const char *v, void *cb) buf = xstrdup(v); argc = split_cmdline(buf, &argv); + if (argc < 0) + die("Bad branch.%s.mergeoptions string", branch); argv = xrealloc(argv, sizeof(*argv) * (argc + 2)); memmove(argv + 1, argv, sizeof(*argv) * (argc + 1)); argc++; diff --git a/git.c b/git.c index fdb0f71..5582c51 100644 --- a/git.c +++ b/git.c @@ -162,6 +162,8 @@ static int handle_alias(int *argcp, const char ***argv) alias_string + 1, alias_command); } count = split_cmdline(alias_string, &new_argv); + if (count < 0) + die("Bad alias.%s string", alias_command); option_count = handle_options(&new_argv, &count, &envchanged); if (envchanged) die("alias '%s' changes environment variables\n" diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 64567fb..11b82f4 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -741,4 +741,14 @@ test_expect_success 'symlinked configuration' ' ' +test_expect_success 'check split_cmdline return' " + git config alias.split-cmdline-fix 'echo \"' && + test_must_fail git split-cmdline-fix && + echo foo > foo && + git add foo && + git commit -m 'initial commit' && + git config branch.master.mergeoptions 'echo \"' && + test_must_fail git merge master + " + test_done -- 1.6.0.2.GIT ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] maint: check return of split_cmdline to avoid bad config strings 2008-09-24 14:50 ` [PATCH v2] " Deskin Miller @ 2008-09-24 22:32 ` Miklos Vajna 0 siblings, 0 replies; 4+ messages in thread From: Miklos Vajna @ 2008-09-24 22:32 UTC (permalink / raw) To: Deskin Miller; +Cc: git, gitster [-- Attachment #1: Type: text/plain, Size: 521 bytes --] On Wed, Sep 24, 2008 at 10:50:29AM -0400, Deskin Miller <deskinm@umich.edu> wrote: > As the testcase demonstrates, it's possible for split_cmdline to return -1 and > deallocate any memory it's allocated, if the config string is missing an end > quote. In both the cases below, which are the only calling sites, the return > isn't checked, and using the pointer causes a pretty immediate segfault. > > Signed-off-by: Deskin Miller <deskinm@umich.edu> Acked-by: Miklos Vajna <vmiklos@frugalware.org> Thanks. [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-09-24 22:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-09-24 6:10 [PATCH] maint: check return of split_cmdline to avoid bad config strings Deskin Miller 2008-09-24 9:28 ` Miklos Vajna 2008-09-24 14:50 ` [PATCH v2] " Deskin Miller 2008-09-24 22:32 ` Miklos Vajna
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).