[v2] fix: release transaction before releasing handle

Message ID 20211120200320.542060-1-kevr@0cost.org
State Under Review
Headers show
Series [v2] fix: release transaction before releasing handle | expand

Commit Message

Kevin Morris Nov. 20, 2021, 8:03 p.m. UTC
If a transaction exists, release it if possible. If
`alpm_trans_release` errors out (returns a non-zero),
return a -1 in `alpm_release`.

This patch addresses the following issue observed in `pyalpm`:
https://gitlab.archlinux.org/archlinux/pyalpm/-/issues/25, where
`pyalpm` ends up raising twice due to an unexpected failure during
handle release, while a transaction is locked.

This is a modified version of
https://patchwork.archlinux.org/project/pacman/patch/20200119191833.581492-1-morganamilo@archlinux.org/
and is a derivation of his (Morganamilo's) work.

Signed-off-by: Kevin Morris <kevr@0cost.org>
---
 lib/libalpm/alpm.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

morganamilo Nov. 21, 2021, 10:05 p.m. UTC | #1
On 20/11/2021 20:03, Kevin Morris via pacman-dev wrote:
> If a transaction exists, release it if possible. If
> `alpm_trans_release` errors out (returns a non-zero),
> return a -1 in `alpm_release`.
> 
> This patch addresses the following issue observed in `pyalpm`:
> https://gitlab.archlinux.org/archlinux/pyalpm/-/issues/25, where
> `pyalpm` ends up raising twice due to an unexpected failure during
> handle release, while a transaction is locked.
> 
> This is a modified version of
> https://patchwork.archlinux.org/project/pacman/patch/20200119191833.581492-1-morganamilo@archlinux.org/
> and is a derivation of his (Morganamilo's) work.
> 
> Signed-off-by: Kevin Morris <kevr@0cost.org>
> ---
>  lib/libalpm/alpm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
> index 6708e202..47d02fea 100644
> --- a/lib/libalpm/alpm.c
> +++ b/lib/libalpm/alpm.c
> @@ -100,6 +100,10 @@ int SYMEXPORT alpm_release(alpm_handle_t *myhandle)
>  
>  	CHECK_HANDLE(myhandle, return -1);
>  
> +	if (myhandle->trans != NULL && alpm_trans_release(myhandle)) {
> +		return -1;
> +	}
> +
>  	/* close local database */
>  	db = myhandle->db_local;
>  	if(db) {
> 

So looking through the code, alpm_release calls many functions that may
fail but only actually looks at the return value of
alpm_unregister_all_syncdbs.

alpm_unregister_all_syncdbs can only fail if there's a transaction
active, which this patch prevents.

We should probably error check all the function calls.

Patch

diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
index 6708e202..47d02fea 100644
--- a/lib/libalpm/alpm.c
+++ b/lib/libalpm/alpm.c
@@ -100,6 +100,10 @@  int SYMEXPORT alpm_release(alpm_handle_t *myhandle)
 
 	CHECK_HANDLE(myhandle, return -1);
 
+	if (myhandle->trans != NULL && alpm_trans_release(myhandle)) {
+		return -1;
+	}
+
 	/* close local database */
 	db = myhandle->db_local;
 	if(db) {