[pacman-dev] [PATCH 2/2] Bail out on NULL destfile in curl_download_internal()

[Dave Reisner]

On Wed, Aug 17, 2011 at 10:15:17AM +0200, Lukas Fleischer wrote:
> Avoid a potential segfault that may occur if we use a temporary file and
> fail to build the destination file name from the effective URL.
> 
> Signed-off-by: Lukas Fleischer <archlinux at cryptocrack.de>
> ---
> Another approach is to use a random default destfile name. Since the URL
> parsing is very unlikely to fail, I took the easy option here.
> 
>  lib/libalpm/dload.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 731d807..c552d2b 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -373,6 +373,11 @@ static int curl_download_internal(struct dload_payload *payload,
>  				destfile = get_fullpath(localpath, effective_filename, "");
>  			}
>  		}
> +		else {
> +			_alpm_log(handle, ALPM_LOG_ERROR, _("could not parse file name from url (%s)"),
> +					effective_url);
> +			goto cleanup;
> +		}
>  	}
>  
>  	ret = 0;
> -- 
> 1.7.6
> 
> 

This is after the file has already been downloaded. We shouldn't bail
out so easily since we already have the file. It looks like in this
case, we wouldn't even unlink after jumping to cleanup.

I'm not sure that this is the right thing to do. Worst case scenario, we
should probably just skip the rename.

d