[pacman-dev] [PATCH 1/8] avoid double freeing unresolvable packages
If an unresolvable package was loaded from a file it would be free'd again when freeing trans->add. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/trans.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index ada8100..4930e94 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -246,12 +246,19 @@ int SYMEXPORT alpm_trans_release(alpm_handle_t *handle) void _alpm_trans_free(alpm_trans_t *trans) { + alpm_list_t *i; + if(trans == NULL) { return; } - alpm_list_free_inner(trans->unresolvable, - (alpm_list_fn_free)_alpm_pkg_free_trans); + /* only free packages not in trans->add to avoid + * double free'ing packages loaded from files. */ + for(i = trans->unresolvable; i; i = i->next) { + if(!alpm_list_find_ptr(trans->add, i->data)) { + _alpm_pkg_free_trans(i->data); + } + } alpm_list_free(trans->unresolvable); alpm_list_free_inner(trans->add, (alpm_list_fn_free)_alpm_pkg_free_trans); alpm_list_free(trans->add); -- 1.8.5.2
If the user opted not to remove the unresolvable packages from the transaction, the list wasn't saved to the transaction to be free'd in trans_release. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/sync.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 4ae01ac..b6061db 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -370,7 +370,6 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) { alpm_list_t *i, *j; alpm_list_t *deps = NULL; - alpm_list_t *unresolvable = NULL; size_t from_sync = 0; int ret = 0; alpm_trans_t *trans = handle->trans; @@ -427,7 +426,11 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) alpm_pkg_t *pkg = i->data; if(_alpm_resolvedeps(handle, localpkgs, pkg, trans->add, &resolved, remove, data) == -1) { - unresolvable = alpm_list_add(unresolvable, pkg); + /* Unresolvable packages will be removed from the target list; set + * these aside in the transaction as a list we won't operate on. If we + * free them before the end of the transaction, we may kill pointers + * the frontend holds to package objects. */ + trans->unresolvable = alpm_list_add(trans->unresolvable, pkg); } /* Else, [resolved] now additionally contains [pkg] and all of its dependencies not already on the list */ @@ -437,10 +440,10 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) /* If there were unresolvable top-level packages, prompt the user to see if they'd like to ignore them rather than failing the sync */ - if(unresolvable != NULL) { + if(trans->unresolvable != NULL) { int remove_unresolvable = 0; alpm_errno_t saved_err = handle->pm_errno; - QUESTION(handle, ALPM_QUESTION_REMOVE_PKGS, unresolvable, + QUESTION(handle, ALPM_QUESTION_REMOVE_PKGS, trans->unresolvable, NULL, NULL, &remove_unresolvable); if(remove_unresolvable) { /* User wants to remove the unresolvable packages from the @@ -470,12 +473,6 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) } } - /* Unresolvable packages will be removed from the target list; set these - * aside in the transaction as a list we won't operate on. If we free them - * before the end of the transaction, we may kill pointers the frontend - * holds to package objects. */ - trans->unresolvable = unresolvable; - alpm_list_free(trans->add); trans->add = resolved; -- 1.8.5.2
If the db directory did not exist when local_db_populate was called, the pkgcache wouldn't be initialized, causing pkghash_add_pkg to fail. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/be_local.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c index 6029e12..671eed1 100644 --- a/lib/libalpm/be_local.c +++ b/lib/libalpm/be_local.c @@ -459,6 +459,7 @@ static int local_db_populate(alpm_db_t *db) /* no database existing yet is not an error */ db->status &= ~DB_STATUS_EXISTS; db->status |= DB_STATUS_MISSING; + db->pkgcache = _alpm_pkghash_create(0); return 0; } RET_ERR(db->handle, ALPM_ERR_DB_OPEN, -1); -- 1.8.5.2
alpm_pkg_compute_optional returns a generated list that needs to be free'd. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/package.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/pacman/package.c b/src/pacman/package.c index 8ccdc1b..be01f63 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -235,6 +235,7 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) printf("\n"); FREELIST(requiredby); + FREELIST(optionalfor); alpm_list_free(validation); } -- 1.8.5.2
_alpm_resolvedeps resets pm_errno to 0 by calling alpm_checkdeps. Whenever the last call succeeded, pm_errno was not properly set, preventing pacman from properly handling the error and leaking additional memory. We know pm_errno should be ALPM_ERR_UNSATISFIED_DEPS if resolvedeps has failed, so just set it manually. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/sync.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index b6061db..8b8a6ad 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -442,7 +442,6 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) see if they'd like to ignore them rather than failing the sync */ if(trans->unresolvable != NULL) { int remove_unresolvable = 0; - alpm_errno_t saved_err = handle->pm_errno; QUESTION(handle, ALPM_QUESTION_REMOVE_PKGS, trans->unresolvable, NULL, NULL, &remove_unresolvable); if(remove_unresolvable) { @@ -458,7 +457,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) } } else { /* pm_errno was set by resolvedeps, callback may have overwrote it */ - handle->pm_errno = saved_err; + handle->pm_errno = ALPM_ERR_UNSATISFIED_DEPS; alpm_list_free(resolved); ret = -1; goto cleanup; -- 1.8.5.2
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/deps.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index 4eb779f..1f6ad11 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -833,8 +833,9 @@ int _alpm_resolvedeps(alpm_handle_t *handle, alpm_list_t *localpkgs, } ret = -1; } + alpm_list_free(targ); + targ = NULL; } - alpm_list_free(targ); alpm_list_free(deps); if(ret != 0) { -- 1.8.5.2
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/util.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/pacman/util.c b/src/pacman/util.c index 58b0cec..1982c89 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -589,9 +589,10 @@ static int table_display(const alpm_list_t *header, const alpm_list_t *i, *first; size_t *widths = NULL, totalcols, totalwidth; int *has_data = NULL; + int ret = 0; if(rows == NULL) { - return 0; + return ret; } /* we want the first row. if no headers are provided, use the first @@ -605,10 +606,12 @@ static int table_display(const alpm_list_t *header, if(totalwidth > cols) { pm_printf(ALPM_LOG_WARNING, _("insufficient columns available for table display\n")); - return -1; + ret = -1; + goto cleanup; } if(!totalwidth || !widths || !has_data) { - return -1; + ret = -1; + goto cleanup; } if(header) { @@ -620,9 +623,10 @@ static int table_display(const alpm_list_t *header, table_print_line(i->data, padding, totalcols, widths, has_data); } +cleanup: free(widths); free(has_data); - return 0; + return ret; } void list_display(const char *title, const alpm_list_t *list, -- 1.8.5.2
Front-ends should be able to free memory that alpm hands them. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/alpm.h | 4 ++++ lib/libalpm/conflict.c | 6 +++--- lib/libalpm/conflict.h | 3 --- lib/libalpm/deps.c | 8 ++++---- lib/libalpm/deps.h | 1 - lib/libalpm/remove.c | 6 +++--- lib/libalpm/sync.c | 14 +++++++------- src/pacman/remove.c | 3 ++- src/pacman/sync.c | 13 ++++++++----- 9 files changed, 31 insertions(+), 27 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 5628527..28ae851 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -1351,6 +1351,10 @@ enum alpm_caps { const char *alpm_version(void); enum alpm_caps alpm_capabilities(void); +void alpm_fileconflict_free(alpm_fileconflict_t *conflict); +void alpm_depmiss_free(alpm_depmissing_t *miss); +void alpm_conflict_free(alpm_conflict_t *conflict); + /* End of alpm_api */ /** @} */ diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 5a6e2be..4a1d77e 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -62,7 +62,7 @@ static alpm_conflict_t *conflict_new(alpm_pkg_t *pkg1, alpm_pkg_t *pkg2, /** * @brief Free a conflict and its members. */ -void _alpm_conflict_free(alpm_conflict_t *conflict) +void SYMEXPORT alpm_conflict_free(alpm_conflict_t *conflict) { FREE(conflict->package2); FREE(conflict->package1); @@ -135,7 +135,7 @@ static int add_conflict(alpm_handle_t *handle, alpm_list_t **baddeps, pkg1->name, pkg2->name, conflict_str); free(conflict_str); } else { - _alpm_conflict_free(conflict); + alpm_conflict_free(conflict); } return 0; } @@ -290,7 +290,7 @@ error: /** * @brief Frees a conflict and its members. */ -void _alpm_fileconflict_free(alpm_fileconflict_t *conflict) +void SYMEXPORT alpm_fileconflict_free(alpm_fileconflict_t *conflict) { FREE(conflict->ctarget); FREE(conflict->file); diff --git a/lib/libalpm/conflict.h b/lib/libalpm/conflict.h index f7a667d..182eb0e 100644 --- a/lib/libalpm/conflict.h +++ b/lib/libalpm/conflict.h @@ -25,14 +25,11 @@ #include "package.h" alpm_conflict_t *_alpm_conflict_dup(const alpm_conflict_t *conflict); -void _alpm_conflict_free(alpm_conflict_t *conflict); alpm_list_t *_alpm_innerconflicts(alpm_handle_t *handle, alpm_list_t *packages); alpm_list_t *_alpm_outerconflicts(alpm_db_t *db, alpm_list_t *packages); alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, alpm_list_t *upgrade, alpm_list_t *remove); -void _alpm_fileconflict_free(alpm_fileconflict_t *conflict); - #endif /* _ALPM_CONFLICT_H */ /* vim: set ts=2 sw=2 noet: */ diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index 1f6ad11..7ee3ee4 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -57,7 +57,7 @@ static alpm_depmissing_t *depmiss_new(const char *target, alpm_depend_t *dep, return miss; } -void _alpm_depmiss_free(alpm_depmissing_t *miss) +void SYMEXPORT alpm_depmiss_free(alpm_depmissing_t *miss) { _alpm_dep_free(miss->depend); FREE(miss->target); @@ -804,7 +804,7 @@ int _alpm_resolvedeps(alpm_handle_t *handle, alpm_list_t *localpkgs, /* check if one of the packages in the [*packages] list already satisfies * this dependency */ if(find_dep_satisfier(*packages, missdep)) { - _alpm_depmiss_free(miss); + alpm_depmiss_free(miss); continue; } /* check if one of the packages in the [preferred] list already satisfies @@ -818,9 +818,9 @@ int _alpm_resolvedeps(alpm_handle_t *handle, alpm_list_t *localpkgs, _alpm_log(handle, ALPM_LOG_DEBUG, "pulling dependency %s (needed by %s)\n", spkg->name, pkg->name); - _alpm_depmiss_free(miss); + alpm_depmiss_free(miss); } else if(resolvedep(handle, missdep, (targ = alpm_list_add(NULL, handle->db_local)), rem, 0)) { - _alpm_depmiss_free(miss); + alpm_depmiss_free(miss); } else { handle->pm_errno = ALPM_ERR_UNSATISFIED_DEPS; char *missdepstring = alpm_dep_compute_string(missdep); diff --git a/lib/libalpm/deps.h b/lib/libalpm/deps.h index 546521f..0d41d49 100644 --- a/lib/libalpm/deps.h +++ b/lib/libalpm/deps.h @@ -29,7 +29,6 @@ void _alpm_dep_free(alpm_depend_t *dep); alpm_depend_t *_alpm_dep_dup(const alpm_depend_t *dep); -void _alpm_depmiss_free(alpm_depmissing_t *miss); alpm_list_t *_alpm_sortbydeps(alpm_handle_t *handle, alpm_list_t *targets, alpm_list_t *ignore, int reverse); int _alpm_recursedeps(alpm_db_t *db, alpm_list_t **targs, int include_explicit); diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 899952b..04831fa 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -116,7 +116,7 @@ static int remove_prepare_cascade(alpm_handle_t *handle, alpm_list_t *lp) _("could not find %s in database -- skipping\n"), miss->target); } } - alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(lp, (alpm_list_fn_free)alpm_depmiss_free); alpm_list_free(lp); lp = alpm_checkdeps(handle, _alpm_db_get_pkgcache(handle->db_local), trans->remove, NULL, 1); @@ -153,7 +153,7 @@ static void remove_prepare_keep_needed(alpm_handle_t *handle, alpm_list_t *lp) _alpm_pkg_free(pkg); } } - alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(lp, (alpm_list_fn_free)alpm_depmiss_free); alpm_list_free(lp); lp = alpm_checkdeps(handle, _alpm_db_get_pkgcache(handle->db_local), trans->remove, NULL, 1); @@ -232,7 +232,7 @@ int _alpm_remove_prepare(alpm_handle_t *handle, alpm_list_t **data) if(data) { *data = lp; } else { - alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(lp, (alpm_list_fn_free)alpm_depmiss_free); alpm_list_free(lp); } RET_ERR(handle, ALPM_ERR_UNSATISFIED_DEPS, -1); diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 8b8a6ad..1bedbd1 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -451,7 +451,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) dependency-reordered list below */ handle->pm_errno = 0; if(data) { - alpm_list_free_inner(*data, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(*data, (alpm_list_fn_free)alpm_depmiss_free); alpm_list_free(*data); *data = NULL; } @@ -521,7 +521,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) *data = alpm_list_add(*data, newconflict); } } - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_conflict_free); + alpm_list_free_inner(deps, (alpm_list_fn_free)alpm_conflict_free); alpm_list_free(deps); _alpm_dep_free(dep1); _alpm_dep_free(dep2); @@ -539,7 +539,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) trans->unresolvable = alpm_list_add(trans->unresolvable, rsync); } - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_conflict_free); + alpm_list_free_inner(deps, (alpm_list_fn_free)alpm_conflict_free); alpm_list_free(deps); deps = NULL; @@ -585,13 +585,13 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) *data = alpm_list_add(*data, newconflict); } } - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_conflict_free); + alpm_list_free_inner(deps, (alpm_list_fn_free)alpm_conflict_free); alpm_list_free(deps); goto cleanup; } } EVENT(handle, ALPM_EVENT_INTERCONFLICTS_DONE, NULL, NULL); - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_conflict_free); + alpm_list_free_inner(deps, (alpm_list_fn_free)alpm_conflict_free); alpm_list_free(deps); } @@ -621,7 +621,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) if(data) { *data = deps; } else { - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(deps, (alpm_list_fn_free)alpm_depmiss_free); alpm_list_free(deps); } goto cleanup; @@ -1267,7 +1267,7 @@ int _alpm_sync_commit(alpm_handle_t *handle, alpm_list_t **data) if(data) { *data = conflict; } else { - alpm_list_free_inner(conflict, (alpm_list_fn_free)_alpm_fileconflict_free); + alpm_list_free_inner(conflict, (alpm_list_fn_free)alpm_fileconflict_free); alpm_list_free(conflict); } RET_ERR(handle, ALPM_ERR_FILE_CONFLICTS, -1); diff --git a/src/pacman/remove.c b/src/pacman/remove.c index aefe454..368c2e8 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -121,12 +121,13 @@ int pacman_remove(alpm_list_t *targets) char *depstring = alpm_dep_compute_string(miss->depend); colon_printf(_("%s: requires %s\n"), miss->target, depstring); free(depstring); + alpm_depmiss_free(miss); } break; default: break; } - FREELIST(data); + alpm_list_free(data); retval = 1; goto cleanup; } diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 5ed43ed..e1df860 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -763,8 +763,9 @@ int sync_prepare_execute(void) switch(err) { case ALPM_ERR_PKG_INVALID_ARCH: for(i = data; i; i = alpm_list_next(i)) { - const char *pkg = i->data; + char *pkg = i->data; colon_printf(_("package %s does not have a valid architecture\n"), pkg); + free(pkg); } break; case ALPM_ERR_UNSATISFIED_DEPS: @@ -773,6 +774,7 @@ int sync_prepare_execute(void) char *depstring = alpm_dep_compute_string(miss->depend); colon_printf(_("%s: requires %s\n"), miss->target, depstring); free(depstring); + alpm_depmiss_free(miss); } break; case ALPM_ERR_CONFLICTING_DEPS: @@ -788,6 +790,7 @@ int sync_prepare_execute(void) conflict->package1, conflict->package2, reason); free(reason); } + alpm_conflict_free(conflict); } break; default: @@ -847,6 +850,7 @@ int sync_prepare_execute(void) conflict->target, conflict->file); break; } + alpm_fileconflict_free(conflict); } break; case ALPM_ERR_PKG_INVALID: @@ -854,8 +858,9 @@ int sync_prepare_execute(void) case ALPM_ERR_PKG_INVALID_SIG: case ALPM_ERR_DLT_INVALID: for(i = data; i; i = alpm_list_next(i)) { - const char *filename = i->data; + char *filename = i->data; printf(_("%s is invalid or corrupted\n"), filename); + free(filename); } break; default: @@ -869,9 +874,7 @@ int sync_prepare_execute(void) /* Step 4: release transaction resources */ cleanup: - if(data) { - FREELIST(data); - } + alpm_list_free(data); if(trans_release() == -1) { retval = 1; } -- 1.8.5.2
On 07/01/14 02:52, Andrew Gregory wrote:
Front-ends should be able to free memory that alpm hands them.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/alpm.h | 4 ++++ lib/libalpm/conflict.c | 6 +++--- lib/libalpm/conflict.h | 3 --- lib/libalpm/deps.c | 8 ++++---- lib/libalpm/deps.h | 1 - lib/libalpm/remove.c | 6 +++--- lib/libalpm/sync.c | 14 +++++++------- src/pacman/remove.c | 3 ++- src/pacman/sync.c | 13 ++++++++----- 9 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 5628527..28ae851 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -1351,6 +1351,10 @@ enum alpm_caps { const char *alpm_version(void); enum alpm_caps alpm_capabilities(void);
+void alpm_fileconflict_free(alpm_fileconflict_t *conflict); +void alpm_depmiss_free(alpm_depmissing_t *miss);
alpm_depmissing_free() - no point save three letters I was looking for a better position in the head to group these, but did not really see anything. Unless someone else has a good idea, I guess they will be fine there. Rest of patch was fine.
On 07/01/14 02:52, Andrew Gregory wrote:
If an unresolvable package was loaded from a file it would be free'd again when freeing trans->add.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
This patch I do not understand. Where does anything get added to trans->unresolvable that is not removed from trans->add?
lib/libalpm/trans.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index ada8100..4930e94 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -246,12 +246,19 @@ int SYMEXPORT alpm_trans_release(alpm_handle_t *handle)
void _alpm_trans_free(alpm_trans_t *trans) { + alpm_list_t *i; + if(trans == NULL) { return; }
- alpm_list_free_inner(trans->unresolvable, - (alpm_list_fn_free)_alpm_pkg_free_trans); + /* only free packages not in trans->add to avoid + * double free'ing packages loaded from files. */ + for(i = trans->unresolvable; i; i = i->next) { + if(!alpm_list_find_ptr(trans->add, i->data)) { + _alpm_pkg_free_trans(i->data); + } + } alpm_list_free(trans->unresolvable); alpm_list_free_inner(trans->add, (alpm_list_fn_free)_alpm_pkg_free_trans); alpm_list_free(trans->add);
On 07/01/14 02:52, Andrew Gregory wrote:
If the user opted not to remove the unresolvable packages from the transaction, the list wasn't saved to the transaction to be free'd in trans_release.
It is not removed from trans->add. See below.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/sync.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 4ae01ac..b6061db 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -370,7 +370,6 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) { alpm_list_t *i, *j; alpm_list_t *deps = NULL; - alpm_list_t *unresolvable = NULL; size_t from_sync = 0; int ret = 0; alpm_trans_t *trans = handle->trans; @@ -427,7 +426,11 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) alpm_pkg_t *pkg = i->data; if(_alpm_resolvedeps(handle, localpkgs, pkg, trans->add, &resolved, remove, data) == -1) { - unresolvable = alpm_list_add(unresolvable, pkg); + /* Unresolvable packages will be removed from the target list; set + * these aside in the transaction as a list we won't operate on. If we + * free them before the end of the transaction, we may kill pointers + * the frontend holds to package objects. */ + trans->unresolvable = alpm_list_add(trans->unresolvable, pkg); } /* Else, [resolved] now additionally contains [pkg] and all of its dependencies not already on the list */ @@ -437,10 +440,10 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data)
/* If there were unresolvable top-level packages, prompt the user to see if they'd like to ignore them rather than failing the sync */ - if(unresolvable != NULL) { + if(trans->unresolvable != NULL) { int remove_unresolvable = 0; alpm_errno_t saved_err = handle->pm_errno; - QUESTION(handle, ALPM_QUESTION_REMOVE_PKGS, unresolvable, + QUESTION(handle, ALPM_QUESTION_REMOVE_PKGS, trans->unresolvable, NULL, NULL, &remove_unresolvable); if(remove_unresolvable) { /* User wants to remove the unresolvable packages from the @@ -470,12 +473,6 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) } }
- /* Unresolvable packages will be removed from the target list; set these - * aside in the transaction as a list we won't operate on. If we free them - * before the end of the transaction, we may kill pointers the frontend - * holds to package objects. */ - trans->unresolvable = unresolvable; - alpm_list_free(trans->add); trans->add = resolved;
Here... when the unresolved packages are added to trans->unresolvable, the trans->add list is updated to remove them. What am I missing here? Allan
Front-ends should be able to free memory that alpm hands them. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- renamed alpm_depmiss_free -> alpm_depmissing_free lib/libalpm/alpm.h | 4 ++++ lib/libalpm/conflict.c | 6 +++--- lib/libalpm/conflict.h | 3 --- lib/libalpm/deps.c | 8 ++++---- lib/libalpm/deps.h | 1 - lib/libalpm/remove.c | 7 ++++--- lib/libalpm/sync.c | 16 +++++++++------- src/pacman/remove.c | 3 ++- src/pacman/sync.c | 13 ++++++++----- 9 files changed, 34 insertions(+), 27 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index d67d66a..6a2a1fe 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -1351,6 +1351,10 @@ enum alpm_caps { const char *alpm_version(void); enum alpm_caps alpm_capabilities(void); +void alpm_fileconflict_free(alpm_fileconflict_t *conflict); +void alpm_depmissing_free(alpm_depmissing_t *miss); +void alpm_conflict_free(alpm_conflict_t *conflict); + /* End of alpm_api */ /** @} */ diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 369530b..2611aef 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -62,7 +62,7 @@ static alpm_conflict_t *conflict_new(alpm_pkg_t *pkg1, alpm_pkg_t *pkg2, /** * @brief Free a conflict and its members. */ -void _alpm_conflict_free(alpm_conflict_t *conflict) +void SYMEXPORT alpm_conflict_free(alpm_conflict_t *conflict) { FREE(conflict->package2); FREE(conflict->package1); @@ -135,7 +135,7 @@ static int add_conflict(alpm_handle_t *handle, alpm_list_t **baddeps, pkg1->name, pkg2->name, conflict_str); free(conflict_str); } else { - _alpm_conflict_free(conflict); + alpm_conflict_free(conflict); } return 0; } @@ -290,7 +290,7 @@ error: /** * @brief Frees a conflict and its members. */ -void _alpm_fileconflict_free(alpm_fileconflict_t *conflict) +void SYMEXPORT alpm_fileconflict_free(alpm_fileconflict_t *conflict) { FREE(conflict->ctarget); FREE(conflict->file); diff --git a/lib/libalpm/conflict.h b/lib/libalpm/conflict.h index 8cf36ec..886e927 100644 --- a/lib/libalpm/conflict.h +++ b/lib/libalpm/conflict.h @@ -25,14 +25,11 @@ #include "package.h" alpm_conflict_t *_alpm_conflict_dup(const alpm_conflict_t *conflict); -void _alpm_conflict_free(alpm_conflict_t *conflict); alpm_list_t *_alpm_innerconflicts(alpm_handle_t *handle, alpm_list_t *packages); alpm_list_t *_alpm_outerconflicts(alpm_db_t *db, alpm_list_t *packages); alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, alpm_list_t *upgrade, alpm_list_t *remove); -void _alpm_fileconflict_free(alpm_fileconflict_t *conflict); - #endif /* _ALPM_CONFLICT_H */ /* vim: set noet: */ diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index c7dbe1e..b3de1b0 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -57,7 +57,7 @@ static alpm_depmissing_t *depmiss_new(const char *target, alpm_depend_t *dep, return miss; } -void _alpm_depmiss_free(alpm_depmissing_t *miss) +void SYMEXPORT alpm_depmissing_free(alpm_depmissing_t *miss) { _alpm_dep_free(miss->depend); FREE(miss->target); @@ -804,7 +804,7 @@ int _alpm_resolvedeps(alpm_handle_t *handle, alpm_list_t *localpkgs, /* check if one of the packages in the [*packages] list already satisfies * this dependency */ if(find_dep_satisfier(*packages, missdep)) { - _alpm_depmiss_free(miss); + alpm_depmissing_free(miss); continue; } /* check if one of the packages in the [preferred] list already satisfies @@ -818,9 +818,9 @@ int _alpm_resolvedeps(alpm_handle_t *handle, alpm_list_t *localpkgs, _alpm_log(handle, ALPM_LOG_DEBUG, "pulling dependency %s (needed by %s)\n", spkg->name, pkg->name); - _alpm_depmiss_free(miss); + alpm_depmissing_free(miss); } else if(resolvedep(handle, missdep, (targ = alpm_list_add(NULL, handle->db_local)), rem, 0)) { - _alpm_depmiss_free(miss); + alpm_depmissing_free(miss); } else { handle->pm_errno = ALPM_ERR_UNSATISFIED_DEPS; char *missdepstring = alpm_dep_compute_string(missdep); diff --git a/lib/libalpm/deps.h b/lib/libalpm/deps.h index 8a8ff46..e36bbb3 100644 --- a/lib/libalpm/deps.h +++ b/lib/libalpm/deps.h @@ -29,7 +29,6 @@ void _alpm_dep_free(alpm_depend_t *dep); alpm_depend_t *_alpm_dep_dup(const alpm_depend_t *dep); -void _alpm_depmiss_free(alpm_depmissing_t *miss); alpm_list_t *_alpm_sortbydeps(alpm_handle_t *handle, alpm_list_t *targets, alpm_list_t *ignore, int reverse); int _alpm_recursedeps(alpm_db_t *db, alpm_list_t **targs, int include_explicit); diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 0afca24..6fb82ee 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -116,7 +116,7 @@ static int remove_prepare_cascade(alpm_handle_t *handle, alpm_list_t *lp) _("could not find %s in database -- skipping\n"), miss->target); } } - alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(lp, (alpm_list_fn_free)alpm_depmissing_free); alpm_list_free(lp); lp = alpm_checkdeps(handle, _alpm_db_get_pkgcache(handle->db_local), trans->remove, NULL, 1); @@ -153,7 +153,7 @@ static void remove_prepare_keep_needed(alpm_handle_t *handle, alpm_list_t *lp) _alpm_pkg_free(pkg); } } - alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(lp, (alpm_list_fn_free)alpm_depmissing_free); alpm_list_free(lp); lp = alpm_checkdeps(handle, _alpm_db_get_pkgcache(handle->db_local), trans->remove, NULL, 1); @@ -232,7 +232,8 @@ int _alpm_remove_prepare(alpm_handle_t *handle, alpm_list_t **data) if(data) { *data = lp; } else { - alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(lp, + (alpm_list_fn_free)alpm_depmissing_free); alpm_list_free(lp); } RET_ERR(handle, ALPM_ERR_UNSATISFIED_DEPS, -1); diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index cab4c04..c6c50a4 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -451,7 +451,8 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) dependency-reordered list below */ handle->pm_errno = 0; if(data) { - alpm_list_free_inner(*data, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(*data, + (alpm_list_fn_free)alpm_depmissing_free); alpm_list_free(*data); *data = NULL; } @@ -528,7 +529,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) *data = alpm_list_add(*data, newconflict); } } - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_conflict_free); + alpm_list_free_inner(deps, (alpm_list_fn_free)alpm_conflict_free); alpm_list_free(deps); _alpm_dep_free(dep1); _alpm_dep_free(dep2); @@ -546,7 +547,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) trans->unresolvable = alpm_list_add(trans->unresolvable, rsync); } - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_conflict_free); + alpm_list_free_inner(deps, (alpm_list_fn_free)alpm_conflict_free); alpm_list_free(deps); deps = NULL; @@ -592,13 +593,13 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) *data = alpm_list_add(*data, newconflict); } } - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_conflict_free); + alpm_list_free_inner(deps, (alpm_list_fn_free)alpm_conflict_free); alpm_list_free(deps); goto cleanup; } } EVENT(handle, ALPM_EVENT_INTERCONFLICTS_DONE, NULL, NULL); - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_conflict_free); + alpm_list_free_inner(deps, (alpm_list_fn_free)alpm_conflict_free); alpm_list_free(deps); } @@ -628,7 +629,8 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) if(data) { *data = deps; } else { - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free_inner(deps, + (alpm_list_fn_free)alpm_depmissing_free); alpm_list_free(deps); } goto cleanup; @@ -1274,7 +1276,7 @@ int _alpm_sync_commit(alpm_handle_t *handle, alpm_list_t **data) if(data) { *data = conflict; } else { - alpm_list_free_inner(conflict, (alpm_list_fn_free)_alpm_fileconflict_free); + alpm_list_free_inner(conflict, (alpm_list_fn_free)alpm_fileconflict_free); alpm_list_free(conflict); } RET_ERR(handle, ALPM_ERR_FILE_CONFLICTS, -1); diff --git a/src/pacman/remove.c b/src/pacman/remove.c index 56b9333..00bd3b3 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -121,12 +121,13 @@ int pacman_remove(alpm_list_t *targets) char *depstring = alpm_dep_compute_string(miss->depend); colon_printf(_("%s: requires %s\n"), miss->target, depstring); free(depstring); + alpm_depmissing_free(miss); } break; default: break; } - FREELIST(data); + alpm_list_free(data); retval = 1; goto cleanup; } diff --git a/src/pacman/sync.c b/src/pacman/sync.c index f6ee57a..796dd0b 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -763,8 +763,9 @@ int sync_prepare_execute(void) switch(err) { case ALPM_ERR_PKG_INVALID_ARCH: for(i = data; i; i = alpm_list_next(i)) { - const char *pkg = i->data; + char *pkg = i->data; colon_printf(_("package %s does not have a valid architecture\n"), pkg); + free(pkg); } break; case ALPM_ERR_UNSATISFIED_DEPS: @@ -773,6 +774,7 @@ int sync_prepare_execute(void) char *depstring = alpm_dep_compute_string(miss->depend); colon_printf(_("%s: requires %s\n"), miss->target, depstring); free(depstring); + alpm_depmissing_free(miss); } break; case ALPM_ERR_CONFLICTING_DEPS: @@ -788,6 +790,7 @@ int sync_prepare_execute(void) conflict->package1, conflict->package2, reason); free(reason); } + alpm_conflict_free(conflict); } break; default: @@ -847,6 +850,7 @@ int sync_prepare_execute(void) conflict->target, conflict->file); break; } + alpm_fileconflict_free(conflict); } break; case ALPM_ERR_PKG_INVALID: @@ -854,8 +858,9 @@ int sync_prepare_execute(void) case ALPM_ERR_PKG_INVALID_SIG: case ALPM_ERR_DLT_INVALID: for(i = data; i; i = alpm_list_next(i)) { - const char *filename = i->data; + char *filename = i->data; printf(_("%s is invalid or corrupted\n"), filename); + free(filename); } break; default: @@ -869,9 +874,7 @@ int sync_prepare_execute(void) /* Step 4: release transaction resources */ cleanup: - if(data) { - FREELIST(data); - } + alpm_list_free(data); if(trans_release() == -1) { retval = 1; } -- 1.8.5.3
If the user opted not to remove the unresolvable packages from the transaction, the list was neither free'd nor saved to the transaction to be free'd in trans_release. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- This supersedes patch 1/8 (avoid double freeing unresovable packages) as well, which was only necessary because the previous version made it possible for packages to be in both trans->add and trans-unresolvable. lib/libalpm/sync.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index fb13324..c6c50a4 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -460,6 +460,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) /* pm_errno was set by resolvedeps, callback may have overwrote it */ handle->pm_errno = ALPM_ERR_UNSATISFIED_DEPS; alpm_list_free(resolved); + alpm_list_free(unresolvable); ret = -1; goto cleanup; } -- 1.8.5.3
On 07/02/14 11:49, Andrew Gregory wrote:
If the user opted not to remove the unresolvable packages from the transaction, the list was neither free'd nor saved to the transaction to be free'd in trans_release.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
This supersedes patch 1/8 (avoid double freeing unresovable packages) as well, which was only necessary because the previous version made it possible for packages to be in both trans->add and trans-unresolvable.
Much better! And I note all tests pass with --valgrind now. Allan
participants (2)
-
Allan McRae
-
Andrew Gregory