[pacman-dev] [PATCH 1/2] alpm: check for invalid sync db before replacing original

[Andrew Gregory]

On 09/07/16 at 07:22pm, ivy.foster at gmail.com wrote:
> From: Ivy Foster <ivy.foster at gmail.com>
> 
> Closes #46107
> 
> Signed-off-by: Ivy Foster <ivy.foster at gmail.com>
> ---
>  lib/libalpm/alpm.h    |  1 +
>  lib/libalpm/be_sync.c | 26 +++++++++++++++++++++++++-
>  lib/libalpm/error.c   |  2 ++
>  3 files changed, 28 insertions(+), 1 deletion(-)

This is a step in the right direction, but the problem of downloading
an invalid db over a valid one still exists.  The errors given in the
bug report are not actually from the invalid db, they're from invalid
signature files.  If we download a junk db but no signature file, the
valid db would still be overwritten by the invalid one.

> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 168d71b..0dd68a7 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -75,6 +75,7 @@ typedef enum _alpm_errno_t {
>  	ALPM_ERR_DB_VERSION,
>  	ALPM_ERR_DB_WRITE,
>  	ALPM_ERR_DB_REMOVE,
> +	ALPM_ERR_DB_BACKUP,
>  	/* Servers */
>  	ALPM_ERR_SERVER_BAD_URL,
>  	ALPM_ERR_SERVER_NONE,
> diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> index 32a669d..ee438f8 100644
> --- a/lib/libalpm/be_sync.c
> +++ b/lib/libalpm/be_sync.c
> @@ -182,6 +182,9 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>  	mode_t oldmask;
>  	alpm_handle_t *handle;
>  	alpm_siglevel_t level;
> +	char *newdb;
> +	char *olddb;
> +	size_t len;
>  
>  	/* Sanity checks */
>  	ASSERT(db != NULL, return -1);
> @@ -218,10 +221,23 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>  
>  	dbext = db->handle->dbext;
>  
> +	len = strlen(syncpath) + strlen(db->treename) + strlen(dbext) + 2;
> +	/* TODO fix leak syncpath and umask unset */
> +	MALLOC(newdb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> +	snprintf(newdb, len, "%s/%s%s", syncpath, db->treename, dbext);
> +	len += 4;
> +	/* TODO fix leak syncpath and umask unset */
> +	MALLOC(olddb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> +	snprintf(olddb, len, "%s.bak", newdb);

This runs the risk of conflicting with another db if dbext is "" or
".bak", for example, if I have repos core and core.bak, syncing core
would clobber the core.bak db.

> +	if (rename(newdb, olddb) == -1) {
> +		ret = -1;
> +		handle->pm_errno = ALPM_ERR_DB_BACKUP;
> +		goto cleanup;
> +	}

This is backwards.  Instead of moving the good db out of the way and
hoping we can move it back if something goes wrong, we should download
the new database to a temp file then move it into place if it's good.

>  	for(i = db->servers; i; i = i->next) {
>  		const char *server = i->data, *final_db_url = NULL;
>  		struct dload_payload payload;
> -		size_t len;
>  		int sig_ret = 0;
>  
>  		memset(&payload, 0, sizeof(struct dload_payload));
> @@ -315,15 +331,23 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>  		}
>  	}
>  
> +cleanup:
>  	if(ret == -1) {
>  		/* pm_errno was set by the download code */
>  		_alpm_log(handle, ALPM_LOG_DEBUG, "failed to sync db: %s\n",
>  				alpm_strerror(handle->pm_errno));
> +		if (handle->pm_errno != ALPM_ERR_DB_BACKUP && rename(olddb, newdb) == -1) {
> +			_alpm_log(handle, ALPM_LOG_DEBUG, "failed to replace original db: %s\n",
> +					alpm_strerror(ALPM_ERR_DB_BACKUP));
> +		}
>  	} else {
> +		unlink(olddb);
>  		handle->pm_errno = 0;
>  	}
>  
>  	_alpm_handle_unlock(handle);
> +	free(newdb);
> +	free(olddb);
>  	free(syncpath);
>  	umask(oldmask);
>  	return ret;
> diff --git a/lib/libalpm/error.c b/lib/libalpm/error.c
> index 2d6d071..e707d43 100644
> --- a/lib/libalpm/error.c
> +++ b/lib/libalpm/error.c
> @@ -78,6 +78,8 @@ const char SYMEXPORT *alpm_strerror(alpm_errno_t err)
>  			return _("could not update database");
>  		case ALPM_ERR_DB_REMOVE:
>  			return _("could not remove database entry");
> +		case ALPM_ERR_DB_BACKUP:
> +			return _("could not back up old database");
>  		/* Servers */
>  		case ALPM_ERR_SERVER_BAD_URL:
>  			return _("invalid url for server");
> -- 
> 2.9.3