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

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()
Related show

Commit Message

Allan McRae Dec. 16, 2019, 3:04 p.m. UTC
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(-)

Comments

Andrew Gregory Dec. 17, 2019, 7:13 a.m. UTC | #1
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",
Allan McRae Dec. 19, 2019, 1:17 a.m. UTC | #2
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",
> .
>
Andrew Gregory Dec. 19, 2019, 3:23 a.m. UTC | #3
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.

Patch

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