[pacman-dev] [PATCH 1/2] Attempted to free lock on failure in alpm_db_update()

newer
[pacman-dev] [PATCH] Ensure regex...

older
[pacman-dev] Adding an expiry time...

Allan McRae

16 Dec 2019 16 Dec '19

3:04 p.m.

Also fixes a memory leak under an error condition. Signed-off-by: Allan McRae --- lib/libalpm/be_sync.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 041b2266..5502d92d 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -215,7 +215,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) RET_ERR(handle, ALPM_ERR_HANDLE_LOCK, -1); } - dbext = db->handle->dbext; + dbext = handle->dbext; for(i = db->servers; i; i = i->next) { const char *server = i->data, *final_db_url = NULL; @@ -232,9 +232,9 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) len = strlen(server) + strlen(db->treename) + strlen(dbext) + 2; MALLOC(payload.fileurl, len, { - free(syncpath); - umask(oldmask); - RET_ERR(handle, ALPM_ERR_MEMORY, -1); + handle->pm_errno = ALPM_ERR_MEMORY; + ret = -1; + goto cleanup; } ); snprintf(payload.fileurl, len, "%s/%s%s", server, db->treename, dbext); @@ -250,8 +250,9 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) /* 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; - break; + goto cleanup; } unlink(sigpath); free(sigpath); @@ -277,9 +278,9 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) MALLOC(payload.fileurl, len, { - free(syncpath); - umask(oldmask); - RET_ERR(handle, ALPM_ERR_MEMORY, -1); + handle->pm_errno = ALPM_ERR_MEMORY; + ret = -1; + goto cleanup; } ); @@ -324,6 +325,7 @@ 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", -- 2.24.1

Show replies by date

Allan McRae

16 Dec 16 Dec

3:05 p.m.

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

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 --- 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(-) 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); -- 2.24.1

Eli Schwartz

3:21 p.m.

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

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 ---

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);

-- Eli Schwartz Bug Wrangler and Trusted User

Allan McRae

11:30 p.m.

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

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 ---

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

19 Dec 19 Dec

3:23 a.m.

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

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 ---

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

Andrew Gregory

17 Dec 17 Dec

7:13 a.m.

s/Attempted to//? On 12/17/19 at 01:04am, Allan McRae wrote:

...

Also fixes a memory leak under an error condition.

Signed-off-by: Allan McRae --- lib/libalpm/be_sync.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 041b2266..5502d92d 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c ... @@ -324,6 +325,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) } }

+cleanup:

Shouldn't this be before the previous if(updated){...} block?

...

if(ret == -1) { /* pm_errno was set by the download code */ _alpm_log(handle, ALPM_LOG_DEBUG, "failed to sync db: %s\n",

Allan McRae

19 Dec 19 Dec

1:17 a.m.

On 17/12/19 5:13 pm, Andrew Gregory wrote:

...

s/Attempted to//?

On 12/17/19 at 01:04am, Allan McRae wrote:

...
Also fixes a memory leak under an error condition.

Signed-off-by: Allan McRae --- lib/libalpm/be_sync.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 041b2266..5502d92d 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c ... @@ -324,6 +325,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) } }

+cleanup:

Shouldn't this be before the previous if(updated){...} block?

It could be... No need to drop the cache and reset validation if we are restoring the old database, but it does not hurt.

...

...
if(ret == -1) { /* pm_errno was set by the download code */ _alpm_log(handle, ALPM_LOG_DEBUG, "failed to sync db: %s\n", .

Andrew Gregory

3:23 a.m.

On 12/19/19 at 11:17am, Allan McRae wrote:

...

On 17/12/19 5:13 pm, Andrew Gregory wrote:

...
s/Attempted to//?

On 12/17/19 at 01:04am, Allan McRae wrote:

...
Also fixes a memory leak under an error condition.

Signed-off-by: Allan McRae --- lib/libalpm/be_sync.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 041b2266..5502d92d 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c ... @@ -324,6 +325,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) } }

+cleanup:

Shouldn't this be before the previous if(updated){...} block?

It could be... No need to drop the cache and reset validation if we are restoring the old database, but it does not hurt.

That doesn't happen until patch 2 though. If they're going to be split into separate patches, this patch should be able to stand on its own. It's unlikely, but the restoration could also fail, causing a package list to be loaded from an invalid database.

1599

Age (days ago)

1602

Last active (days ago)

List overview

Download

7 comments

3 participants

participants (3)

Allan McRae
Andrew Gregory
Eli Schwartz

[pacman-dev] [PATCH 1/2] Attempted to free lock on failure in alpm_db_update()

tags

participants (3)