[pacman-dev] [PATCH v2] pacman/upgrade: Check malloc() return value
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
On 03/09/14 at 03:47pm, Sören Brinkmann wrote:
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 you move the malloc outside the loop and make it an array we have a better chance of hitting this early enough that we still have a little memory left for cleanup.
if(strstr(i->data, "://")) { char *str = alpm_fetch_pkgurl(config->handle, i->data); -- 1.9.0
Hi Andrew, On Mon, 2014-03-10 at 10:00AM -0400, Andrew Gregory wrote:
On 03/09/14 at 03:47pm, Sören Brinkmann wrote:
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 you move the malloc outside the loop and make it an array we have a better chance of hitting this early enough that we still have a little memory left for cleanup.
I think that is not done that easily. Actually not without some significant rewrite of this code. Those allocated rs go into an alpm_list_t which is eventually freed using FREELIST. Doing the memory management differently here is not that straight forward. Sören
On 03/10/14 at 07:32pm, Sören Brinkmann wrote:
Hi Andrew,
On Mon, 2014-03-10 at 10:00AM -0400, Andrew Gregory wrote:
On 03/09/14 at 03:47pm, Sören Brinkmann wrote:
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 you move the malloc outside the loop and make it an array we have a better chance of hitting this early enough that we still have a little memory left for cleanup.
I think that is not done that easily. Actually not without some significant rewrite of this code. Those allocated rs go into an alpm_list_t which is eventually freed using FREELIST. Doing the memory management differently here is not that straight forward.
Sören
The resulting list is only used in one place and isn't passed to any other functions. It would be trivial to convert it to a standard C array. Leaving it as-is with a list instead of an array leaves this function still vulnerable to a malloc failure from alpm_list_add. The only thing the list is used for is to set the siglevel, so you could even store the siglevels directly in the array instead of integer flags. apg
On Tue, 2014-03-11 at 09:57AM -0400, Andrew Gregory wrote:
On 03/10/14 at 07:32pm, Sören Brinkmann wrote:
Hi Andrew,
On Mon, 2014-03-10 at 10:00AM -0400, Andrew Gregory wrote:
On 03/09/14 at 03:47pm, Sören Brinkmann wrote:
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 you move the malloc outside the loop and make it an array we have a better chance of hitting this early enough that we still have a little memory left for cleanup.
I think that is not done that easily. Actually not without some significant rewrite of this code. Those allocated rs go into an alpm_list_t which is eventually freed using FREELIST. Doing the memory management differently here is not that straight forward.
Sören
The resulting list is only used in one place and isn't passed to any other functions. It would be trivial to convert it to a standard C array. Leaving it as-is with a list instead of an array leaves this function still vulnerable to a malloc failure from alpm_list_add. The only thing the list is used for is to set the siglevel, so you could even store the siglevels directly in the array instead of integer flags.
Honestly, memory management and convoluted error paths seem to be a general problem in this code base. I see what I can do. You're probably right and it's not as a big task as I originally thought to get things at least slightly improved. Sören
participants (2)
-
Andrew Gregory
-
Sören Brinkmann