[MPlayer-dev-eng] [PATCH 2/7] Use Proxy-Authorization instead of Authorization for proxy auth

[Reimar Döffinger]

On Tue, Nov 16, 2010 at 09:53:06PM +0100, Clément Bœsch wrote:
> On Tue, Nov 16, 2010 at 09:05:24PM +0100, Reimar Döffinger wrote:
> > On Fri, Nov 12, 2010 at 10:40:57AM +0100, Clément Bœsch wrote:
> > > @@ -637,13 +637,13 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co
> > >  
> > >  	b64_usr_pass[out_len]='\0';
> > >  
> > > -	auth = malloc(encoded_len+22);
> > > +	auth = malloc(encoded_len + strlen(auth_str) + sizeof(": Basic "));
> > >  	if( auth==NULL ) {
> > >  		mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed);
> > >  		goto out;
> > >  	}
> > >  
> > > -	sprintf( auth, "Authorization: Basic %s", b64_usr_pass);
> > > +	sprintf( auth, "%s: Basic %s", auth_str, b64_usr_pass);
> > 
> > I think their ok, though I think personally I'd feel slightly
> > more comfortable if it was something like
> > > buffer_len = encoded_len + 100; // arbitrary, large enough number
> > > malloc(buffer_len);
> > > snprintf(, buffer_len, ....);
> > 
> > While hard-coded is not that great, it avoids wasting CPU
> > time on strlen, and also the duplicated ": Basic " string
> > is a risk, and this way at least ensure there'll never be
> > a buffer overflow no matter what (not having to thing about
> > integer overflows, someone changing only one of those strings,
> > ...).
> 
> The CPU time on strlen in this case is imo totally inappropriate, however
> I agree with you arguments about the duplicated string, so what about the
> attached patch?

Well, while it's very unlikely to be a _new_ issue with this patch,
to review it properly I'd still have to check that the addition
will not cause an integer overflow.
And the CPU (and honestly more code complexity) is something I just consider
because, honestly, I can't see much of a point in using a "100 % correct"
number.