[pacman-dev] [PATCH v3 0/4] pacman/upgrade
So, Andrew urged me to dive a little deeper into this and here's the result. On top of the original fix for the potential NULL-pointer dereference there are three more patches now. Those address the memory management and error paths in the upgrade code. Sören Sören Brinkmann (4): pacman/upgrade: Check malloc() return value pacman/upgrade: Refactor memory management pacman/upgrade: Fix memory leaks pacman/upgrade: Bail early on errors src/pacman/upgrade.c | 52 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 20 deletions(-) -- 1.9.0
Check the return value of malloc() before dereferencing the returned pointer. Signed-off-by: Sören Brinkmann <soeren.brinkmann@gmail.com> --- v2: - add print statement to log the error --- src/pacman/upgrade.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 5416f6180b39..11da4dcb5e53 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -51,6 +51,10 @@ int pacman_upgrade(alpm_list_t *targets) */ for(i = targets; i; i = alpm_list_next(i)) { int *r = malloc(sizeof(int)); + if(r == NULL) { + pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n")); + return 1; + } if(strstr(i->data, "://")) { char *str = alpm_fetch_pkgurl(config->handle, i->data); -- 1.9.0
Refactor the upgrade routine to use an array that can be allocated in one step instead of an alpm_list that is gradually extended in loops. Signed-off-by: Sören Brinkmann <soeren.brinkmann@gmail.com> --- src/pacman/upgrade.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 11da4dcb5e53..e0cdf564971e 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -39,23 +39,25 @@ */ int pacman_upgrade(alpm_list_t *targets) { - int retval = 0; - alpm_list_t *i, *j, *remote = NULL; + int retval = 0, *file_is_remote; + alpm_list_t *i; + unsigned int n, num_targets; if(targets == NULL) { pm_printf(ALPM_LOG_ERROR, _("no targets specified (use -h for help)\n")); return 1; } - /* Check for URL targets and process them - */ - for(i = targets; i; i = alpm_list_next(i)) { - int *r = malloc(sizeof(int)); - if(r == NULL) { - pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n")); - return 1; - } + num_targets = alpm_list_count(targets); + /* Check for URL targets and process them */ + file_is_remote = malloc(num_targets * sizeof(*file_is_remote)); + if(file_is_remote == NULL) { + pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n")); + return 1; + } + + for(i = targets, n = 0; n < num_targets; i = alpm_list_next(i), n++) { if(strstr(i->data, "://")) { char *str = alpm_fetch_pkgurl(config->handle, i->data); if(str == NULL) { @@ -65,13 +67,11 @@ int pacman_upgrade(alpm_list_t *targets) } else { free(i->data); i->data = str; - *r = 1; + file_is_remote[n] = 1; } } else { - *r = 0; + file_is_remote[n] = 0; } - - remote = alpm_list_add(remote, r); } if(retval) { @@ -85,12 +85,12 @@ int pacman_upgrade(alpm_list_t *targets) printf(_("loading packages...\n")); /* add targets to the created transaction */ - for(i = targets, j = remote; i; i = alpm_list_next(i), j = alpm_list_next(j)) { + for(i = targets, n = 0; n < num_targets; i = alpm_list_next(i), n++) { const char *targ = i->data; alpm_pkg_t *pkg; alpm_siglevel_t level; - if(*(int *)j->data) { + if(file_is_remote[n]) { level = alpm_option_get_remote_file_siglevel(config->handle); } else { level = alpm_option_get_local_file_siglevel(config->handle); @@ -112,7 +112,7 @@ int pacman_upgrade(alpm_list_t *targets) config->explicit_adds = alpm_list_add(config->explicit_adds, pkg); } - FREELIST(remote); + free(file_is_remote); if(retval) { trans_release(); -- 1.9.0
On 03/11/14 at 07:29pm, Sören Brinkmann wrote:
Refactor the upgrade routine to use an array that can be allocated in one step instead of an alpm_list that is gradually extended in loops.
Signed-off-by: Sören Brinkmann <soeren.brinkmann@gmail.com> ---
Looks good; I just have a few small style nitpicks.
src/pacman/upgrade.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 11da4dcb5e53..e0cdf564971e 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -39,23 +39,25 @@ */ int pacman_upgrade(alpm_list_t *targets) { - int retval = 0; - alpm_list_t *i, *j, *remote = NULL; + int retval = 0, *file_is_remote; + alpm_list_t *i; + unsigned int n, num_targets;
if(targets == NULL) { pm_printf(ALPM_LOG_ERROR, _("no targets specified (use -h for help)\n")); return 1; }
- /* Check for URL targets and process them - */ - for(i = targets; i; i = alpm_list_next(i)) { - int *r = malloc(sizeof(int)); - if(r == NULL) { - pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n")); - return 1; - } + num_targets = alpm_list_count(targets);
+ /* Check for URL targets and process them */ + file_is_remote = malloc(num_targets * sizeof(*file_is_remote));
Our style guidelines require that sizeof() be used with types, not values.
+ if(file_is_remote == NULL) { + pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n")); + return 1; + } + + for(i = targets, n = 0; n < num_targets; i = alpm_list_next(i), n++) {
I would prefer to use i as the conditional check here instead of n since it is such a common idiom throughout our codebase.
if(strstr(i->data, "://")) { char *str = alpm_fetch_pkgurl(config->handle, i->data); if(str == NULL) { @@ -65,13 +67,11 @@ int pacman_upgrade(alpm_list_t *targets) } else { free(i->data); i->data = str; - *r = 1; + file_is_remote[n] = 1; } } else { - *r = 0; + file_is_remote[n] = 0; } - - remote = alpm_list_add(remote, r); }
if(retval) { @@ -85,12 +85,12 @@ int pacman_upgrade(alpm_list_t *targets)
printf(_("loading packages...\n")); /* add targets to the created transaction */ - for(i = targets, j = remote; i; i = alpm_list_next(i), j = alpm_list_next(j)) { + for(i = targets, n = 0; n < num_targets; i = alpm_list_next(i), n++) {
Same as above.
const char *targ = i->data; alpm_pkg_t *pkg; alpm_siglevel_t level;
- if(*(int *)j->data) { + if(file_is_remote[n]) { level = alpm_option_get_remote_file_siglevel(config->handle); } else { level = alpm_option_get_local_file_siglevel(config->handle); @@ -112,7 +112,7 @@ int pacman_upgrade(alpm_list_t *targets) config->explicit_adds = alpm_list_add(config->explicit_adds, pkg); }
- FREELIST(remote); + free(file_is_remote);
if(retval) { trans_release(); -- 1.9.0
On 25/03/14 00:10, Andrew Gregory wrote:
On 03/11/14 at 07:29pm, Sören Brinkmann wrote:
Refactor the upgrade routine to use an array that can be allocated in one step instead of an alpm_list that is gradually extended in loops.
Signed-off-by: Sören Brinkmann <soeren.brinkmann@gmail.com> ---
Looks good; I just have a few small style nitpicks.
I made these minor changes on my working branch. A
Make sure allocated memory is freed before returning. Signed-off-by: Sören Brinkmann <soeren.brinkmann@gmail.com> --- src/pacman/upgrade.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index e0cdf564971e..5ad08216ff06 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -75,12 +75,13 @@ int pacman_upgrade(alpm_list_t *targets) } if(retval) { - return retval; + goto fail_free; } /* Step 1: create a new transaction */ if(trans_init(config->flags, 1) == -1) { - return 1; + retval = 1; + goto fail_free; } printf(_("loading packages...\n")); @@ -112,15 +113,21 @@ int pacman_upgrade(alpm_list_t *targets) config->explicit_adds = alpm_list_add(config->explicit_adds, pkg); } - free(file_is_remote); - if(retval) { - trans_release(); - return retval; + goto fail_release; } + free(file_is_remote); + /* now that targets are resolved, we can hand it all off to the sync code */ return sync_prepare_execute(); + +fail_release: + trans_release(); +fail_free: + free(file_is_remote); + + return retval; } /* vim: set noet: */ -- 1.9.0
When an error is detected in a loop, immediately bail out and report the error. Signed-off-by: Sören Brinkmann <soeren.brinkmann@gmail.com> --- src/pacman/upgrade.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 5ad08216ff06..b85b0c4a4ee1 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -64,6 +64,7 @@ int pacman_upgrade(alpm_list_t *targets) pm_printf(ALPM_LOG_ERROR, "'%s': %s\n", (char *)i->data, alpm_strerror(alpm_errno(config->handle))); retval = 1; + break; } else { free(i->data); i->data = str; @@ -101,14 +102,14 @@ int pacman_upgrade(alpm_list_t *targets) pm_printf(ALPM_LOG_ERROR, "'%s': %s\n", targ, alpm_strerror(alpm_errno(config->handle))); retval = 1; - continue; + break; } if(alpm_add_pkg(config->handle, pkg) == -1) { pm_printf(ALPM_LOG_ERROR, "'%s': %s\n", targ, alpm_strerror(alpm_errno(config->handle))); alpm_pkg_free(pkg); retval = 1; - continue; + break; } config->explicit_adds = alpm_list_add(config->explicit_adds, pkg); } -- 1.9.0
On 03/11/14 at 07:29pm, Sören Brinkmann wrote:
When an error is detected in a loop, immediately bail out and report the error.
Signed-off-by: Sören Brinkmann <soeren.brinkmann@gmail.com> --- src/pacman/upgrade.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
-1. We use retval/continue so that we can give the user errors for all targets at once instead of making them repeatedly run pacman. apg
diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 5ad08216ff06..b85b0c4a4ee1 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -64,6 +64,7 @@ int pacman_upgrade(alpm_list_t *targets) pm_printf(ALPM_LOG_ERROR, "'%s': %s\n", (char *)i->data, alpm_strerror(alpm_errno(config->handle))); retval = 1; + break; } else { free(i->data); i->data = str; @@ -101,14 +102,14 @@ int pacman_upgrade(alpm_list_t *targets) pm_printf(ALPM_LOG_ERROR, "'%s': %s\n", targ, alpm_strerror(alpm_errno(config->handle))); retval = 1; - continue; + break; } if(alpm_add_pkg(config->handle, pkg) == -1) { pm_printf(ALPM_LOG_ERROR, "'%s': %s\n", targ, alpm_strerror(alpm_errno(config->handle))); alpm_pkg_free(pkg); retval = 1; - continue; + break; } config->explicit_adds = alpm_list_add(config->explicit_adds, pkg); } -- 1.9.0
On Wed, 2014-03-12 at 12:06AM -0400, Andrew Gregory wrote:
On 03/11/14 at 07:29pm, Sören Brinkmann wrote:
When an error is detected in a loop, immediately bail out and report the error.
Signed-off-by: Sören Brinkmann <soeren.brinkmann@gmail.com> --- src/pacman/upgrade.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
-1. We use retval/continue so that we can give the user errors for all targets at once instead of making them repeatedly run pacman.
Makes sense. This should be dropped. Thanks, Sören
On 12/03/14 12:29, Sören Brinkmann wrote:
So, Andrew urged me to dive a little deeper into this and here's the result. On top of the original fix for the potential NULL-pointer dereference there are three more patches now. Those address the memory management and error paths in the upgrade code.
Sören
Sören Brinkmann (4): pacman/upgrade: Check malloc() return value pacman/upgrade: Refactor memory management pacman/upgrade: Fix memory leaks pacman/upgrade: Bail early on errors
src/pacman/upgrade.c | 52 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 20 deletions(-)
I have only had a quick look. But patch #2 removes changes made in patch #1 and patch #3 corrects a memory leak made in patch #2. That looks like it should be merged into a single patch. Allan
On Wed, 2014-03-12 at 01:16PM +1000, Allan McRae wrote:
On 12/03/14 12:29, Sören Brinkmann wrote:
So, Andrew urged me to dive a little deeper into this and here's the result. On top of the original fix for the potential NULL-pointer dereference there are three more patches now. Those address the memory management and error paths in the upgrade code.
Sören
Sören Brinkmann (4): pacman/upgrade: Check malloc() return value pacman/upgrade: Refactor memory management pacman/upgrade: Fix memory leaks pacman/upgrade: Bail early on errors
src/pacman/upgrade.c | 52 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 20 deletions(-)
I have only had a quick look. But patch #2 removes changes made in patch #1
I kept fixing the NULL-pointer dereference and refactoring separated. If the refactoring turns out to be buggy it could be reverted without re-intorducing the NULL-pointer dereference.
and patch #3 corrects a memory leak made in patch #2.
Not fully true. Before, the memory management was just more subtle, hidden in alpm_list_add and FREELIST, the leak was already there.
That looks like it should be merged into a single patch.
Feel free to squash things as you think it's appropriate. Sören
On 12/03/14 14:03, Sören Brinkmann wrote:
On Wed, 2014-03-12 at 01:16PM +1000, Allan McRae wrote:
On 12/03/14 12:29, Sören Brinkmann wrote:
So, Andrew urged me to dive a little deeper into this and here's the result. On top of the original fix for the potential NULL-pointer dereference there are three more patches now. Those address the memory management and error paths in the upgrade code.
Sören
Sören Brinkmann (4): pacman/upgrade: Check malloc() return value pacman/upgrade: Refactor memory management pacman/upgrade: Fix memory leaks pacman/upgrade: Bail early on errors
src/pacman/upgrade.c | 52 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 20 deletions(-)
I have only had a quick look. But patch #2 removes changes made in patch #1
I kept fixing the NULL-pointer dereference and refactoring separated. If the refactoring turns out to be buggy it could be reverted without re-intorducing the NULL-pointer dereference.
Far enough. I'm always requesting patches are broken into smaller chunks, so I will go with that.
and patch #3 corrects a memory leak made in patch #2.
Not fully true. Before, the memory management was just more subtle, hidden in alpm_list_add and FREELIST, the leak was already there.
You are correct. I blame grant and fellowship writing season for destroying my brain. A
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Sören Brinkmann