{"thread":{"id":"15645","subject":"[PATCH] maint: check return of split_cmdline to avoid bad config strings","startedAt":"2008-09-24T06:10:28Z","lastAt":"2008-09-24T22:32:52Z","messageCount":4,"participants":["Deskin Miller","Miklos Vajna"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"91447","messageId":"20080924061028.GA2792@riemann.deskinm.fdns.net","threadId":"15645","inReplyTo":null,"subject":"[PATCH] maint: check return of split_cmdline to avoid bad config strings","fromName":"Deskin Miller","fromEmail":"deskinm@umich.edu","sentAt":"2008-09-24T06:10:28Z","receivedAt":"2008-09-24T06:10:28Z","isPatch":true,"sender":{"key":"deskinm@umich.edu","avatar":"https://gravatar.com/avatar/d340a0e612cdf0a79535c71863c0b4c535e9aba63b42032226ae903e638b64f9?d=mp&s=160"},"body":">From 7679d395856d17d7853eea0fc196435eab9be08f Mon Sep 17 00:00:00 2001\nFrom: Deskin Miller \nDate: Mon, 22 Sep 2008 11:06:41 -0400\nSubject: [PATCH] maint: check return of split_cmdline to avoid bad config strings\n\nSigned-off-by: Deskin Miller \n---\nAs the testcase demonstrates, it's possible to have split_cmdline return -1 and\ndeallocate any memory it's allocated, if the config string is missing an end\nquote. In both the cases below, the return isn't checked, causing a pretty\nimmediate segfault.\n builtin-merge.c | 2 ++\n git.c | 2 ++\n t/t1300-repo-config.sh | 10 ++++++++++\n 3 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex b280444..dcaf368 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -442,6 +442,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \n \t\tbuf = xstrdup(v);\n \t\targc = split_cmdline(buf, &argv);\n+\t\tif (argc < 0)\n+\t\t\tdie(\"Bad branch.%s.mergeoptions string\", branch);\n \t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n \t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n \t\targc++;\ndiff --git a/git.c b/git.c\nindex fdb0f71..5582c51 100644\n--- a/git.c\n+++ b/git.c\n@@ -162,6 +162,8 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\t alias_string + 1, alias_command);\n \t\t}\n \t\tcount = split_cmdline(alias_string, &new_argv);\n+\t\tif (count < 0)\n+\t\t\tdie(\"Bad alias.%s string\", alias_command);\n \t\toption_count = handle_options(&new_argv, &count, &envchanged);\n \t\tif (envchanged)\n \t\t\tdie(\"alias '%s' changes environment variables\\n\"\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 64567fb..3794d23 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -741,4 +741,14 @@ test_expect_success 'symlinked configuration' '\n \n '\n \n+test_expect_success 'check split_cmdline return' \"\n+\tgit config alias.split-cmdline-fix 'echo \\\"' &&\n+\tgit split-cmdline-fix || test \\$? = 128 &&\n+\techo foo > foo &&\n+\tgit add foo &&\n+\tgit commit -m 'initial commit' &&\n+\tgit config branch.master.mergeoptions 'echo \\\"' &&\n+\tgit merge master || test \\$? = 128\n+\t\"\n+\n test_done\n-- \n1.6.0.2.GIT\n"},{"id":"91454","messageId":"20080924092847.GD23137@genesis.frugalware.org","threadId":"15645","inReplyTo":"20080924061028.GA2792@riemann.deskinm.fdns.net","subject":"Re: [PATCH] maint: check return of split_cmdline to avoid bad config strings","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-24T09:28:47Z","receivedAt":"2008-09-24T09:28:47Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Wed, Sep 24, 2008 at 02:10:28AM -0400, Deskin Miller wrote:\n> As the testcase demonstrates, it's possible to have split_cmdline return -1 and\n> deallocate any memory it's allocated, if the config string is missing an end\n> quote. In both the cases below, the return isn't checked, causing a pretty\n> immediate segfault.\n\nI think this could go to the commit message.\n\n> diff --git a/builtin-merge.c b/builtin-merge.c\n> index b280444..dcaf368 100644\n> --- a/builtin-merge.c\n> +++ b/builtin-merge.c\n> @@ -442,6 +442,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n> \n> \t\tbuf = xstrdup(v);\n> \t\targc = split_cmdline(buf, &argv);\n> +\t\tif (argc < 0)\n> +\t\t\tdie(\"Bad branch.%s.mergeoptions string\", branch);\n> \t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n> \t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n> \t\targc++;\n\nACK.\n\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 64567fb..3794d23 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -741,4 +741,14 @@ test_expect_success 'symlinked configuration' '\n> \n> '\n> \n> +test_expect_success 'check split_cmdline return' \"\n> +\tgit config alias.split-cmdline-fix 'echo \\\"' &&\n> +\tgit split-cmdline-fix || test \\$? = 128 &&\n> +\techo foo > foo &&\n> +\tgit add foo &&\n> +\tgit commit -m 'initial commit' &&\n> +\tgit config branch.master.mergeoptions 'echo \\\"' &&\n> +\tgit merge master || test \\$? = 128\n> +\t\"\n\nWhy don't you use test_must_fail here?\n"},{"id":"91471","messageId":"20080924145029.GA3052@riemann.deskinm.fdns.net","threadId":"15645","inReplyTo":"20080924092847.GD23137@genesis.frugalware.org","subject":"Re: [PATCH v2] maint: check return of split_cmdline to avoid bad config strings","fromName":"Deskin Miller","fromEmail":"deskinm@umich.edu","sentAt":"2008-09-24T14:50:29Z","receivedAt":"2008-09-24T14:50:29Z","isPatch":true,"sender":{"key":"deskinm@umich.edu","avatar":"https://gravatar.com/avatar/d340a0e612cdf0a79535c71863c0b4c535e9aba63b42032226ae903e638b64f9?d=mp&s=160"},"body":">From 0d52afa86d757220fefa2440c78c81bd2c8b790a Mon Sep 17 00:00:00 2001\nFrom: Deskin Miller \nDate: Mon, 22 Sep 2008 11:06:41 -0400\nSubject: [PATCH] maint: check return of split_cmdline to avoid bad config strings\n\nAs the testcase demonstrates, it's possible for split_cmdline to return -1 and\ndeallocate any memory it's allocated, if the config string is missing an end\nquote. In both the cases below, which are the only calling sites, the return\nisn't checked, and using the pointer causes a pretty immediate segfault.\n\nSigned-off-by: Deskin Miller \n---\nOn Wed, Sep 24, 2008 at 11:28:47AM +0200, Miklos Vajna wrote:\n> On Wed, Sep 24, 2008 at 02:10:28AM -0400, Deskin Miller wrote:\n> > As the testcase demonstrates, it's possible to have split_cmdline return -1 and\n> > deallocate any memory it's allocated, if the config string is missing an end\n> > quote. In both the cases below, the return isn't checked, causing a pretty\n> > immediate segfault.\n> \n> I think this could go to the commit message.\n\nDone in v2.\n\n> > +\tgit merge master || test \\$? = 128\n> > +\t\"\n> \n> Why don't you use test_must_fail here?\n\nDidn't know about it. Fixed in v2.\n\nThanks for the advice.\n\n builtin-merge.c | 2 ++\n git.c | 2 ++\n t/t1300-repo-config.sh | 10 ++++++++++\n 3 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex b280444..dcaf368 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -442,6 +442,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \n \t\tbuf = xstrdup(v);\n \t\targc = split_cmdline(buf, &argv);\n+\t\tif (argc < 0)\n+\t\t\tdie(\"Bad branch.%s.mergeoptions string\", branch);\n \t\targv = xrealloc(argv, sizeof(*argv) * (argc + 2));\n \t\tmemmove(argv + 1, argv, sizeof(*argv) * (argc + 1));\n \t\targc++;\ndiff --git a/git.c b/git.c\nindex fdb0f71..5582c51 100644\n--- a/git.c\n+++ b/git.c\n@@ -162,6 +162,8 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\t alias_string + 1, alias_command);\n \t\t}\n \t\tcount = split_cmdline(alias_string, &new_argv);\n+\t\tif (count < 0)\n+\t\t\tdie(\"Bad alias.%s string\", alias_command);\n \t\toption_count = handle_options(&new_argv, &count, &envchanged);\n \t\tif (envchanged)\n \t\t\tdie(\"alias '%s' changes environment variables\\n\"\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 64567fb..11b82f4 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -741,4 +741,14 @@ test_expect_success 'symlinked configuration' '\n \n '\n \n+test_expect_success 'check split_cmdline return' \"\n+\tgit config alias.split-cmdline-fix 'echo \\\"' &&\n+\ttest_must_fail git split-cmdline-fix &&\n+\techo foo > foo &&\n+\tgit add foo &&\n+\tgit commit -m 'initial commit' &&\n+\tgit config branch.master.mergeoptions 'echo \\\"' &&\n+\ttest_must_fail git merge master\n+\t\"\n+\n test_done\n-- \n1.6.0.2.GIT\n"},{"id":"91535","messageId":"20080924223252.GL23137@genesis.frugalware.org","threadId":"15645","inReplyTo":"20080924145029.GA3052@riemann.deskinm.fdns.net","subject":"Re: [PATCH v2] maint: check return of split_cmdline to avoid bad config strings","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-24T22:32:52Z","receivedAt":"2008-09-24T22:32:52Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Wed, Sep 24, 2008 at 10:50:29AM -0400, Deskin Miller wrote:\n> As the testcase demonstrates, it's possible for split_cmdline to return -1 and\n> deallocate any memory it's allocated, if the config string is missing an end\n> quote. In both the cases below, which are the only calling sites, the return\n> isn't checked, and using the pointer causes a pretty immediate segfault.\n> \n> Signed-off-by: Deskin Miller \n\nAcked-by: Miklos Vajna \n\nThanks.\n"}]}