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