[pacman-dev] [PATCH] Unsafe database unregistering
Xavier
shiningxc at gmail.com
Thu Jul 19 03:59:30 EDT 2007
On Mon, Jul 09, 2007 at 05:06:05PM +0200, Xavier wrote:
> On Sat, Jul 07, 2007 at 09:44:47PM +0200, Xavier wrote:
> > On Sat, Jul 07, 2007 at 09:27:33PM +0200, Xavier wrote:
> > > And the alpm_release function in libalpm/alpm.c :
> > >
> > > 77 while((dbs_left = alpm_list_count(handle->dbs_sync)) > 0) {
> > > 78 pmdb_t *db = (pmdb_t *)handle->dbs_sync->data;
> > > 79 _alpm_log(PM_LOG_DEBUG, _("removing DB %s, %d remaining..."), db->treename, dbs_left);
> > > 80 alpm_db_unregister(db);
> > > 81 db = NULL;
> > > 82 }
> > >
> > > There is an infinite loop there when alpm_db_unregister (db.c) fails,
> >
> > oops the _alpm_log line was cut during pasting, fixed it :)
> >
> > Anyway, I forgot to mention I found where this code was introduced :
> > http://projects.archlinux.org/git/?p=pacman.git;a=commitdiff;h=55f178c629ada663d2a8e5bbed029ec9482f00ea
> >
> > So it looks like what the previous code did was even worse :)
> > While I'm not very happy about how it was fixed,
> > I can't find a better way to fix it..
> > I still think it's not very nice to have a code like that which can loop
> > infinitely so easily.
>
> Erm, what can make alpm_db_unregister(db) fail isn't even specific to the db,
> so the same checks are done multiple times.
> Couldn't we have these checks done in alpm_release() instead, and have a
> private _alpm_db_unregister(db) that wouldn't fail, instead of a public one?
> The only thing that alpm_release do is unregistering databases anyway, so the
> public alpm_db_unregister(db) isn't even used by pacman.
>
> That would make the code cleaner and safer without losing any functionality imo.
> The only thing that is worrying me is that currently, there are symetrical
> pmdb_t *alpm_db_register() and alpm_db_unregister(pmdb_t*) functions, which
> makes sense, but the following patch removes that (from the client pov).
>
Maybe it was not ok to remove that alpm_db_unregister public function, so I
kept it, and added a warning in a comment instead.
But now I provide an additional alpm_db_unregister_all() function, so this
should fix all problems.
So this might be a better patch :
>From c31156cf14201fa7533057d75be82f330ca3dad4 Mon Sep 17 00:00:00 2001
From: Chantry Xavier <shiningxc at gmail.com>
Date: Mon, 9 Jul 2007 17:02:29 +0200
Subject: [PATCH] libalpm/db.c : add alpm_db_unregister_all.
This basically moves the code from alpm_release, which was mostly about
unregistering all databases, to a safer alpm_db_unregister_all.
This allows to avoid modifying the dbs_sync list while iterating over it,
and and also prevent alpm_release from looping infinitely when a database
can't be unregistered.
Signed-off-by: Chantry Xavier <shiningxc at gmail.com>
---
lib/libalpm/alpm.c | 16 +-----------
lib/libalpm/alpm.h | 1 +
lib/libalpm/db.c | 63 ++++++++++++++++++++++++++++++++++++++++++---------
lib/libalpm/db.h | 1 +
4 files changed, 56 insertions(+), 25 deletions(-)
diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
index e3ad211..20d4a74 100644
--- a/lib/libalpm/alpm.c
+++ b/lib/libalpm/alpm.c
@@ -62,24 +62,12 @@ int SYMEXPORT alpm_initialize(void)
*/
int SYMEXPORT alpm_release(void)
{
- int dbs_left = 0;
-
ALPM_LOG_FUNC;
ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
- /* close local database */
- if(handle->db_local) {
- alpm_db_unregister(handle->db_local);
- handle->db_local = NULL;
- }
- /* and also sync ones */
- while((dbs_left = alpm_list_count(handle->dbs_sync)) > 0) {
- pmdb_t *db = (pmdb_t *)handle->dbs_sync->data;
- _alpm_log(PM_LOG_DEBUG, "removing DB %s, %d remaining...",
- db->treename, dbs_left);
- alpm_db_unregister(db);
- db = NULL;
+ if(alpm_db_unregister_all() == -1) {
+ return(-1);
}
_alpm_handle_free(handle);
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 9e641f3..98028de 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -145,6 +145,7 @@ alpm_list_t *alpm_option_get_syncdbs();
pmdb_t *alpm_db_register(const char *treename);
int alpm_db_unregister(pmdb_t *db);
+int alpm_db_unregister_all();
const char *alpm_db_get_name(const pmdb_t *db);
const char *alpm_db_get_url(const pmdb_t *db);
diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c
index 306d0b9..aa02352 100644
--- a/lib/libalpm/db.c
+++ b/lib/libalpm/db.c
@@ -70,6 +70,35 @@ pmdb_t SYMEXPORT *alpm_db_register(const char *treename)
return(_alpm_db_register(treename));
}
+/** Unregister all package databases
+ * @return 0 on success, -1 on error (pm_errno is set accordingly)
+ */
+int SYMEXPORT alpm_db_unregister_all()
+{
+ alpm_list_t *i;
+
+ ALPM_LOG_FUNC;
+
+ /* Sanity checks */
+ ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
+ /* Do not unregister a database if a transaction is on-going */
+ ASSERT(handle->trans == NULL || handle->trans->state == STATE_INTERRUPTED,
+ RET_ERR(PM_ERR_TRANS_NOT_NULL, -1));
+
+ /* close local database */
+ _alpm_db_unregister(handle->db_local);
+ handle->db_local = NULL;
+
+ /* and also sync ones */
+ for(i = handle->dbs_sync; i; i = i->next) {
+ pmdb_t *db = i->data;
+ _alpm_db_unregister(db);
+ i->data = NULL;
+ }
+ FREELIST(handle->dbs_sync);
+ return(0);
+}
+
/** Unregister a package database
* @param db pointer to the package database to unregister
* @return 0 on success, -1 on error (pm_errno is set accordingly)
@@ -84,13 +113,17 @@ int SYMEXPORT alpm_db_unregister(pmdb_t *db)
ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
ASSERT(db != NULL, RET_ERR(PM_ERR_WRONG_ARGS, -1));
/* Do not unregister a database if a transaction is on-going */
- ASSERT(handle->trans == NULL || handle->trans->state == STATE_INTERRUPTED,
+ ASSERT(handle->trans == NULL || handle->trans->state == STATE_INTERRUPTED,
RET_ERR(PM_ERR_TRANS_NOT_NULL, -1));
if(db == handle->db_local) {
handle->db_local = NULL;
found = 1;
} else {
+ /* Warning : this function shouldn't be used to unregister all sync databases
+ * by walking through the list returned by alpm_option_get_syncdbs,
+ * because the db is removed from that list here.
+ */
void *data;
handle->dbs_sync = alpm_list_remove(handle->dbs_sync, db, _alpm_db_cmp, &data);
if(data) {
@@ -102,16 +135,7 @@ int SYMEXPORT alpm_db_unregister(pmdb_t *db)
RET_ERR(PM_ERR_DB_NOT_FOUND, -1);
}
- _alpm_log(PM_LOG_DEBUG, "unregistering database '%s'", db->treename);
-
- /* Cleanup */
- _alpm_db_free_pkgcache(db);
-
- _alpm_log(PM_LOG_DEBUG, "closing database '%s'", db->treename);
- _alpm_db_close(db);
-
- _alpm_db_free(db);
-
+ _alpm_db_unregister(db);
return(0);
}
@@ -542,6 +566,23 @@ error:
/** @} */
+/* Helper function for alpm_db_unregister{_all} */
+void _alpm_db_unregister(pmdb_t *db)
+{
+ if(db == NULL) {
+ return;
+ }
+ _alpm_log(PM_LOG_DEBUG, "unregistering database '%s'", db->treename);
+
+ /* Cleanup */
+ _alpm_db_free_pkgcache(db);
+
+ _alpm_log(PM_LOG_DEBUG, "closing database '%s'", db->treename);
+ _alpm_db_close(db);
+
+ _alpm_db_free(db);
+}
+
pmdb_t *_alpm_db_new(const char *dbpath, const char *treename)
{
pmdb_t *db;
diff --git a/lib/libalpm/db.h b/lib/libalpm/db.h
index 2597c8c..d31bf60 100644
--- a/lib/libalpm/db.h
+++ b/lib/libalpm/db.h
@@ -53,6 +53,7 @@ void _alpm_db_free(pmdb_t *db);
int _alpm_db_cmp(const void *db1, const void *db2);
alpm_list_t *_alpm_db_search(pmdb_t *db, const alpm_list_t *needles);
pmdb_t *_alpm_db_register(const char *treename);
+void _alpm_db_unregister(pmdb_t *db);
/* be.c, backend specific calls */
int _alpm_db_install(pmdb_t *db, const char *dbfile);
--
1.5.2.4
More information about the pacman-dev
mailing list