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

[Mike Hommey <mh@glandium.org>]

On Sun, Dec 09, 2007 at 10:15:15AM -0800, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
> 
> > 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.
> 
> > @@ -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