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 <[email protected]>
> ---
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
