Re: [pacman-dev] [PATCH 1/2] Attempted to free lock on failure in alpm_db_update()
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",
Re: [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
Re: [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
signature.asc
Description: OpenPGP digital signature
[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
[pacman-dev] [PATCH 1/2] Attempted to free lock on failure in alpm_db_update()
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
