lkml.org 
[lkml]   [2011]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] staging: altera-stapl: Fix memory leak of altera_init()
    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.
    \
     
     \ /
      Last update: 2011-05-31 01:23    [W:0.025 / U:31.192 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site