* [PATCH] staging: altera-stapl: Fix memory leak of altera_init() @ 2011-05-30 20:45 Andre Bartke 2011-05-30 23:11 ` Peter Hüwe 0 siblings, 1 reply; 3+ messages in thread From: Andre Bartke @ 2011-05-30 20:45 UTC (permalink / raw) To: mchehab; +Cc: devel, linux-kernel, Andre Bartke In case kzalloc() fails the second or third time we should free the previous allocated resources. Signed-off-by: Andre Bartke <andre.bartke@gmail.com> --- drivers/staging/altera-stapl/altera.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/staging/altera-stapl/altera.c b/drivers/staging/altera-stapl/altera.c index 05aad35..56ba557 100644 --- a/drivers/staging/altera-stapl/altera.c +++ b/drivers/staging/altera-stapl/altera.c @@ -2435,11 +2435,16 @@ int altera_init(struct altera_config *config, const struct firmware *fw) if (!key) return -ENOMEM; value = kzalloc(257 * sizeof(char), GFP_KERNEL); - if (!value) + if (!value) { + kfree(key); return -ENOMEM; + } astate = kzalloc(sizeof(struct altera_state), GFP_KERNEL); - if (!astate) + if (!astate) { + kfree(key); + kfree(value); return -ENOMEM; + } astate->config = config; if (!astate->config->jtag_io) { -- 1.7.5.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] staging: altera-stapl: Fix memory leak of altera_init() 2011-05-30 20:45 [PATCH] staging: altera-stapl: Fix memory leak of altera_init() Andre Bartke @ 2011-05-30 23:11 ` Peter Hüwe 2011-05-30 23:13 ` Jesper Juhl 0 siblings, 1 reply; 3+ messages in thread From: Peter Hüwe @ 2011-05-30 23:11 UTC (permalink / raw) To: Andre Bartke; +Cc: devel, mchehab, devel, Andre Bartke, linux-kernel Am Montag 30 Mai 2011, 22:45:04 schrieb Andre Bartke: > In case kzalloc() fails the second or third time > we should free the previous allocated resources. Good catch! Personally I prefer putting the cleanup logic to the bottom, maybe like this - but that's just personal preference. >From 1a13a1d7a2bad26f050ecc342741b6c07cac2b8a Mon Sep 17 00:00:00 2001 From: Peter Huewe <peterhuewe@gmx.de> Date: Tue, 31 May 2011 00:54:27 +0200 Subject: [PATCH] staging: altera-stapl: Fix memory leak of altera_init() In case kzalloc() fails the second or third time we should free the previous allocated resources. In order to keep one return point and to keep the cleanup code to one place, some reordering was necessary. Also while at it, removed the *sizeof(char) - to quote Linus: "" Also removed the silly "* sizeof(u8)". If that isn't 1, we have way deeper problems than a simple multiplication can fix. """ Reported-by: Andre Bartke <andre.bartke@gmail.com> Signed-off-by: Peter Huewe <peterhuewe@gmx.de> --- drivers/staging/altera-stapl/altera.c | 33 +++++++++++++++++++++------------ 1 files changed, 21 insertions(+), 12 deletions(-) diff --git a/drivers/staging/altera-stapl/altera.c b/drivers/staging/altera-stapl/altera.c index 05aad35..09392ce 100644 --- a/drivers/staging/altera-stapl/altera.c +++ b/drivers/staging/altera-stapl/altera.c @@ -2430,16 +2430,23 @@ int altera_init(struct altera_config *config, const struct firmware *fw) int index = 0; s32 offset = 0L; s32 error_address = 0L; + int retval = 0; - key = kzalloc(33 * sizeof(char), GFP_KERNEL); - if (!key) - return -ENOMEM; - value = kzalloc(257 * sizeof(char), GFP_KERNEL); - if (!value) - return -ENOMEM; + key = kzalloc(33, GFP_KERNEL); + if (!key) { + retval = -ENOMEM; + goto out; + } + value = kzalloc(257, GFP_KERNEL); + if (!value) { + retval = -ENOMEM; + goto free_key; + } astate = kzalloc(sizeof(struct altera_state), GFP_KERNEL); - if (!astate) - return -ENOMEM; + if (!astate) { + retval = -ENOMEM; + goto free_value; + } astate->config = config; if (!astate->config->jtag_io) { @@ -2518,10 +2525,12 @@ int altera_init(struct altera_config *config, const struct firmware *fw) } else if (exec_result) printk(KERN_ERR "%s: error %d\n", __func__, exec_result); - kfree(key); - kfree(value); kfree(astate); - - return 0; +free_value: + kfree(value); +free_key: + kfree(key); +out: + return retval; } EXPORT_SYMBOL(altera_init); -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] staging: altera-stapl: Fix memory leak of altera_init() 2011-05-30 23:11 ` Peter Hüwe @ 2011-05-30 23:13 ` Jesper Juhl 0 siblings, 0 replies; 3+ messages in thread From: Jesper Juhl @ 2011-05-30 23:13 UTC (permalink / raw) To: Peter Hüwe Cc: Andre Bartke, devel, mchehab, devel, Andre Bartke, linux-kernel [-- Attachment #1: Type: TEXT/PLAIN, Size: 3574 bytes --] On Tue, 31 May 2011, Peter Hüwe wrote: > Am Montag 30 Mai 2011, 22:45:04 schrieb Andre Bartke: > > In case kzalloc() fails the second or third time > > we should free the previous allocated resources. > > Good catch! > Personally I prefer putting the cleanup logic to the bottom, maybe like this - but that's just personal preference. > Another alternative would be something like this: key = kzalloc(33 * sizeof(char), GFP_KERNEL); value = kzalloc(257 * sizeof(char), GFP_KERNEL); astate = kzalloc(sizeof(struct altera_state), GFP_KERNEL); if (!astate || !value || ! key) { kfree(astate); kfree(value); kfree(key); return -ENOMEM; } Ohh and btw, in your suggestion (below), why don't you initialize 'retval' to -ENOMEM and then set it to zero once after all the allocations have completed? That would remove the need to set it to -ENOMEM 3 times before the goto's... Sure, you are seting it 3 times in error paths and my suggestion would introduce a retval=0; in the normal path, but the code would be shorter and more readable IMHO. /Jesper Juhl > >From 1a13a1d7a2bad26f050ecc342741b6c07cac2b8a Mon Sep 17 00:00:00 2001 > From: Peter Huewe <peterhuewe@gmx.de> > Date: Tue, 31 May 2011 00:54:27 +0200 > Subject: [PATCH] staging: altera-stapl: Fix memory leak of altera_init() > > In case kzalloc() fails the second or third time > we should free the previous allocated resources. > In order to keep one return point and to keep the cleanup code to one > place, some reordering was necessary. > > Also while at it, removed the *sizeof(char) - to quote Linus: > "" Also removed the silly "* sizeof(u8)". If that isn't 1, we have way > deeper problems than a simple multiplication can fix. """ > > Reported-by: Andre Bartke <andre.bartke@gmail.com> > Signed-off-by: Peter Huewe <peterhuewe@gmx.de> > --- > drivers/staging/altera-stapl/altera.c | 33 +++++++++++++++++++++------------ > 1 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/altera-stapl/altera.c b/drivers/staging/altera-stapl/altera.c > index 05aad35..09392ce 100644 > --- a/drivers/staging/altera-stapl/altera.c > +++ b/drivers/staging/altera-stapl/altera.c > @@ -2430,16 +2430,23 @@ int altera_init(struct altera_config *config, const struct firmware *fw) > int index = 0; > s32 offset = 0L; > s32 error_address = 0L; > + int retval = 0; > > - key = kzalloc(33 * sizeof(char), GFP_KERNEL); > - if (!key) > - return -ENOMEM; > - value = kzalloc(257 * sizeof(char), GFP_KERNEL); > - if (!value) > - return -ENOMEM; > + key = kzalloc(33, GFP_KERNEL); > + if (!key) { > + retval = -ENOMEM; > + goto out; > + } > + value = kzalloc(257, GFP_KERNEL); > + if (!value) { > + retval = -ENOMEM; > + goto free_key; > + } > astate = kzalloc(sizeof(struct altera_state), GFP_KERNEL); > - if (!astate) > - return -ENOMEM; > + if (!astate) { > + retval = -ENOMEM; > + goto free_value; > + } > > astate->config = config; > if (!astate->config->jtag_io) { > @@ -2518,10 +2525,12 @@ int altera_init(struct altera_config *config, const struct firmware *fw) > } else if (exec_result) > printk(KERN_ERR "%s: error %d\n", __func__, exec_result); > > - kfree(key); > - kfree(value); > kfree(astate); > - > - return 0; > +free_value: > + kfree(value); > +free_key: > + kfree(key); > +out: > + return retval; > } > EXPORT_SYMBOL(altera_init); > -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-05-30 23:20 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-05-30 20:45 [PATCH] staging: altera-stapl: Fix memory leak of altera_init() Andre Bartke 2011-05-30 23:11 ` Peter Hüwe 2011-05-30 23:13 ` Jesper Juhl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox