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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 9, 2007, 18:15 UTC
Message-ID: <7vy7c3pwek.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1197219900-19334-2-git-send-email-mh@glandium.org>

Mike Hommey <mh@glandium.org> writes:

Show 9 quoted lines

> Also, replace whitespaces with tabs in some places
>
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>
>  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.

Show 30 quoted lines

> @@ -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.