[pacman-dev] [PATCH] Skip rename() on NULL destfile in curl_download_internal()

[Lukas Fleischer]

On Wed, Aug 17, 2011 at 11:45:35AM -0400, Dave Reisner wrote:
> On Wed, Aug 17, 2011 at 11:34:19AM -0400, Dave Reisner wrote:
> > On Wed, Aug 17, 2011 at 04:45:35PM +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>
> > > ---
> > > Untested, sorry. Should be trivial enough not to break anything though.
> > > Hopefully.
> > > 
> > >  lib/libalpm/dload.c |   14 +++++++++-----
> > >  1 files changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> > > index 731d807..5cbb6b4 100644
> > > --- a/lib/libalpm/dload.c
> > > +++ b/lib/libalpm/dload.c
> > > @@ -384,12 +384,16 @@ cleanup:
> > >  	}
> > >  
> > >  	if(ret == 0) {
> > > -		if(rename(tempfile, destfile)) {
> > > -			_alpm_log(handle, ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"),
> > > -					tempfile, destfile, strerror(errno));
> > > -			ret = -1;
> > > +		if (destfile) {
> > > +			if(rename(tempfile, destfile)) {
> > > +				_alpm_log(handle, ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"),
> > > +						tempfile, destfile, strerror(errno));
> > > +				ret = -1;
> > > +			} else if(final_file) {
> > > +				*final_file = strdup(strrchr(destfile, '/') + 1);
> > > +			}
> > >  		} else if(final_file) {
> > > -			*final_file = strdup(strrchr(destfile, '/') + 1);
> > > +			*final_file = strdup(strrchr(tempfile, '/') + 1);
> > 
> > If you're going to change this line, can you use the STRDUP macro like I
> > should have?
> > 
> >   STRDUP(final_file, tempfile, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> > 
> > Same for the added strdup inside the if(destfile) block. Looks fine
> > otherwise.
> > 
> > >  		}
> > >  	}
> 
> Note that this doesn't actually solve the issue of a zero length
> filename...
> 
>  ┌┤noclaf at rampage:1 bugfix ~/src/c/pacman
>  └╼ sudo ./src/pacman/pacman -U http://www.archlinux.org/
>  alpmtmp.sj9IKg           23.2K  443.7K/s 00:00:00 [-----------------------] 100%
> error: could not rename /mnt/Haven/packages/alpmtmp.sj9IKg to /mnt/Haven/packages/ (Not a directory)
> warning: failed to download http://www.archlinux.org/
> error: 'http://www.archlinux.org/': unexpected error

Well, this is right and I would say we shouldn't even try to fix this
here. There might be other file system dependent limitations to file
names. If you want to do this (kind of) properly, there should be
another patch that checks for the length of the file name, as well as a
set of allowed characters (intersection of what the common file systems
accept) and replace everything else.

The intention of this patch is to fix a potential segfault by ensuring
that we never rename() and/or strrchr() a NULL pointer.