Re: [PATCH 3/3] remote-curl: sanity check ref advertisement from server

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 18, 2013, 01:41 UTC
Message-ID: <20130218014113.GC3221@elie.Belkin>
In-Reply-To: <20130217192830.GB25096@sigill.intra.peff.net>

Jeff King wrote:

> On Sun, Feb 17, 2013 at 03:05:34AM -0800, Jonathan Nieder wrote:
>> Jeff King wrote:

Show 10 quoted lines

>>> +		if (verify_ref_advertisement(last->buf, last->len) < 0)
>>> +			die("ref advertisement is invalid at %s", refs_url);
>>
>> Won't this error out with
>>
>> 	protocol error: bad line length character: ERR
>>
>> instead of the current more helpful behavior for ERR lines?
>
> I don't think so. Don't ERR lines appear inside their own packets?

Yes, I misread get_remote_heads for some reason.  Thanks for checking.

[...]

Show 6 quoted lines

> The one thing we do also check, though, is that we end with a flush
> packet. So depending on what servers produce, it may mean we trigger
> this complaint instead of passing the ERR along to fetch-pack.
>
> Rather than doing this fake syntactic verification, I wonder if we
> should simply call get_remote_heads, which does a more thorough check

I'm not sure whether servers are expected to send a flush after an ERR packet. The only codepath I know of in git itself that sends such packets is git-daemon, which does not flush after the error (but is not used in the stateless-rpc case). http-backend uses HTTP error codes for its errors.

If I am reading get_remote_heads correctly, calling it with the following tweak should work ok. The extra thread is just to feed a string into a fd-based interface and could be avoided for "list", too, if it costs too much.

diff --git i/connect.c w/connect.c
index 49e56ba3..55ee7de7 100644
--- i/connect.c
+++ w/connect.c
@@ -68,7 +68,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 {
 	int got_at_least_one_head = 0;
 
-	*list = NULL;
+	if (list)
+		*list = NULL;
 	for (;;) {
 		struct ref *ref;
 		unsigned char old_sha1[20];
@@ -92,6 +93,9 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			die("protocol error: expected sha/ref, got '%s'", buffer);
 		name = buffer + 41;
 
+		if (!list)
+			continue;
+
 		name_len = strlen(name);
 		if (len != name_len + 41) {
 			free(server_capabilities);