[pacman-dev] libalpm: free trans before databases

Message ID 20200119191833.581492-1-morganamilo@archlinux.org
State Changes Requested
Headers show
Series
  • [pacman-dev] libalpm: free trans before databases
Related show

Commit Message

morganamilo Jan. 19, 2020, 7:18 p.m. UTC
When releasing the handle, alpm tries to do some self clean up by
freeing the databases and transaction.

However, databases refuse to unregister is there is an in progress
transaction. Causing them to leak if the handle is released while
a transaction is active.

Comments

Allan McRae Jan. 31, 2020, 3:28 a.m. UTC | #1
On 20/1/20 5:18 am, morganamilo wrote:
> When releasing the handle, alpm tries to do some self clean up by
> freeing the databases and transaction.
> 
> However, databases refuse to unregister is there is an in progress
> transaction. Causing them to leak if the handle is released while
> a transaction is active.
> 

So...  I've been sitting on this for a while.   It does not seem the
right answer to the issue to me.  But I am not sure what the right
answer is!

Suggestions inline below.

> diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
> index 1a378db9..02b8fc9b 100644
> --- a/lib/libalpm/alpm.c
> +++ b/lib/libalpm/alpm.c
> @@ -30,6 +30,7 @@
>  #include "alpm_list.h"
>  #include "handle.h"
>  #include "log.h"
> +#include "trans.h"
>  #include "util.h"
>  
>  /** \addtogroup alpm_interface Interface Functions
> @@ -114,6 +115,8 @@ int SYMEXPORT alpm_release(alpm_handle_t *myhandle)
>  
>  	CHECK_HANDLE(myhandle, return -1);
>  
> +	_alpm_trans_free(myhandle->trans);
> +

Could we do an alpm_trans_release() here if handle->trans is non-null
and abort if that fails?

>  	/* close local database */
>  	db = myhandle->db_local;
>  	if(db) {
> diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
> index e3193f40..1e42f4a8 100644
> --- a/lib/libalpm/handle.c
> +++ b/lib/libalpm/handle.c
> @@ -34,7 +34,6 @@
>  #include "alpm_list.h"
>  #include "util.h"
>  #include "log.h"
> -#include "trans.h"
>  #include "alpm.h"
>  #include "deps.h"
>  
> @@ -74,7 +73,6 @@ void _alpm_handle_free(alpm_handle_t *handle)
>  #endif
>  
>  	/* free memory */
> -	_alpm_trans_free(handle->trans);

This needs to stay.  It is unexpected that _alpm_handle_free does not
free its whole memory.

>  	FREE(handle->root);
>  	FREE(handle->dbpath);
>  	FREE(handle->dbext);
>

Patch

diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
index 1a378db9..02b8fc9b 100644
--- a/lib/libalpm/alpm.c
+++ b/lib/libalpm/alpm.c
@@ -30,6 +30,7 @@ 
 #include "alpm_list.h"
 #include "handle.h"
 #include "log.h"
+#include "trans.h"
 #include "util.h"
 
 /** \addtogroup alpm_interface Interface Functions
@@ -114,6 +115,8 @@  int SYMEXPORT alpm_release(alpm_handle_t *myhandle)
 
 	CHECK_HANDLE(myhandle, return -1);
 
+	_alpm_trans_free(myhandle->trans);
+
 	/* close local database */
 	db = myhandle->db_local;
 	if(db) {
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
index e3193f40..1e42f4a8 100644
--- a/lib/libalpm/handle.c
+++ b/lib/libalpm/handle.c
@@ -34,7 +34,6 @@ 
 #include "alpm_list.h"
 #include "util.h"
 #include "log.h"
-#include "trans.h"
 #include "alpm.h"
 #include "deps.h"
 
@@ -74,7 +73,6 @@  void _alpm_handle_free(alpm_handle_t *handle)
 #endif
 
 	/* free memory */
-	_alpm_trans_free(handle->trans);
 	FREE(handle->root);
 	FREE(handle->dbpath);
 	FREE(handle->dbext);