[pacman-dev] [PATCH 1/1] fix db filename handling

[Dave Reisner]

On Sat, Sep 07, 2013 at 12:05:00AM -0400, Dave Reisner wrote:
> On Wed, Sep 04, 2013 at 05:35:51PM +0200, Christian Hesse wrote:
> > From: Christian Hesse <mail at eworm.de>
> > 
> > If the server redirects from ${repo}.db to ${repo}.db.tar.gz pacman gets
> > this wrong: It saves to new filename and fails when accessing
> > ${repo}.db.
> > 
> > This introduces a new field 'deny_rename' to payload. If set pacman
> > downloads to the filename specified in payload.remote_name.
> > ---
> >  lib/libalpm/be_sync.c |  2 ++
> >  lib/libalpm/dload.c   | 36 ++++++++++++++++++++----------------
> >  lib/libalpm/dload.h   |  1 +
> >  3 files changed, 23 insertions(+), 16 deletions(-)
> > 
> > diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> > index 123d953..622153d 100644
> > --- a/lib/libalpm/be_sync.c
> > +++ b/lib/libalpm/be_sync.c
> > @@ -223,6 +223,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
> >  		payload.handle = handle;
> >  		payload.force = force;
> >  		payload.unlink_on_fail = 1;
> > +		payload.deny_rename = 1;
> >  
> >  		ret = _alpm_download(&payload, syncpath, NULL, NULL);
> >  		_alpm_dload_payload_reset(&payload);
> > @@ -245,6 +246,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
> >  			snprintf(payload.fileurl, len, "%s/%s.db.sig", server, db->treename);
> >  			payload.handle = handle;
> >  			payload.force = 1;
> > +			payload.deny_rename = 1;
> >  			payload.errors_ok = (level & ALPM_SIG_DATABASE_OPTIONAL);
> >  
> >  			/* set hard upper limit of 16KiB */
> > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> > index 8537b3d..381e548 100644
> > --- a/lib/libalpm/dload.c
> > +++ b/lib/libalpm/dload.c
> > @@ -547,23 +547,27 @@ static int curl_download_internal(struct dload_payload *payload,
> >  		goto cleanup;
> >  	}
> >  
> > -	if(payload->content_disp_name) {
> > -		/* content-disposition header has a better name for our file */
> > -		free(payload->destfile_name);
> > -		payload->destfile_name = get_fullpath(localpath, payload->content_disp_name, "");
> > +	if(payload->deny_rename) {
> > +		payload->destfile_name = get_fullpath(localpath, payload->remote_name, "");
> 
> This is actually a leak. payload->destfile_name should already be set
> with this exact value earlier in the function. You only need to worry
> about the negative case where we allow renaming.
> 
> I've fixed this up on my own branch.
> 

And this is me replying to the wrong patch. Looks like v2 has the same
issue, though.

> >  	} else {
> > -		const char *effective_filename = strrchr(effective_url, '/');
> > -		if(effective_filename && strlen(effective_filename) > 2) {
> > -			effective_filename++;
> > -
> > -			/* if destfile was never set, we wrote to a tempfile. even if destfile is
> > -			 * set, we may have followed some redirects and the effective url may
> > -			 * have a better suggestion as to what to name our file. in either case,
> > -			 * refactor destfile to this newly derived name. */
> > -			if(!payload->destfile_name || strcmp(effective_filename,
> > -						strrchr(payload->destfile_name, '/') + 1) != 0) {
> > -				free(payload->destfile_name);
> > -				payload->destfile_name = get_fullpath(localpath, effective_filename, "");
> > +		if(payload->content_disp_name) {
> > +			/* content-disposition header has a better name for our file */
> > +			free(payload->destfile_name);
> > +			payload->destfile_name = get_fullpath(localpath, payload->content_disp_name, "");
> > +		} else {
> > +			const char *effective_filename = strrchr(effective_url, '/');
> > +			if(effective_filename && strlen(effective_filename) > 2) {
> > +				effective_filename++;
> > +
> > +				/* if destfile was never set, we wrote to a tempfile. even if destfile is
> > +				 * set, we may have followed some redirects and the effective url may
> > +				 * have a better suggestion as to what to name our file. in either case,
> > +				 * refactor destfile to this newly derived name. */
> > +				if(!payload->destfile_name || strcmp(effective_filename,
> > +							strrchr(payload->destfile_name, '/') + 1) != 0) {
> > +					free(payload->destfile_name);
> > +					payload->destfile_name = get_fullpath(localpath, effective_filename, "");
> > +				}
> >  			}
> >  		}
> >  	}
> > diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> > index 95bb91a..bd01d8b 100644
> > --- a/lib/libalpm/dload.h
> > +++ b/lib/libalpm/dload.h
> > @@ -38,6 +38,7 @@ struct dload_payload {
> >  	int allow_resume;
> >  	int errors_ok;
> >  	int unlink_on_fail;
> > +	int deny_rename;
> >  	alpm_list_t *servers;
> >  #ifdef HAVE_LIBCURL
> >  	CURLcode curlerr;       /* last error produced by curl */
> > -- 
> > 1.8.4
> > 
> >