[pacman-dev] [PATCH] Fix several memleaks, mostly related to errors handling.
Dan McGee
dpmcgee at gmail.com
Fri Nov 23 23:51:01 EST 2007
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 at 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 at 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
More information about the pacman-dev
mailing list