Re: [PATCH 2/4] Use strbuf in http code

On Sun, Dec 09, 2007 at 10:15:15AM -0800, Junio C Hamano wrote:

Mike Hommey [off-list ref] writes:

quoted

Also, replace whitespaces with tabs in some places

Signed-off-by: Mike Hommey <redacted>
---

 While testing this, I noticed 3 things:
 - CURL_MULTI makes the code very racy
 - a lot of the code doesn't do anything useful without CURL_MULTI
 - the code is redundant

Yeah, there does seem to be a lot of duplicated code that does common
setup with slightly different request string.

quoted

@@ -1115,16 +1109,11 @@ static char *quote_ref_url(const char *base, const char *ref)
 
 int fetch_ref(char *ref, unsigned char *sha1)
 {
-        char *url;
-        char hex[42];
-        struct buffer buffer;
+	char *url;
+	struct strbuf buffer = STRBUF_INIT;
 	char *base = remote->url;
 	struct active_request_slot *slot;
 	struct slot_results results;
-        buffer.size = 41;
-        buffer.posn = 0;
-        buffer.buffer = hex;
-        hex[41] = '\0';
 
 	url = quote_ref_url(base, ref);
 	slot = get_active_slot();

@@ -1142,9 +1131,9 @@ int fetch_ref(char *ref, unsigned char *sha1)
 		return error("Unable to start request");
 	}
 
-        hex[40] = '\0';
-        get_sha1_hex(hex, sha1);
-        return 0;
+	buffer.buf[40] = '\0';
+	get_sha1_hex(buffer.buf, sha1);
+	return 0;
 }

A conversion like this is worrysome and needs to be rethought I think.

At least with the old code, we knew hex[40] was a safe location to make
assignment to, even though we did not check if what it contained made
sense --- the other end might have had a garbage in that URL (but the
caller hopefully would be responsible for noticing that).  But with your
change, I do not think you have that guarantee.  fwrite_buffer() may
have extended the buffer using strbuf API, but it may have received less
than what you are expecting, in which case you may not have buf[40]
touchable for you, no?

I at the same time think the original code is buggy.  It initializes
buffer.buffer to the on-stack storage hex[], but lets fwrite_buffer() to
call xrealloc() on it.

Both codes are also buggy in case the ref is a symbolic ref, and that
happens. I got bitten by this while testing.

Considering the assumption being made that refs are all properly filled
with sha1s, both codes are mostly equally bad.

Fixing the issue would obviously be the subject for another patch.

Mike