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

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