Message ID | 20191216150500.213552-1-allan@archlinux.org |
---|---|
State | Changes Requested |
Headers | show |
Series | [pacman-dev,1/2] Attempted to free lock on failure in alpm_db_update() | expand |
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 <allan@archlinux.org> > --- > 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",
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 <allan@archlinux.org> >> --- >> 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", > . >
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 <allan@archlinux.org> > >> --- > >> 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.
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",
Also fixes a memory leak under an error condition. Signed-off-by: Allan McRae <allan@archlinux.org> --- lib/libalpm/be_sync.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)