@matheusbsbs / matheus-bagliano / commits / 2a4552021a

remote-curl: always parse incoming refs

When remote-curl receives a list of refs from a server, it keeps the whole buffer intact. When we get a "list" command, we feed the result to get_remote_heads, and when we get a "fetch" or "push" command, we feed it to fetch-pack or send-pack, respectively. If the HTTP response from the server is truncated for any reason, we will get an incomplete ref advertisement. If we then feed this incomplete list to fetch-pack, one of a few things may happen: 1. If the truncation is in a packet header, fetch-pack will notice the bogus line and complain. 2. If the truncation is inside a packet, fetch-pack will keep waiting for us to send the rest of the packet, which we never will. 3. If the truncation is at a packet boundary, fetch-pack will keep waiting for us to send the next packet, which we never will. As a result, fetch-pack hangs, waiting for input. However, remote-curl believes it has sent all of the advertisement, and therefore waits for fetch-pack to speak. The two processes end up in a deadlock. We do notice the broken ref list if we feed it to get_remote_heads. So if git asks the helper to do a "list" followed by a "fetch", we are safe; we'll abort during the list operation, which parses the refs. This patch teaches remote-curl to always parse and save the incoming ref list when we read the ref advertisement from a server. That means that we will always verify and abort before even running fetch-pack (or send-pack) when reading a corrupted list, even if we do not run the "list" command explicitly. Since we save the result, in the common case of running "list" then "fetch", we do not do any extra parsing at all. In the case of just a "fetch", we do an extra round of parsing, but only once. Note also that the "fetch" case will now also initialize server_capabilities from the remote (in remote-curl; we already would do so inside fetch-pack). Doing "list+fetch" already does this. It doesn't actually matter now, but the new behavior is arguably more correct, should remote-curl ever start caring about the server's capability list. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Feb 20, 2013 at 15:07 UTC 2a4552021a92be17c7c4d2d2313df9913e8eb4bf

1 file changed +13 -9

remote-curl.c

+13 -9

@@ -76,6 +76,7 @@ struct discovery {

76 char *buf_alloc;

77 char *buf;

78 size_t len;

79 + struct ref *refs;

80 unsigned proto_git : 1;

81 };

82 static struct discovery *last_discovery;

@@ -145,11 +146,12 @@ static void free_discovery(struct discovery *d)

146 if (d == last_discovery)

147 last_discovery = NULL;

148 free(d->buf_alloc);

149 + free_refs(d->refs);

150 free(d);

151 }

152 }

153

152 -static struct discovery* discover_refs(const char *service)

154 +static struct discovery* discover_refs(const char *service, int for_push)

155 {

156 struct strbuf exp = STRBUF_INIT;

157 struct strbuf type = STRBUF_INIT;

@@ -221,6 +223,11 @@ static struct discovery* discover_refs(const char *service)

223 last->proto_git = 1;

224 }

225

226 + if (last->proto_git)

227 + last->refs = parse_git_refs(last, for_push);

228 + else

229 + last->refs = parse_info_refs(last);

230 +

231 free(refs_url);

232 strbuf_release(&exp);

233 strbuf_release(&type);

@@ -234,13 +241,11 @@ static struct ref *get_refs(int for_push)

241 struct discovery *heads;

242

243 if (for_push)

237 - heads = discover_refs("git-receive-pack");

244 + heads = discover_refs("git-receive-pack", for_push);

245 else

239 - heads = discover_refs("git-upload-pack");

246 + heads = discover_refs("git-upload-pack", for_push);

247

241 - if (heads->proto_git)

242 - return parse_git_refs(heads, for_push);

243 - return parse_info_refs(heads);

248 + return heads->refs;

249 }

250

251 static void output_refs(struct ref *refs)

@@ -254,7 +259,6 @@ static void output_refs(struct ref *refs)

259 }

260 printf("\n");

261 fflush(stdout);

257 - free_refs(refs);

262 }

263

264 struct rpc_state {

@@ -670,7 +674,7 @@ static int fetch_git(struct discovery *heads,

674

675 static int fetch(int nr_heads, struct ref **to_fetch)

676 {

673 - struct discovery *d = discover_refs("git-upload-pack");

677 + struct discovery *d = discover_refs("git-upload-pack", 0);

678 if (d->proto_git)

679 return fetch_git(d, nr_heads, to_fetch);

680 else

@@ -789,7 +793,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)

793

794 static int push(int nr_spec, char **specs)

795 {

792 - struct discovery *heads = discover_refs("git-receive-pack");

796 + struct discovery *heads = discover_refs("git-receive-pack", 1);

797 int ret;

798

799 if (heads->proto_git)