[pacman-dev] [PATCH 2/2] Backup old database before updating and restore on failure

[Andrew Gregory]

On 12/17/19 at 01:05am, Allan McRae wrote:
> If alpm_db_update() fails due to an invalid signature, then the system
> is left with an unusable repo database.  Instead, backup the currently
> working database before performing the update, and restore on error.
> 
> Note that the calls rename and unlink are not checked for errors. If these
> fail, there is nothing to be done anyway.  It also allows for less complex
> flow, as these function fail gracefully when passed NULL arguments.
> 
> Signed-off-by: Allan McRae <allan at archlinux.org>
> ---

I really don't like the backup/restore method and would much prefer to
download to a new file and rotate it in once verified, but this is at
least a step in the right direction.

> That is a lot of teduim for adding ".bak" to the end of a string...

Sounds like a great excuse to start using asprintf.

> 
> I'd appreciate more eyes on these changes.
> 
>  lib/libalpm/be_sync.c | 58 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> index 5502d92d..ae46c120 100644
> --- a/lib/libalpm/be_sync.c
> +++ b/lib/libalpm/be_sync.c
> @@ -136,6 +136,7 @@ valid:
>  	return 0;
>  }
>  
> +

Extra whitespace change.

>  /** Update a package database
>   *
>   * An update of the package database \a db will be attempted. Unless
> @@ -173,14 +174,15 @@ valid:
>   */
>  int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>  {
> -	char *syncpath;
> -	const char *dbext;
> +	char *syncpath, *dbsig = NULL, *dbfilebak = NULL, *dbsigbak = NULL;
> +	const char *dbext, *dbfile;
>  	alpm_list_t *i;
>  	int updated = 0;
>  	int ret = -1;
>  	mode_t oldmask;
>  	alpm_handle_t *handle;
>  	int siglevel;
> +	size_t len;
>  
>  	/* Sanity checks */
>  	ASSERT(db != NULL, return -1);
> @@ -217,10 +219,39 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>  
>  	dbext = handle->dbext;
>  
> +	/* create backup of current database version */
> +	dbfile = _alpm_db_path(db);
> +	len = strlen(dbfile) + 5;
> +	MALLOC(dbfilebak, len,
> +		{
> +			handle->pm_errno = ALPM_ERR_MEMORY;
> +			ret = -1;
> +			goto cleanup;
> +		}
> +	);
> +	snprintf(dbfilebak, len, "%s.bak", dbfile);
> +
> +
> +	dbsig = _alpm_sigpath(db->handle, dbfile);
> +	len = strlen(dbsig) + 5;
> +	MALLOC(dbsigbak, len,
> +		{
> +			handle->pm_errno = ALPM_ERR_MEMORY;
> +			ret = -1;
> +			goto cleanup;
> +		}
> +	);
> +	snprintf(dbsigbak, len, "%s.bak", dbsig);
> +
> +	/* remove any old backup files to prevent potential sig mismatch */
> +	unlink(dbfilebak);
> +	unlink(dbsigbak);
> +	_alpm_copyfile(dbfile, dbfilebak);
> +	_alpm_copyfile(dbsig, dbsigbak);

_alpm_copyfile doesn't currently use either O_EXCL or O_TRUNC when it
opens the destination, so if the unlink fails, this could result in
thoroughly broken backup files.

>  	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));
> @@ -247,17 +278,6 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>  		updated = (updated || ret == 0);
>  
>  		if(ret != -1 && updated && (siglevel & ALPM_SIG_DATABASE)) {
> -			/* an existing sig file is no good at this point */
> -			char *sigpath = _alpm_sigpath(handle, _alpm_db_path(db));
> -			if(!sigpath) {
> -				/* pm_errno should be set */
> -				ret = -1;
> -				goto cleanup;
> -			}
> -			unlink(sigpath);
> -			free(sigpath);
> -
> -
>  			/* check if the final URL from internal downloader looks reasonable */
>  			if(final_db_url != NULL) {
>  				if(strlen(final_db_url) < 3
> @@ -327,13 +347,23 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>  
>  cleanup:
>  	if(ret == -1) {
> +		rename(dbfilebak, dbfile);
> +		rename(dbsigbak, dbsig);

This obliterates the invalid file, making troubleshooting impossible.

We really need to be sure that either this restoration succeeds or the
validity and cache were reset.

> +
>  		/* 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));
>  	} else {
> +		unlink(dbfilebak);
> +		unlink(dbsigbak);
> +
>  		handle->pm_errno = ALPM_ERR_OK;
>  	}
>  
> +	free(dbfilebak);
> +	free(dbsig);
> +	free(dbsigbak);
> +
>  	_alpm_handle_unlock(handle);
>  	free(syncpath);
>  	umask(oldmask);
> -- 
> 2.24.1