http-push: trim trailing newline from remote symref

When we fetch a symbolic ref file from the remote, we get the whole string "ref: refs/heads/master\n", recognize it by skipping past the "ref: ", and store the rest. We should chomp the trailing newline.

This bug was introduced in ae021d8 (use skip_prefix to avoid magic numbers, 2014-06-18), which did not notice that the length computation fed to xmemdupz was quietly tweaked by 1 to account for this.

We can solve it by explicitly trimming the newline, which is more obvious. Note that we use strbuf_rtrim here, which will actually cut off any trailing whitespace, not just a single newline. This is a good thing, though, as it makes our parsing more liberal (and spaces are not valid in refnames anyway).

---
This is a regression in v2.1.0.

It was causing t5540 to fail, but I realized I have been building with NO_EXPAT for a while, so I didn't notice. Frankly, I'm kind of surprised and disturbed that nobody noticed it before now. More evidence that we can kill off dumb http-push? I would have thought somebody else would have noticed the test failure, though.

I am embarrassed to have introduced the bug during a refactoring patch. But in my defense, the original code was quite subtle and horrible, and I think the end result at least is much obvious (and is a good point in favor of skip_prefix's existence!). The original came from eecc836 (Another memory overrun in http-push.c, 2007-03-01). Looking at that patch, I can't understand how the code before it ever worked in the first place.

 http-push.c | 3 +++
 1 file changed, 3 insertions(+)

index 26dfa67..184d24a 100644
@@ -1578,6 +1578,9 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 	if (buffer.len == 0)
 		return;
 
+	/* Cut off trailing newline. */
+	strbuf_rtrim(&buffer);
+
 	/* If it's a symref, set the refname; otherwise try for a sha1 */
 	if (skip_prefix(buffer.buf, "ref: ", &name)) {
 		*symref = xmemdupz(name, buffer.len - (name - buffer.buf));

2.2.1.425.g441bb3c

While looking into Kyle's earlier response, I found that there is a semi-duplicate of this function for the http-walker side:
http_fetch_ref. It already uses strbuf_rtrim, so I feel doubly good about moving to its use here.

Almost certainly this duplicated functionality could be factored out. I have very little interest in spending time cleaning up the http-push code, though.

-Peff

I have this line in my 2.1.4 test output log:

t5540-http-push-webdav.sh .......................... ok

And again in my 2.2.2 test output log:

t5540-http-push-webdav.sh .......................... ok

Running the 2.2.2 version with -v I get:

t5540-http-push-webdav.sh ..
ok 1 - setup remote repository
ok 2 - create password-protected repository
ok 3 - setup askpass helper
ok 4 - clone remote repository
ok 5 - push to remote repository with packed refs
ok 6 - push already up-to-date
ok 7 - push to remote repository with unpacked refs
ok 8 - http-push fetches unpacked objects
ok 9 - http-push fetches packed objects
ok 10 - create and delete remote branch
ok 11 - MKCOL sends directory names with trailing slashes
ok 12 - PUT and MOVE sends object to URLs with SHA-1 hash suffix
ok 13 - non-fast-forward push fails
ok 14 - non-fast-forward push show ref status
ok 15 - non-fast-forward push shows help message
not ok 16 - force with lease aka cas # TODO known breakage
ok 17 - push to password-protected repository (user in URL)
not ok 18 - user was prompted only once for password # TODO known breakage not ok 19 - push to password-protected repository (no user in URL) # TODO known breakage

# still have 3 known breakage(s)
# passed all remaining 16 test(s)
1..19
ok

I also get the same results using v2.3.0-rc0.

I do not build with NO_EXPAT. This is running the tests on OS X without this patch applied. Is something else required to get a failure?

-Kyle

Hmm. I think it is probably this:

http://curl.haxx.se/docs/adv_20150108B.html

where curl has started complaining about URLs with newlines in them. So ae021d8 did introduce a bug, but older versions of curl did not really care. The combination of ae021d8 with a new version of curl triggers the problem.

And that also explains why it worked prior to eecc8367f4; curl was more forgiving. Also, interestingly, if you "git log -S'- 6' http-push.c", you can see the exact same bug reappear and go away in 2006/2007. The implicit "chop one character" behavior is there in the original 3dfaf7bc, adding http-push support. Then it disappears as a side effect of bfbd0bb6, and then comes back again in eecc8367.

Anyway. I think my patch is still the right thing. But that does explain why we didn't notice the test failure.

-Peff

I'm running curl 7.38, and in this context "older" is anything before 7.40, so that would explain it. curl 7.38 was released 2014-09-10, so it's only 4 months old at this point. 7.40 was only released 5 days ago on 2015-01-08 which is probably why there have not been a whole lot of reports about this so far.

After updating to curl 7.40 I get:

t5540-http-push-webdav.sh (Wstat: 256 Tests: 19 Failed: 1)
Failed test: 10
Non-zero exit status: 1

And then after applying your patch I'm back to:

t5540-http-push-webdav.sh .. ok

So definitely a needed patch.

-Kyle

I see a Ubuntu box I have nearby has this:

curl (7.35.0-1ubuntu2.3) trusty-security; urgency=medium

* SECURITY UPDATE: URL request injection
- debian/patches/CVE-2014-8150.patch: drop bad chars from URL in lib/url.c, added test to tests/data/Makefile.am, tests/data/test1529, tests/libtest/Makefile.inc, tests/libtest/lib1529.c.
- CVE-2014-8150

-- Marc Deslauriers <[email protected]> Wed, 14 Jan 2015 08:49:32 -0500

That explains why I started seeing the same on a box with 7.35.x which looks older than 7.40.