[pacman-dev] [PATCH] Fix several memleaks, mostly related to errors handling.
* The frontend calls alpm_trans_prepare(&data), and in case of errors, receive the missing dependencies / conflicts / etc in the data pointer. It apparently needs to free this structure totally with : alpm_list_free_inner(data, free) alpm_list_free(data) So I added alpm_list_free_inner(data, free) in pacman/{sync.c,remove.c,add,c} * in _alpm_sync_prepare, the deps and asked lists were not freed in case of errors (unresolvable conflicts). Besides the code for handling this case was duplicated. * in _alpm_remove_commit, free was used instead of alpm_list_free for newfiles. * newline fix in pacman/sync.c Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- lib/libalpm/remove.c | 2 +- lib/libalpm/sync.c | 42 ++++++++++++++++++------------------------ src/pacman/add.c | 1 + src/pacman/remove.c | 1 + src/pacman/sync.c | 3 ++- 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 592cf39..e1f19ec 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -315,7 +315,7 @@ int _alpm_remove_commit(pmtrans_t *trans, pmdb_t *db) (pkg_count - alpm_list_count(targ) + 1)); position++; } - free(newfiles); + alpm_list_free(newfiles); } /* set progress to 100% after we finish unlinking files */ diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index f03a78b..b49bbfb 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -496,12 +496,13 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync if(deps) { int errorout = 0; alpm_list_t *asked = NULL; + pmconflict_t *conflict = NULL; for(i = deps; i && !errorout; i = i->next) { - pmconflict_t *conflict = i->data; pmsyncpkg_t *sync; pmpkg_t *found = NULL; + conflict = i->data; _alpm_log(PM_LOG_DEBUG, "package '%s' conflicts with '%s'\n", conflict->package1, conflict->package2); /* check if the conflicting package is about to be removed/replaced. @@ -614,42 +615,35 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync /* abort */ _alpm_log(PM_LOG_ERROR, _("unresolvable package conflicts detected\n")); errorout = 1; - if(data) { - if((conflict = malloc(sizeof(pmconflict_t))) == NULL) { - _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %zd bytes\n"), sizeof(pmconflict_t)); - FREELIST(*data); - pm_errno = PM_ERR_MEMORY; - ret = -1; - goto cleanup; - } - *conflict = *(pmconflict_t *)i->data; - *data = alpm_list_add(*data, conflict); - } } } } else { _alpm_log(PM_LOG_ERROR, _("unresolvable package conflicts detected\n")); errorout = 1; - if(data) { - if((conflict = malloc(sizeof(pmconflict_t))) == NULL) { - _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %zd bytes\n"), sizeof(pmconflict_t)); - FREELIST(*data); - pm_errno = PM_ERR_MEMORY; - ret = -1; - goto cleanup; - } - *conflict = *(pmconflict_t *)i->data; - *data = alpm_list_add(*data, conflict); - } } } if(errorout) { + /* The last conflict was unresolvable, so we duplicate it and add it to *data */ pm_errno = PM_ERR_CONFLICTING_DEPS; + if(data) { + pmconflict_t *lastconflict = conflict; + if((conflict = malloc(sizeof(pmconflict_t))) == NULL) { + _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %zd bytes\n"), + sizeof(pmconflict_t)); + FREELIST(*data); + pm_errno = PM_ERR_MEMORY; + } else { + *conflict = *lastconflict; + *data = alpm_list_add(*data, conflict); + } + } + FREELIST(asked); + FREELIST(deps); ret = -1; goto cleanup; } - FREELIST(deps); FREELIST(asked); + FREELIST(deps); } EVENT(trans, PM_TRANS_EVT_INTERCONFLICTS_DONE, NULL, NULL); } diff --git a/src/pacman/add.c b/src/pacman/add.c index 7d18749..e04707f 100644 --- a/src/pacman/add.c +++ b/src/pacman/add.c @@ -175,6 +175,7 @@ int pacman_add(alpm_list_t *targets) break; } add_cleanup(); + alpm_list_free_inner(data, free); alpm_list_free(data); return(1); } diff --git a/src/pacman/remove.c b/src/pacman/remove.c index e1e4f04..1028d9e 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -133,6 +133,7 @@ int pacman_remove(alpm_list_t *targets) depstring); free(depstring); } + alpm_list_free_inner(data, free); alpm_list_free(data); break; default: diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 41d18a9..889c682 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -621,7 +621,7 @@ int sync_trans(alpm_list_t *targets, int sync_only) case PM_ERR_CONFLICTING_DEPS: for(i = data; i; i = alpm_list_next(i)) { pmconflict_t *conflict = alpm_list_getdata(i); - printf(_(":: %s: conflicts with %s"), + printf(_(":: %s: conflicts with %s\n"), alpm_conflict_get_package1(conflict), alpm_conflict_get_package2(conflict)); } break; @@ -706,6 +706,7 @@ int sync_trans(alpm_list_t *targets, int sync_only) /* Step 4: release transaction resources */ cleanup: if(data) { + alpm_list_free_inner(data, free); alpm_list_free(data); } if(alpm_trans_release() == -1) { -- 1.5.3.6
This looks good, but take a look at my one thought below in the patch. On Nov 23, 2007 6:19 PM, Chantry Xavier <shiningxc@gmail.com> wrote:
* The frontend calls alpm_trans_prepare(&data), and in case of errors, receive the missing dependencies / conflicts / etc in the data pointer. It apparently needs to free this structure totally with : alpm_list_free_inner(data, free) alpm_list_free(data)
So I added alpm_list_free_inner(data, free) in pacman/{sync.c,remove.c,add,c}
* in _alpm_sync_prepare, the deps and asked lists were not freed in case of errors (unresolvable conflicts). Besides the code for handling this case was duplicated.
* in _alpm_remove_commit, free was used instead of alpm_list_free for newfiles.
* newline fix in pacman/sync.c
Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- lib/libalpm/remove.c | 2 +- lib/libalpm/sync.c | 42 ++++++++++++++++++------------------------ src/pacman/add.c | 1 + src/pacman/remove.c | 1 + src/pacman/sync.c | 3 ++- 5 files changed, 23 insertions(+), 26 deletions(-)
diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 592cf39..e1f19ec 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -315,7 +315,7 @@ int _alpm_remove_commit(pmtrans_t *trans, pmdb_t *db) (pkg_count - alpm_list_count(targ) + 1)); position++; } - free(newfiles); + alpm_list_free(newfiles); }
/* set progress to 100% after we finish unlinking files */ diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index f03a78b..b49bbfb 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -496,12 +496,13 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync if(deps) { int errorout = 0; alpm_list_t *asked = NULL; + pmconflict_t *conflict = NULL;
for(i = deps; i && !errorout; i = i->next) { - pmconflict_t *conflict = i->data; pmsyncpkg_t *sync; pmpkg_t *found = NULL;
+ conflict = i->data; _alpm_log(PM_LOG_DEBUG, "package '%s' conflicts with '%s'\n", conflict->package1, conflict->package2); /* check if the conflicting package is about to be removed/replaced. @@ -614,42 +615,35 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync /* abort */ _alpm_log(PM_LOG_ERROR, _("unresolvable package conflicts detected\n")); errorout = 1; - if(data) { - if((conflict = malloc(sizeof(pmconflict_t))) == NULL) { - _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %zd bytes\n"), sizeof(pmconflict_t)); - FREELIST(*data); - pm_errno = PM_ERR_MEMORY; - ret = -1; - goto cleanup; - } - *conflict = *(pmconflict_t *)i->data; - *data = alpm_list_add(*data, conflict); - } } } } else { _alpm_log(PM_LOG_ERROR, _("unresolvable package conflicts detected\n")); errorout = 1; - if(data) { - if((conflict = malloc(sizeof(pmconflict_t))) == NULL) { - _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %zd bytes\n"), sizeof(pmconflict_t)); - FREELIST(*data); - pm_errno = PM_ERR_MEMORY; - ret = -1; - goto cleanup; - } - *conflict = *(pmconflict_t *)i->data; - *data = alpm_list_add(*data, conflict); - } } } if(errorout) { + /* The last conflict was unresolvable, so we duplicate it and add it to *data */ pm_errno = PM_ERR_CONFLICTING_DEPS; + if(data) { + pmconflict_t *lastconflict = conflict; + if((conflict = malloc(sizeof(pmconflict_t))) == NULL) { + _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %zd bytes\n"), + sizeof(pmconflict_t)); + FREELIST(*data); + pm_errno = PM_ERR_MEMORY; Can the above be replaced by a call to MALLOC?
+ } else { + *conflict = *lastconflict; + *data = alpm_list_add(*data, conflict); + } + } + FREELIST(asked); + FREELIST(deps); ret = -1; goto cleanup; } - FREELIST(deps); FREELIST(asked); + FREELIST(deps); } EVENT(trans, PM_TRANS_EVT_INTERCONFLICTS_DONE, NULL, NULL); } diff --git a/src/pacman/add.c b/src/pacman/add.c index 7d18749..e04707f 100644 --- a/src/pacman/add.c +++ b/src/pacman/add.c @@ -175,6 +175,7 @@ int pacman_add(alpm_list_t *targets) break; } add_cleanup(); + alpm_list_free_inner(data, free); alpm_list_free(data); return(1); } diff --git a/src/pacman/remove.c b/src/pacman/remove.c index e1e4f04..1028d9e 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -133,6 +133,7 @@ int pacman_remove(alpm_list_t *targets) depstring); free(depstring); } + alpm_list_free_inner(data, free); alpm_list_free(data); break; default: diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 41d18a9..889c682 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -621,7 +621,7 @@ int sync_trans(alpm_list_t *targets, int sync_only) case PM_ERR_CONFLICTING_DEPS: for(i = data; i; i = alpm_list_next(i)) { pmconflict_t *conflict = alpm_list_getdata(i); - printf(_(":: %s: conflicts with %s"), + printf(_(":: %s: conflicts with %s\n"), alpm_conflict_get_package1(conflict), alpm_conflict_get_package2(conflict)); } break; @@ -706,6 +706,7 @@ int sync_trans(alpm_list_t *targets, int sync_only) /* Step 4: release transaction resources */ cleanup: if(data) { + alpm_list_free_inner(data, free); alpm_list_free(data); } if(alpm_trans_release() == -1) { -- 1.5.3.6
On Fri, Nov 23, 2007 at 11:51:01PM -0500, Dan McGee wrote:
This looks good, but take a look at my one thought below in the patch.
On Nov 23, 2007 6:19 PM, Chantry Xavier <shiningxc@gmail.com> wrote:
@@ -614,42 +615,35 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync if(errorout) { + /* The last conflict was unresolvable, so we duplicate it and add it to *data */ pm_errno = PM_ERR_CONFLICTING_DEPS; + if(data) { + pmconflict_t *lastconflict = conflict; + if((conflict = malloc(sizeof(pmconflict_t))) == NULL) { + _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %zd bytes\n"), + sizeof(pmconflict_t)); + FREELIST(*data); + pm_errno = PM_ERR_MEMORY; Can the above be replaced by a call to MALLOC?
That's the first thing I thought about, but I don't know if that is possible. There are two lists that can only be freed after the error happens : *data and deps (asked could be freed before). Or maybe it is possible to do something like : MALLOC(miss, sizeof(pmdepmissing_t), FREELIST(*data); FREELIST(deps); RET_ERR(PM_ERR_MEMORY, -1)); Though, there is also the "goto cleanup;" that would be skipped, and so the "alpm_list_free(list);", so that's yet another call to add there. So that would duplicate the "FREELIST(deps)'" and "alpm_list_free(list);" calls.
+ } else { + *conflict = *lastconflict; + *data = alpm_list_add(*data, conflict); + } + } + FREELIST(asked); + FREELIST(deps); ret = -1; goto cleanup; } - FREELIST(deps); FREELIST(asked); + FREELIST(deps); } EVENT(trans, PM_TRANS_EVT_INTERCONFLICTS_DONE, NULL, NULL); }
On Nov 24, 2007 5:02 AM, Xavier <shiningxc@gmail.com> wrote:
That's the first thing I thought about, but I don't know if that is possible. There are two lists that can only be freed after the error happens : *data and deps (asked could be freed before). Or maybe it is possible to do something like : MALLOC(miss, sizeof(pmdepmissing_t), FREELIST(*data); FREELIST(deps); RET_ERR(PM_ERR_MEMORY, -1));
Though, there is also the "goto cleanup;" that would be skipped, and so the "alpm_list_free(list);", so that's yet another call to add there. So that would duplicate the "FREELIST(deps)'" and "alpm_list_free(list);" calls.
While this one has already been merged, you could still use the macro by simply throwing something like (void)0 in the third param. Or even pm_errno = BLAHBLAH. This way we still get the output from the macro, even if we don't get the real failure+return.
On Mon, Nov 26, 2007 at 03:49:14PM -0600, Aaron Griffin wrote:
On Nov 24, 2007 5:02 AM, Xavier <shiningxc@gmail.com> wrote:
That's the first thing I thought about, but I don't know if that is possible. There are two lists that can only be freed after the error happens : *data and deps (asked could be freed before). Or maybe it is possible to do something like : MALLOC(miss, sizeof(pmdepmissing_t), FREELIST(*data); FREELIST(deps); RET_ERR(PM_ERR_MEMORY, -1));
Though, there is also the "goto cleanup;" that would be skipped, and so the "alpm_list_free(list);", so that's yet another call to add there. So that would duplicate the "FREELIST(deps)'" and "alpm_list_free(list);" calls.
While this one has already been merged, you could still use the macro by simply throwing something like (void)0 in the third param. Or even pm_errno = BLAHBLAH. This way we still get the output from the macro, even if we don't get the real failure+return.
I see, but how do we detect if it failed or not? Using 0, it doesn't seem to be possible. Maybe by setting pm_errno as you said, or ret. So maybe something like this : MALLOC(miss, sizeof(pmdepmissing_t), ret = -1); if(ret == -1) { /* free the lists */ pm_errno = blabla; goto cleanup; } ? I don't know how MACRO handle arguments, so I wasn't sure this was possible.
On Nov 26, 2007 5:07 PM, Xavier <shiningxc@gmail.com> wrote:
On Mon, Nov 26, 2007 at 03:49:14PM -0600, Aaron Griffin wrote:
On Nov 24, 2007 5:02 AM, Xavier <shiningxc@gmail.com> wrote:
That's the first thing I thought about, but I don't know if that is possible. There are two lists that can only be freed after the error happens : *data and deps (asked could be freed before). Or maybe it is possible to do something like : MALLOC(miss, sizeof(pmdepmissing_t), FREELIST(*data); FREELIST(deps); RET_ERR(PM_ERR_MEMORY, -1));
Though, there is also the "goto cleanup;" that would be skipped, and so the "alpm_list_free(list);", so that's yet another call to add there. So that would duplicate the "FREELIST(deps)'" and "alpm_list_free(list);" calls.
While this one has already been merged, you could still use the macro by simply throwing something like (void)0 in the third param. Or even pm_errno = BLAHBLAH. This way we still get the output from the macro, even if we don't get the real failure+return.
I see, but how do we detect if it failed or not? Using 0, it doesn't seem to be possible. Maybe by setting pm_errno as you said, or ret.
So maybe something like this :
MALLOC(miss, sizeof(pmdepmissing_t), ret = -1); if(ret == -1) { /* free the lists */ pm_errno = blabla; goto cleanup; }
?
I don't know how MACRO handle arguments, so I wasn't sure this was possible.
I think it looks OK. Remember that a macro gets handled during preprocessing and isn't a function call at all, so you can do a few things you wouldn't normally be able to. Want to check? Look into using "gcc -E <sourcefile>", which just spits the preprocessed code back at you so you can check things. -Dan
On Mon, Nov 26, 2007 at 05:37:34PM -0500, Dan McGee wrote:
MALLOC(miss, sizeof(pmdepmissing_t), ret = -1); if(ret == -1) { /* free the lists */ pm_errno = blabla; goto cleanup; }
?
I don't know how MACRO handle arguments, so I wasn't sure this was possible.
I think it looks OK. Remember that a macro gets handled during preprocessing and isn't a function call at all, so you can do a few things you wouldn't normally be able to. Want to check? Look into using "gcc -E <sourcefile>", which just spits the preprocessed code back at you so you can check things.
Ok, thanks for the tip. Does it still look OK by using pm_errno instead of ret? MALLOC(miss, sizeof(pmdepmissing_t), pm_errno = PM_ERR_MEMORY); if(om_errno == PM_ERR_MEMORY) { /* free the lists */ ret = -1; goto cleanup; } I'm asking because I noticed MALLOC could be used in resolvedeps too, but there isn't already a ret variable there :) I made this change for resolvedeps in my working branch, and for sync_prepare in my sync branch.
On Nov 26, 2007 5:31 PM, Xavier <shiningxc@gmail.com> wrote:
Does it still look OK by using pm_errno instead of ret?
MALLOC(miss, sizeof(pmdepmissing_t), pm_errno = PM_ERR_MEMORY); if(om_errno == PM_ERR_MEMORY) { /* free the lists */ ret = -1; goto cleanup; }
I'm asking because I noticed MALLOC could be used in resolvedeps too, but there isn't already a ret variable there :) I made this change for resolvedeps in my working branch, and for sync_prepare in my sync branch.
You could also always just check if(!miss), which is what I'd do. That's why I mentioned just doing nothing in the third param. Either way it's the same thing, so just do what you're comfortable with.
Good catches!
* in _alpm_sync_prepare, the deps and asked lists were not freed in case of errors (unresolvable conflicts). Besides the code for handling this case was duplicated.
The good old sync.c: probably you can find more bugs in sync_prepare monster;-) I don't want to hurt anyone, but I couldn't find any concept in sync_prepare [as I said earlier: that looks a bunch of #FS fixes instead]. The asked list is totally needless for example... So I suggest again rewriting it from scratch (with or without using my pending patches) <- it is easier then review the current code imho. Bye ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
On Sat, Nov 24, 2007 at 02:00:53PM +0100, Nagy Gabor wrote:
Good catches!
* in _alpm_sync_prepare, the deps and asked lists were not freed in case of errors (unresolvable conflicts). Besides the code for handling this case was duplicated.
The good old sync.c: probably you can find more bugs in sync_prepare monster;-) I don't want to hurt anyone, but I couldn't find any concept in sync_prepare [as I said earlier: that looks a bunch of #FS fixes instead]. The asked list is totally needless for example...
So I suggest again rewriting it from scratch (with or without using my pending patches) <- it is easier then review the current code imho.
You're right, trying to fix a little memleak in this messy code doesn't help much, but it didn't require much efforts either. I wasn't trying to review this function anyway. I just ran several pactests with valgrind, and tried to fix all memleaks I could find. One happened to be in that part.
participants (5)
-
Aaron Griffin
-
Chantry Xavier
-
Dan McGee
-
Nagy Gabor
-
Xavier