{"thread":{"id":"38343","subject":"[PATCH] http-push: trim trailing newline from remote symref","startedAt":"2015-01-13T02:28:58Z","lastAt":"2015-01-16T01:19:24Z","messageCount":6,"participants":["Jeff King","Kyle J. McKay","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"254590","messageId":"20150113022857.GA4087@peff.net","threadId":"38343","inReplyTo":null,"subject":"[PATCH] http-push: trim trailing newline from remote symref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T02:28:58Z","receivedAt":"2015-01-13T02:28:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we fetch a symbolic ref file from the remote, we get\nthe whole string \"ref: refs/heads/master\\n\", recognize it by\nskipping past the \"ref: \", and store the rest. We should\nchomp the trailing newline.\n\nThis bug was introduced in ae021d8 (use skip_prefix to avoid\nmagic numbers, 2014-06-18), which did not notice that the\nlength computation fed to xmemdupz was quietly tweaked by 1\nto account for this.\n\nWe can solve it by explicitly trimming the newline, which is\nmore obvious. Note that we use strbuf_rtrim here, which will\nactually cut off any trailing whitespace, not just a single\nnewline. This is a good thing, though, as it makes our\nparsing more liberal (and spaces are not valid in refnames\nanyway).\n\nSigned-off-by: Jeff King \n---\nThis is a regression in v2.1.0.\n\nIt was causing t5540 to fail, but I realized I have been building with\nNO_EXPAT for a while, so I didn't notice. Frankly, I'm kind of surprised\nand disturbed that nobody noticed it before now. More evidence that we\ncan kill off dumb http-push? I would have thought somebody else would\nhave noticed the test failure, though.\n\nI am embarrassed to have introduced the bug during a refactoring patch.\nBut in my defense, the original code was quite subtle and horrible, and\nI think the end result at least is much obvious (and is a good point in\nfavor of skip_prefix's existence!). The original came from eecc836\n(Another memory overrun in http-push.c, 2007-03-01). Looking at that\npatch, I can't understand how the code before it ever worked in the\nfirst place.\n\n http-push.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/http-push.c b/http-push.c\nindex 26dfa67..184d24a 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1578,6 +1578,9 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)\n \tif (buffer.len == 0)\n \t\treturn;\n \n+\t/* Cut off trailing newline. */\n+\tstrbuf_rtrim(&buffer);\n+\n \t/* If it's a symref, set the refname; otherwise try for a sha1 */\n \tif (skip_prefix(buffer.buf, \"ref: \", &name)) {\n \t\t*symref = xmemdupz(name, buffer.len - (name - buffer.buf));\n-- \n2.2.1.425.g441bb3c\n"},{"id":"254605","messageId":"4E1001F7-2D26-483A-9339-E2570559ECF0@gmail.com","threadId":"38343","inReplyTo":"20150113022857.GA4087@peff.net","subject":"Re: [PATCH] http-push: trim trailing newline from remote symref","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-01-13T16:26:31Z","receivedAt":"2015-01-13T16:26:31Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jan 12, 2015, at 18:28, Jeff King wrote:\n\n> When we fetch a symbolic ref file from the remote, we get\n> the whole string \"ref: refs/heads/master\\n\", recognize it by\n> skipping past the \"ref: \", and store the rest. We should\n> chomp the trailing newline.\n> [..]\n> This is a regression in v2.1.0.\n>\n> It was causing t5540 to fail, but I realized I have been building with\n> NO_EXPAT for a while, so I didn't notice. Frankly, I'm kind of \n> surprised\n> and disturbed that nobody noticed it before now. More evidence that we\n> can kill off dumb http-push? I would have thought somebody else would\n> have noticed the test failure, though.\n\nI have this line in my 2.1.4 test output log:\n\nt5540-http-push-webdav.sh .......................... ok\n\nAnd again in my 2.2.2 test output log:\n\nt5540-http-push-webdav.sh .......................... ok\n\nRunning the 2.2.2 version with -v I get:\n\nt5540-http-push-webdav.sh ..\nok 1 - setup remote repository\nok 2 - create password-protected repository\nok 3 - setup askpass helper\nok 4 - clone remote repository\nok 5 - push to remote repository with packed refs\nok 6 - push already up-to-date\nok 7 - push to remote repository with unpacked refs\nok 8 - http-push fetches unpacked objects\nok 9 - http-push fetches packed objects\nok 10 - create and delete remote branch\nok 11 - MKCOL sends directory names with trailing slashes\nok 12 - PUT and MOVE sends object to URLs with SHA-1 hash suffix\nok 13 - non-fast-forward push fails\nok 14 - non-fast-forward push show ref status\nok 15 - non-fast-forward push shows help message\nnot ok 16 - force with lease aka cas # TODO known breakage\nok 17 - push to password-protected repository (user in URL)\nnot ok 18 - user was prompted only once for password # TODO known \nbreakage\nnot ok 19 - push to password-protected repository (no user in URL) # \nTODO known breakage\n# still have 3 known breakage(s)\n# passed all remaining 16 test(s)\n1..19\nok\n\nI also get the same results using v2.3.0-rc0.\n\nI do not build with NO_EXPAT. This is running the tests on OS X \nwithout this patch applied. Is something else required to get a \nfailure?\n\n-Kyle\n"},{"id":"254619","messageId":"20150113195806.GA13282@peff.net","threadId":"38343","inReplyTo":"4E1001F7-2D26-483A-9339-E2570559ECF0@gmail.com","subject":"Re: [PATCH] http-push: trim trailing newline from remote symref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T19:58:06Z","receivedAt":"2015-01-13T19:58:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 13, 2015 at 08:26:31AM -0800, Kyle J. McKay wrote:\n\n> I have this line in my 2.1.4 test output log:\n> \n> t5540-http-push-webdav.sh .......................... ok\n> [...]\n> I do not build with NO_EXPAT. This is running the tests on OS X without\n> this patch applied. Is something else required to get a failure?\n\nHmm. I think it is probably this:\n\n http://curl.haxx.se/docs/adv_20150108B.html\n\nwhere curl has started complaining about URLs with newlines in them. So\nae021d8 did introduce a bug, but older versions of curl did not really\ncare. The combination of ae021d8 with a new version of curl triggers the\nproblem.\n\nAnd that also explains why it worked prior to eecc8367f4; curl was more\nforgiving. Also, interestingly, if you \"git log -S'- 6' http-push.c\",\nyou can see the exact same bug reappear and go away in 2006/2007. The\nimplicit \"chop one character\" behavior is there in the original\n3dfaf7bc, adding http-push support. Then it disappears as a side effect\nof bfbd0bb6, and then comes back again in eecc8367.\n\nAnyway. I think my patch is still the right thing. But that does explain\nwhy we didn't notice the test failure.\n\n-Peff\n"},{"id":"254621","messageId":"20150113204140.GA16238@peff.net","threadId":"38343","inReplyTo":"20150113022857.GA4087@peff.net","subject":"Re: [PATCH] http-push: trim trailing newline from remote symref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-13T20:41:40Z","receivedAt":"2015-01-13T20:41:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 12, 2015 at 09:28:58PM -0500, Jeff King wrote:\n\n> When we fetch a symbolic ref file from the remote, we get\n> the whole string \"ref: refs/heads/master\\n\", recognize it by\n> skipping past the \"ref: \", and store the rest. We should\n> chomp the trailing newline.\n> \n> This bug was introduced in ae021d8 (use skip_prefix to avoid\n> magic numbers, 2014-06-18), which did not notice that the\n> length computation fed to xmemdupz was quietly tweaked by 1\n> to account for this.\n> \n> We can solve it by explicitly trimming the newline, which is\n> more obvious. Note that we use strbuf_rtrim here, which will\n> actually cut off any trailing whitespace, not just a single\n> newline. This is a good thing, though, as it makes our\n> parsing more liberal (and spaces are not valid in refnames\n> anyway).\n\nWhile looking into Kyle's earlier response, I found that there is a\nsemi-duplicate of this function for the http-walker side:\nhttp_fetch_ref. It already uses strbuf_rtrim, so I feel doubly good\nabout moving to its use here.\n\nAlmost certainly this duplicated functionality could be factored out. I\nhave very little interest in spending time cleaning up the http-push\ncode, though.\n\n-Peff\n"},{"id":"254633","messageId":"05091340-887B-4743-AB62-39E609E22CEE@gmail.com","threadId":"38343","inReplyTo":"20150113195806.GA13282@peff.net","subject":"Re: [PATCH] http-push: trim trailing newline from remote symref","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-01-14T00:21:01Z","receivedAt":"2015-01-14T00:21:01Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jan 13, 2015, at 11:58, Jeff King wrote:\n> On Tue, Jan 13, 2015 at 08:26:31AM -0800, Kyle J. McKay wrote:\n>\n>> I have this line in my 2.1.4 test output log:\n>>\n>> t5540-http-push-webdav.sh .......................... ok\n>> [...]\n>> I do not build with NO_EXPAT. This is running the tests on OS X \n>> without\n>> this patch applied. Is something else required to get a failure?\n>\n> Hmm. I think it is probably this:\n>\n> http://curl.haxx.se/docs/adv_20150108B.html\n>\n> where curl has started complaining about URLs with newlines in them. \n> So\n> ae021d8 did introduce a bug, but older versions of curl did not really\n> care. The combination of ae021d8 with a new version of curl triggers \n> the\n> problem.\n\nI'm running curl 7.38, and in this context \"older\" is anything before \n7.40, so that would explain it. curl 7.38 was released 2014-09-10, so \nit's only 4 months old at this point. 7.40 was only released 5 days \nago on 2015-01-08 which is probably why there have not been a whole \nlot of reports about this so far.\n\n> And that also explains why it worked prior to eecc8367f4; curl was \n> more\n> forgiving. Also, interestingly, if you \"git log -S'- 6' http-push.c\",\n> you can see the exact same bug reappear and go away in 2006/2007. The\n> implicit \"chop one character\" behavior is there in the original\n> 3dfaf7bc, adding http-push support. Then it disappears as a side \n> effect\n> of bfbd0bb6, and then comes back again in eecc8367.\n\nAfter updating to curl 7.40 I get:\n\nt5540-http-push-webdav.sh (Wstat: 256 Tests: 19 Failed: 1)\n Failed test: 10\n Non-zero exit status: 1\n\n> Anyway. I think my patch is still the right thing. But that does \n> explain\n> why we didn't notice the test failure.\n\nAnd then after applying your patch I'm back to:\n\nt5540-http-push-webdav.sh .. ok\n\nSo definitely a needed patch.\n\nTested-by: Kyle J. McKay \n\n-Kyle\n"},{"id":"254800","messageId":"xmqqd26f4j4z.fsf@gitster.dls.corp.google.com","threadId":"38343","inReplyTo":"05091340-887B-4743-AB62-39E609E22CEE@gmail.com","subject":"Re: [PATCH] http-push: trim trailing newline from remote symref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-16T01:19:24Z","receivedAt":"2015-01-16T01:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> On Jan 13, 2015, at 11:58, Jeff King wrote:\n> ...\n> I'm running curl 7.38, and in this context \"older\" is anything before\n> 7.40, so that would explain it. curl 7.38 was released 2014-09-10, so\n> it's only 4 months old at this point. 7.40 was only released 5 days\n> ago on 2015-01-08 which is probably why there have not been a whole\n> lot of reports about this so far.\n>\n> After updating to curl 7.40 I get:\n>\n> t5540-http-push-webdav.sh (Wstat: 256 Tests: 19 Failed: 1)\n> Failed test: 10\n> Non-zero exit status: 1\n>\n>> Anyway. I think my patch is still the right thing. But that does\n>> explain\n>> why we didn't notice the test failure.\n>\n> And then after applying your patch I'm back to:\n>\n> t5540-http-push-webdav.sh .. ok\n\nI see a Ubuntu box I have nearby has this:\n\ncurl (7.35.0-1ubuntu2.3) trusty-security; urgency=medium\n\n * SECURITY UPDATE: URL request injection\n - debian/patches/CVE-2014-8150.patch: drop bad chars from URL in\n lib/url.c, added test to tests/data/Makefile.am, tests/data/test1529,\n tests/libtest/Makefile.inc, tests/libtest/lib1529.c.\n - CVE-2014-8150\n\n -- Marc Deslauriers Wed, 14 Jan 2015 08:49:32 -0500\n\nThat explains why I started seeing the same on a box with 7.35.x\nwhich looks older than 7.40.\n"}]}