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

[Reimar Döffinger]

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.