[pacman-dev] [PATCH] Fix some memleaks in alpm/add.c
From b9673a106b1f985cb81f620f394620e6eca88516 Mon Sep 17 00:00:00 2001 From: Nagy Gabor <ngaba@bibl.u-szeged.hu> Date: Tue, 15 Jul 2008 13:30:34 +0200 Subject: [PATCH] Fix some memleaks in alpm/add.c In case of error some allocated memory wasn't freed in commit_single_pkg. Note: The return value of this function is not used. Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu> --- lib/libalpm/add.c | 20 +++++++++++++------- 1 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 5bf3fcd..3b60fb3 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -689,7 +689,7 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, /* set up fake remove transaction */ int ret = upgrade_remove(oldpkg, newpkg, trans, db); if(ret != 0) { - return(ret); + goto cleanup; } } @@ -701,7 +701,9 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, _alpm_log(PM_LOG_DEBUG, "extracting files\n"); if ((archive = archive_read_new()) == NULL) { - RET_ERR(PM_ERR_LIBARCHIVE, -1); + pm_errno = PM_ERR_LIBARCHIVE; + ret = -1; + goto cleanup; } archive_read_support_compression_all(archive); @@ -710,7 +712,9 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, _alpm_log(PM_LOG_DEBUG, "archive: %s\n", newpkg->origin_data.file); if(archive_read_open_filename(archive, newpkg->origin_data.file, ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { - RET_ERR(PM_ERR_PKG_OPEN, -1); + pm_errno = PM_ERR_PKG_OPEN; + ret = -1; + goto cleanup; } /* save the cwd so we can restore it later */ @@ -772,7 +776,7 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, } if(errors) { - ret = 1; + ret = -1; if(is_upgrade) { _alpm_log(PM_LOG_ERROR, _("problem occurred while upgrading %s\n"), newpkg->name); @@ -798,7 +802,9 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, alpm_pkg_get_name(newpkg), alpm_pkg_get_version(newpkg)); alpm_logaction("error: could not update database entry %s-%s\n", alpm_pkg_get_name(newpkg), alpm_pkg_get_version(newpkg)); - RET_ERR(PM_ERR_DB_WRITE, -1); + pm_errno = PM_ERR_DB_WRITE; + ret = -1; + goto cleanup; } if(_alpm_db_add_pkgincache(db, newpkg) == -1) { @@ -833,9 +839,9 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, EVENT(trans, PM_TRANS_EVT_ADD_DONE, newpkg, oldpkg); } +cleanup: _alpm_pkg_free(oldpkg); - - return(0); + return(ret); } int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) -- 1.5.6.2
On Tue, Jul 15, 2008 at 6:36 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
From b9673a106b1f985cb81f620f394620e6eca88516 Mon Sep 17 00:00:00 2001 From: Nagy Gabor <ngaba@bibl.u-szeged.hu> Date: Tue, 15 Jul 2008 13:30:34 +0200 Subject: [PATCH] Fix some memleaks in alpm/add.c
In case of error some allocated memory wasn't freed in commit_single_pkg. Note: The return value of this function is not used.
Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu> ---
Thanks. Perhaps we should use the return value? -Dan
Idézés Dan McGee <dpmcgee@gmail.com>:
On Tue, Jul 15, 2008 at 6:36 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
From b9673a106b1f985cb81f620f394620e6eca88516 Mon Sep 17 00:00:00 2001 From: Nagy Gabor <ngaba@bibl.u-szeged.hu> Date: Tue, 15 Jul 2008 13:30:34 +0200 Subject: [PATCH] Fix some memleaks in alpm/add.c
In case of error some allocated memory wasn't freed in commit_single_pkg. Note: The return value of this function is not used.
Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu> ---
Thanks. Perhaps we should use the return value?
Well, now our add code won't stop in case of error just go ahead;-) As I guessed this behaviour change was introduced by commit 591bfabbd38bf4f8f209977f416a4e5fd3cc2baf, where we split the huge add_commit function. So to answer to your question, I think we should. Others? Bye ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
2008/7/16 Nagy Gabor <ngaba@bibl.u-szeged.hu>:
Idézés Dan McGee <dpmcgee@gmail.com>:
On Tue, Jul 15, 2008 at 6:36 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
From b9673a106b1f985cb81f620f394620e6eca88516 Mon Sep 17 00:00:00 2001 From: Nagy Gabor <ngaba@bibl.u-szeged.hu> Date: Tue, 15 Jul 2008 13:30:34 +0200 Subject: [PATCH] Fix some memleaks in alpm/add.c
In case of error some allocated memory wasn't freed in commit_single_pkg. Note: The return value of this function is not used.
Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu> ---
Thanks. Perhaps we should use the return value?
Well, now our add code won't stop in case of error just go ahead;-) As I guessed this behaviour change was introduced by commit 591bfabbd38bf4f8f209977f416a4e5fd3cc2baf, where we split the huge add_commit function. So to answer to your question, I think we should. Others?
There is no point discussing whether we should use it or not, what is important is to discuss *how* to use it. There are two main ways : 1) just printing a message : error, warning or just debug ? 2) stops here, and probably returning -1 . Should we run ldconfig before returning? The ldconfig question in case 2 also applies to the case where we cancel a transaction with ctrl+c (sate_interrupted).
The ldconfig question in case 2 also applies to the case where we cancel a transaction with ctrl+c (sate_interrupted).
Personally I would put ldconfig to trans.c (alpm_trans_commit?). Bye
Well, now our add code won't stop in case of error just go ahead;-) As I guessed this behaviour change was introduced by commit 591bfabbd38bf4f8f209977f416a4e5fd3cc2baf, where we split the huge add_commit function. So to answer to your question, I think we should. Others?
There is no point discussing whether we should use it or not, what is important is to discuss *how* to use it. There are two main ways : 1) just printing a message : error, warning or just debug ?
Most of them is printed via alpm_log in the split function.
2) stops here, and probably returning -1 . Should we run ldconfig before returning?
Before the mentioned commit, we followed 2) without ldconfig. Now we don't stop (and thus run ldconfig always).
The ldconfig question in case 2 also applies to the case where we cancel a transaction with ctrl+c (sate_interrupted).
I can cancel the transaction in the middle of a commit? (I mean "between" packages...) That's not good. If pacman upgraded only the half of the packages, inconsistent database is predicted. (Maybe I'm too strict here.) This is true for the original question, the ability to rollback the transaction would be the best there. Without rollback I have no clue... Maybe the current one is better. Others? Bye
participants (3)
-
Dan McGee
-
Nagy Gabor
-
Xavier