diff mbox series

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

Message ID 20191216150500.213552-2-allan@archlinux.org
State Changes Requested
Headers show
Series [pacman-dev,1/2] Attempted to free lock on failure in alpm_db_update() | expand

Commit Message

Allan McRae Dec. 16, 2019, 3:05 p.m. UTC 
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@archlinux.org>
---

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

I'd appreciate more eyes on these changes.

 lib/libalpm/be_sync.c | 58 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 14 deletions(-)

Comments

Eli Schwartz Dec. 16, 2019, 3:21 p.m. UTC | #1 
On 12/16/19 10:05 AM, 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@archlinux.org>
> ---
> 
> That is a lot of teduim for adding ".bak" to the end of a string...
> 
> I'd appreciate more eyes on these changes.

Cool!

Would it help to download the db to a temporary file, and rename it on
success?

>  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;
>  }
>  
> +
>  /** 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);
> +
>  	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);
> +
>  		/* 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);
>
Allan McRae Dec. 16, 2019, 11:30 p.m. UTC | #2 
On 17/12/19 1:21 am, Eli Schwartz wrote:
> On 12/16/19 10:05 AM, 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@archlinux.org>
>> ---
>>
>> That is a lot of teduim for adding ".bak" to the end of a string...
>>
>> I'd appreciate more eyes on these changes.
> 
> Cool!
> 
> Would it help to download the db to a temporary file, and rename it on
> success?
> 

I looked at that approach quite some time ago.  The issue is things like
alpm_db_validate take a db pointer and construct paths using that.  It
could be worked around by creating a temporary sync directory and
setting that in the handle, but I thought this way was easier/cleaner.

Allan
Andrew Gregory Dec. 19, 2019, 3:23 a.m. UTC | #3 
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@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
diff mbox series

Patch

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;
 }
 
+
 /** 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);
+
 	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);
+
 		/* 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);