[pacman-dev] [PATCH] 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> --- src/pacman/upgrade.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 5416f6180b39..19aa17218ce4 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -51,6 +51,9 @@ int pacman_upgrade(alpm_list_t *targets) */ for(i = targets; i; i = alpm_list_next(i)) { int *r = malloc(sizeof(int)); + if(r == NULL) { + return 1; + } if(strstr(i->data, "://")) { char *str = alpm_fetch_pkgurl(config->handle, i->data); -- 1.9.0
On 07/03/14 16:24, 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> --- src/pacman/upgrade.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 5416f6180b39..19aa17218ce4 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -51,6 +51,9 @@ int pacman_upgrade(alpm_list_t *targets) */ for(i = targets; i; i = alpm_list_next(i)) { int *r = malloc(sizeof(int)); + if(r == NULL) { + return 1; + }
if(strstr(i->data, "://")) { char *str = alpm_fetch_pkgurl(config->handle, i->data);
Fine. Although if malloc of an int fails, I'm not sure we can do a lot! Allan
On Sat, 2014-03-08 at 05:02PM +1000, Allan McRae wrote:
On 07/03/14 16:24, 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> --- src/pacman/upgrade.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 5416f6180b39..19aa17218ce4 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -51,6 +51,9 @@ int pacman_upgrade(alpm_list_t *targets) */ for(i = targets; i; i = alpm_list_next(i)) { int *r = malloc(sizeof(int)); + if(r == NULL) { + return 1; + }
if(strstr(i->data, "://")) { char *str = alpm_fetch_pkgurl(config->handle, i->data);
Fine. Although if malloc of an int fails, I'm not sure we can do a lot!
Right, but bailing out gracefully is probably better than a segfault? I do not really mind. If you don't want it, don't apply it. But not checking the return value of malloc() is simply wrong, IMHO. Sören
On 08/03/14 17:07, Sören Brinkmann wrote:
On Sat, 2014-03-08 at 05:02PM +1000, Allan McRae wrote:
On 07/03/14 16:24, 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> --- src/pacman/upgrade.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 5416f6180b39..19aa17218ce4 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -51,6 +51,9 @@ int pacman_upgrade(alpm_list_t *targets) */ for(i = targets; i; i = alpm_list_next(i)) { int *r = malloc(sizeof(int)); + if(r == NULL) { + return 1; + }
if(strstr(i->data, "://")) { char *str = alpm_fetch_pkgurl(config->handle, i->data);
Fine. Although if malloc of an int fails, I'm not sure we can do a lot!
Right, but bailing out gracefully is probably better than a segfault? I do not really mind. If you don't want it, don't apply it. But not checking the return value of malloc() is simply wrong, IMHO.
Sören
I want to add something like this in the check: pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n")); otherwise we just drop out with no notification to the user. A segfault at least tells them something went wrong... However, I'm not sure we can achieve that when we fail malloc for an int. Allan
On Sat, 2014-03-08 at 05:29PM +1000, Allan McRae wrote:
On 08/03/14 17:07, Sören Brinkmann wrote:
On Sat, 2014-03-08 at 05:02PM +1000, Allan McRae wrote:
On 07/03/14 16:24, 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> --- src/pacman/upgrade.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 5416f6180b39..19aa17218ce4 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -51,6 +51,9 @@ int pacman_upgrade(alpm_list_t *targets) */ for(i = targets; i; i = alpm_list_next(i)) { int *r = malloc(sizeof(int)); + if(r == NULL) { + return 1; + }
if(strstr(i->data, "://")) { char *str = alpm_fetch_pkgurl(config->handle, i->data);
Fine. Although if malloc of an int fails, I'm not sure we can do a lot!
Right, but bailing out gracefully is probably better than a segfault? I do not really mind. If you don't want it, don't apply it. But not checking the return value of malloc() is simply wrong, IMHO.
Sören
I want to add something like this in the check:
pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n"));
otherwise we just drop out with no notification to the user. A segfault at least tells them something went wrong... However, I'm not sure we can achieve that when we fail malloc for an int.
Hmm. I guess you're right. It is highly unlikely to ever hit this error path. And if it ever really fails, chances are you are in really big trouble. So, the whole thing is probably questionable. If the malloc fails your whole system is probably failing and whether you check the return value (with or without printing a message) may be irrelevant. I'd assume your OS will already yell at you. I just couldn't pass this malloc with missing error handling without taking action. Anyway, no problem to re-spin the patch and add the printf statement, if you want that. Sören
participants (2)
-
Allan McRae
-
Sören Brinkmann