{"thread":{"id":"32107","subject":"[PATCH] config: don't segfault when given --path with a missing value","startedAt":"2012-11-14T04:50:04Z","lastAt":"2012-11-15T18:15:28Z","messageCount":6,"participants":["Carlos Martín Nieto","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"203170","messageId":"1352868604-20459-1-git-send-email-cmn@elego.de","threadId":"32107","inReplyTo":null,"subject":"[PATCH] config: don't segfault when given --path with a missing value","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-11-14T04:50:04Z","receivedAt":"2012-11-14T04:50:04Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"When given a variable without a value, such as '[section] var' and\nasking git-config to treat it as a path, git_config_pathname returns\nan error and doesn't modify its output parameter. show_config assumes\nthat the call is always successful and sets a variable to indicate\nthat vptr should be freed. In case of an error however, trying to do\nthis will cause the program to be killed, as it's pointing to memory\nin the stack.\n\nSet the must_free_vptr flag depending on the return value of\ngit_config_pathname so it's accurate.\n---\n builtin/config.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 442ccc2..60220d5 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -129,8 +129,7 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \t\telse\n \t\t\tsprintf(value, \"%d\", v);\n \t} else if (types == TYPE_PATH) {\n-\t\tgit_config_pathname(&vptr, key_, value_);\n-\t\tmust_free_vptr = 1;\n+\t\tmust_free_vptr = !git_config_pathname(&vptr, key_, value_);\n \t} else if (value_) {\n \t\tvptr = value_;\n \t} else {\n-- \n1.8.0.316.g291341c\n"},{"id":"203290","messageId":"20121115160847.GA6157@sigill.intra.peff.net","threadId":"32107","inReplyTo":"1352868604-20459-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] config: don't segfault when given --path with a missing value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T16:08:49Z","receivedAt":"2012-11-15T16:08:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 08:50:04PM -0800, Carlos Martín Nieto wrote:\n\n> When given a variable without a value, such as '[section] var' and\n> asking git-config to treat it as a path, git_config_pathname returns\n> an error and doesn't modify its output parameter. show_config assumes\n> that the call is always successful and sets a variable to indicate\n> that vptr should be freed. In case of an error however, trying to do\n> this will cause the program to be killed, as it's pointing to memory\n> in the stack.\n\nWhoops.\n\n> Set the must_free_vptr flag depending on the return value of\n> git_config_pathname so it's accurate.\n\nThat is definitely the right thing to do. But do we also need to take\nnote of the error for later? After this code:\n\n> \t} else if (types == TYPE_PATH) {\n> -\t\tgit_config_pathname(&vptr, key_, value_);\n> -\t\tmust_free_vptr = 1;\n> +\t\tmust_free_vptr = !git_config_pathname(&vptr, key_, value_);\n\nWe don't have any clue that nothing got written into vptr. Which means\nit still points at the stack buffer \"value\", which contains\nuninitialized bytes. We will later try to print it, thinking it has the\nexpanded path in it.\n\nDo we need something like:\n\n if (!git_config_pathname(&vptr, key_, value_))\n must_free_vptr = 1;\n else\n vptr = \"\";\n\n?\n\n-Peff\n"},{"id":"203291","messageId":"20121115161149.GB6157@sigill.intra.peff.net","threadId":"32107","inReplyTo":"20121115160847.GA6157@sigill.intra.peff.net","subject":"Re: [PATCH] config: don't segfault when given --path with a missing value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T16:11:50Z","receivedAt":"2012-11-15T16:11:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 08:08:49AM -0800, Jeff King wrote:\n\n> That is definitely the right thing to do. But do we also need to take\n> note of the error for later? After this code:\n> \n> > \t} else if (types == TYPE_PATH) {\n> > -\t\tgit_config_pathname(&vptr, key_, value_);\n> > -\t\tmust_free_vptr = 1;\n> > +\t\tmust_free_vptr = !git_config_pathname(&vptr, key_, value_);\n> \n> We don't have any clue that nothing got written into vptr. Which means\n> it still points at the stack buffer \"value\", which contains\n> uninitialized bytes. We will later try to print it, thinking it has the\n> expanded path in it.\n> \n> Do we need something like:\n> \n> if (!git_config_pathname(&vptr, key_, value_))\n> must_free_vptr = 1;\n> else\n> vptr = \"\";\n\nHmm, actually, we should probably propagate the error (I was thinking\nfor some reason this was in the listing code, but it is really about\ngetting a specific variable, and that variable does not have a sane\nformat. We'll already have printed the non-bool error, so we should\nprobably die. So more like:\n\n if (git_config_pathname(&vptr, key_, value_) < 0)\n return -1;\n must_free_vptr = 1;\n\n-Peff\n"},{"id":"203292","messageId":"20121115161758.GC6157@sigill.intra.peff.net","threadId":"32107","inReplyTo":"20121115161149.GB6157@sigill.intra.peff.net","subject":"Re: [PATCH] config: don't segfault when given --path with a missing value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T16:18:01Z","receivedAt":"2012-11-15T16:18:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 08:11:50AM -0800, Jeff King wrote:\n\n> Hmm, actually, we should probably propagate the error (I was thinking\n> for some reason this was in the listing code, but it is really about\n> getting a specific variable, and that variable does not have a sane\n> format. We'll already have printed the non-bool error, so we should\n> probably die. So more like:\n> \n> if (git_config_pathname(&vptr, key_, value_) < 0)\n> return -1;\n> must_free_vptr = 1;\n\nYou may want to squash in this test, which triggers your original\nproblem, but also demonstrates the use of uninitialized memory (although\nyou need to run under valgrind or similar to reliably notice it).\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex e127f35..7c4c372 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -803,6 +803,11 @@ test_expect_success NOT_MINGW 'get --path copes with unset $HOME' '\n \ttest_cmp expect result\n '\n \n+test_expect_success 'get --path barfs on boolean variable' '\n+\techo \"[path]bool\" >.git/config &&\n+\ttest_must_fail git config --get --path path.bool\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n\n-Peff\n"},{"id":"203305","messageId":"1353003001-22600-1-git-send-email-cmn@elego.de","threadId":"32107","inReplyTo":"20121115161758.GC6157@sigill.intra.peff.net","subject":"[PATCH] config: don't segfault when given --path with a missing value","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-11-15T18:10:01Z","receivedAt":"2012-11-15T18:10:01Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"When given a variable without a value, such as '[section] var' and\nasking git-config to treat it as a path, git_config_pathname returns\nan error and doesn't modify its output parameter. show_config assumes\nthat the call is always successful and sets a variable to indicate\nthat vptr should be freed. In case of an error however, trying to do\nthis will cause the program to be killed, as it's pointing to memory\nin the stack.\n\nDetect the error and return immediately to avoid freeing or accessing\nthe uninitialed memory in the stack.\n\nSigned-off-by: Carlos Martín Nieto \n\n---\n\nOn Thu, Nov 15, 2012 at 08:11:50AM -0800, Jeff King wrote:\n\n> Hmm, actually, we should probably propagate the error (I was thinking\n> for some reason this was in the listing code, but it is really about\n> getting a specific variable, and that variable does not have a sane\n> format. We'll already have printed the non-bool error, so we should\n> probably die. So more like:\n> \n> if (git_config_pathname(&vptr, key_, value_) < 0)\n> return -1;\n> must_free_vptr = 1;\n\nYeah, that's more sensible. I didn't notice that the buffer never gets\nwritten to in this codepath, and the trying to print it out is silly\nwhen we know that there is nothing valid to print. Thanks for the\nreview. I've included your test as well, which really makes all of\nthis your code. Do we have some equivalent of a Basically-writen-by\nline?\n\n builtin/config.c | 3 ++-\n t/t1300-repo-config.sh | 5 +++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 442ccc2..4dc5ffa 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -129,7 +129,8 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \t\telse\n \t\t\tsprintf(value, \"%d\", v);\n \t} else if (types == TYPE_PATH) {\n-\t\tgit_config_pathname(&vptr, key_, value_);\n+\t\tif (git_config_pathname(&vptr, key_, value_) < 0)\n+\t\t\treturn -1;\n \t\tmust_free_vptr = 1;\n \t} else if (value_) {\n \t\tvptr = value_;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex a477453..17272e0 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -803,6 +803,11 @@ test_expect_success NOT_MINGW 'get --path copes with unset $HOME' '\n \ttest_cmp expect result\n '\n \n+test_expect_success 'get --path barfs on boolean variable' '\n+\techo \"[path]bool\" >.git/config &&\n+\ttest_must_fail git config --get --path path.bool\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n1.8.0.316.g291341c\n"},{"id":"203306","messageId":"20121115181527.GA22506@sigill.intra.peff.net","threadId":"32107","inReplyTo":"1353003001-22600-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] config: don't segfault when given --path with a missing value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T18:15:28Z","receivedAt":"2012-11-15T18:15:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 10:10:01AM -0800, Carlos Martín Nieto wrote:\n\n> When given a variable without a value, such as '[section] var' and\n> asking git-config to treat it as a path, git_config_pathname returns\n> an error and doesn't modify its output parameter. show_config assumes\n> that the call is always successful and sets a variable to indicate\n> that vptr should be freed. In case of an error however, trying to do\n> this will cause the program to be killed, as it's pointing to memory\n> in the stack.\n> \n> Detect the error and return immediately to avoid freeing or accessing\n> the uninitialed memory in the stack.\n> \n> Signed-off-by: Carlos Martín Nieto \n\nAcked-by: Jeff King \n\n> Yeah, that's more sensible. I didn't notice that the buffer never gets\n> written to in this codepath, and the trying to print it out is silly\n> when we know that there is nothing valid to print.\n\n> Thanks for the review. I've included your test as well, which really\n> makes all of this your code.\n\nEh, I guess so. You did the hard part of finding it, though. ;)\n\n> Do we have some equivalent of a Basically-writen-by line?\n\nNothing structured. But I am comfortable enough with the number of times\nI am mentioned in \"git log\" already, so don't worry about it.\n\n-Peff\n"}]}