[pacman-dev] [PATCH] Add check for NULL data parameter in sync commit
From: Allan McRae <allan.mcrae@qimr.edu.au> To: Discussion list for pacman development <pacman-dev@archlinux.org> Subject: [pacman-dev] [PATCH] Add check for NULL data parameter in sync commit Date: Sat, 22 Dec 2007 14:06:44 +1000 Reply-To: Discussion list for pacman development <pacman-dev@archlinux.org> User-Agent: Thunderbird 2.0.0.9 (X11/20071213)
From ccb76f0d909f3532af8e4004a125d21f9012ae78 Mon Sep 17 00:00:00 2001 From: Allan McRae <mcrae_allan@hotmail.com> Date: Sat, 22 Dec 2007 14:01:48 +1000 Subject: [PATCH] Add check for NULL data parameter in sync commit
Fixes FS#7380: alpm crashes on passing NULL to alpm_trans_commit in a sync operation. Adds sanity check in _alpm_trans_commit for case PM_TRANS_TYPE_SYNC.
Signed-off-by: Allan McRae <mcrae_allan@hotmail.com> --- lib/libalpm/trans.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 582c76b..31b2747 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -415,6 +415,8 @@ int _alpm_trans_commit(pmtrans_t *trans, alpm_list_t **data) } break; case PM_TRANS_TYPE_SYNC: + /* Sanity checks */ + ASSERT(data != NULL, RET_ERR(PM_ERR_WRONG_ARGS, -1)); if(_alpm_sync_commit(trans, handle->db_local, data) == -1) { /* pm_errno is set by _alpm_sync_commit() */ return(-1);
Indeed, this bug still exists. But I interpret your patch as a workaround (or hotfix), right? I mean we should fix alpm_sync_commit to work with NULL **data (which means we don't need additional error info) Bye
Nagy Gabor wrote:
Indeed, this bug still exists. But I interpret your patch as a workaround (or hotfix), right? I mean we should fix alpm_sync_commit to work with NULL **data (which means we don't need additional error info)
Bye Yeah, it was really a hotfix. I hadn't thought about people wanting to ignore the error messages. Here is what alpm_trans_commit calls:
alpm_trans_commit -> _alpm_trans_commit -> _alpm_sync_commit -> test_delta_md5sum & test_pkg_md5sum -> test_md5sum -> _alpm_trans_prepare -> _alpm_sync_prepare -> _alpm_resolvedeps The only places I can find writing to data are without a if(data) block are: _alpm_trans_prepare _alpm_sync_prepare test_md5sum The attached patch fixes those and thus should fixed the problem at its source.
Yeah, it was really a hotfix. I hadn't thought about people wanting to ignore the error messages. Here is what alpm_trans_commit calls:
alpm_trans_commit -> _alpm_trans_commit -> _alpm_sync_commit -> test_delta_md5sum & test_pkg_md5sum -> test_md5sum -> _alpm_trans_prepare -> _alpm_sync_prepare -> _alpm_resolvedeps
The only places I can find writing to data are without a if(data) block are: _alpm_trans_prepare _alpm_sync_prepare test_md5sum
The attached patch fixes those and thus should fixed the problem at its source.
Great work! The patch has just eliminated the segfault. Thanks. Bye
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 7661866..0a8170c 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -675,7 +675,9 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync if(deps) { pm_errno = PM_ERR_UNSATISFIED_DEPS; ret = -1; - *data = deps; + if(data) { + *data = deps; + } goto cleanup; } } Some little comments: If you don't keep the result, you should free deps list with FREELIST().
And the same for the allocated errormsg strings <- here you can put the whole {calloc, snprintf, alpm_list_add} block to if(data). Bye
Nagy Gabor wrote:
Some little comments: If you don't keep the result, you should free deps list with FREELIST().
And the same for the allocated errormsg strings <- here you can put the whole {calloc, snprintf, alpm_list_add} block to if(data).
Bye
Updated patch attached that fixes the above issues. Allan
participants (3)
-
Allan McRae
-
Allan McRae
-
Nagy Gabor