[MPlayer-dev-eng] [PATCH] add -referrer option to allow HTTP referrer to be set

[chocolateboy]

On 30/05/10 16:22, Reimar Döffinger wrote:
> On Sun, May 30, 2010 at 03:54:01PM +0100, chocolateboy wrote:
>    
>> Thanks for the swift and helpful feedback. I've attached the
>> suggested patch (using strlen/malloc).
>>      
> Before you get annoyed tell me, I can of course clean it up myself,
> but I always assume submitters are interested in hearing comments
> and fixing on their own.
>
>    
>> @@ -226,6 +227,27 @@
>>   	else
>>   	    http_set_field( http_hdr, "User-Agent: MPlayer/"VERSION);
>>
>> +	if (network_referrer)
>> +	{
>> +		size_t len;
>> +		char *referrer = NULL;
>> +
>> +		len = sizeof("Referer: ") + strlen(network_referrer) + 1;
>>      
> Sizeof will result in one more. Also it won't really gain you anything over just
> writing 9 unless you e.g. use a define to make sure it is always the same thing
> as in the printf.
>
>    
>> +		if (len>  0) { /* check for overflow */
>> +			referrer = malloc(len);
>> +		}
>> +		if (referrer == NULL) {
>> +			mp_msg(MSGT_NETWORK, MSGL_FATAL, MSGTR_MemAllocFailed);
>> +			goto err_out;
>> +		} else {
>> +			snprintf(referrer, len, "Referer: %s", network_referrer);
>> +			http_set_field(http_hdr, referrer);
>> +			free(referrer);
>> +		}
>>      
> I'd simplify it to something like
> referrer = malloc(len);
> // Check len to assure we don't do something really bad in case of an overflow
> if (len>  10&&  referrer) {
>      snprintf(referrer, len, "Referer: %s", network_referrer);
>      http_set_field(http_hdr, referrer);
> } else {
>      mp_msg(MSGT_NETWORK, MSGL_FATAL, MSGTR_MemAllocFailed);
> }
> free(referrer);
>
> In particular, completely erroring for the error-case IMO is overdoing
> it.
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
>
>
>    

 > I always assume submitters are interested in hearing comments and 
fixing on their own.

Absolutely!

 > In particular, completely erroring for the error-case IMO is 
overdoing it.

Ah, fair enough. I was trying to follow the "house style" on that, but I 
didn't do an exhaustive search, and probably erred on the side of 
paranoid hysteria :-)

 > I'd simplify it to something like

Thanks. I've implemented the bulk of that in the attached patch. I've 
kept a few minor bits of paranoia in there to hopefully guard against 
being called names down the line :-), but I'm quite happy to simplify it 
further, and, obviously, quite happy for you to clean it up for me :-)

chocolateboy
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mplayer_referrer.patch.txt
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100530/681caa31/attachment.txt>