possible memory leak in commands/nand.c?

possible memory leak in commands/nand.c?

[Robert P. J. Day]

  once again, perhaps i'm just misreading this but consider this code
from commands/nand.c, noting the two early calls to asprintf():

===== begin =====

        bb = xzalloc(sizeof(*bb));
        bb->devname = asprintf("/dev/%s", basename(path));
        if (name)
                bb->cdev.name = strdup(name);
        else
                bb->cdev.name = asprintf("%s.bb", basename(path));

        ret = stat(bb->devname, &s);
        if (ret)
                goto free_out;

        bb->raw_size = s.st_size;

        bb->fd = open(bb->devname, O_RDWR);
        if (bb->fd < 0) {
                ret = -ENODEV;
                goto free_out;
        }

        ret = ioctl(bb->fd, MEMGETINFO, &bb->info);
        if (ret)
                goto free_out;

        nand_bb_calc_size(bb);
        bb->cdev.ops = &nand_bb_ops;
        bb->cdev.priv = bb;

        devfs_create(&bb->cdev);

        return 0;

free_out:
        free(bb);
        return ret;

===== end =====

  if something in the latter part of that code fails and control jumps
to the label "free_out", won't this code fail to free() the memory
allocated in the two asprintf() calls?  isn't the programmer
explicitly required to free memory allocated with asprintf() or
vasprintf() calls?

rday
--

========================================================================
Robert P. J. Day                               Waterloo, Ontario, CANADA

            Linux Consulting, Training and Kernel Pedantry.

Web page:                                          http://crashcourse.ca
Twitter:                                       http://twitter.com/rpjday
========================================================================

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: possible memory leak in commands/nand.c?

[Sascha Hauer]

On Sun, Dec 20, 2009 at 02:33:11PM -0500, Robert P. J. Day wrote:
> 
>   once again, perhaps i'm just misreading this but consider this code
> from commands/nand.c, noting the two early calls to asprintf():
> 
> ===== begin =====
> 
>         bb = xzalloc(sizeof(*bb));
>         bb->devname = asprintf("/dev/%s", basename(path));
>         if (name)
>                 bb->cdev.name = strdup(name);
>         else
>                 bb->cdev.name = asprintf("%s.bb", basename(path));
> 
>         ret = stat(bb->devname, &s);
>         if (ret)
>                 goto free_out;
> 
>         bb->raw_size = s.st_size;
> 
>         bb->fd = open(bb->devname, O_RDWR);
>         if (bb->fd < 0) {
>                 ret = -ENODEV;
>                 goto free_out;
>         }
> 
>         ret = ioctl(bb->fd, MEMGETINFO, &bb->info);
>         if (ret)
>                 goto free_out;
> 
>         nand_bb_calc_size(bb);
>         bb->cdev.ops = &nand_bb_ops;
>         bb->cdev.priv = bb;
> 
>         devfs_create(&bb->cdev);
> 
>         return 0;
> 
> free_out:
>         free(bb);
>         return ret;
> 
> ===== end =====
> 
>   if something in the latter part of that code fails and control jumps
> to the label "free_out", won't this code fail to free() the memory
> allocated in the two asprintf() calls?  isn't the programmer
> explicitly required to free memory allocated with asprintf() or
> vasprintf() calls?

Yes, indeed, that's a memory hole here. The following should fix this.
Thanks for noting.

Sascha


From 4e4b03cd61808383a98cb1d10a47025e1909e0bd Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 21 Dec 2009 09:41:52 +0100
Subject: [PATCH] commands/nand.c: Fix memory hole

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 commands/nand.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/commands/nand.c b/commands/nand.c
index cbf1058..55b89af 100644
--- a/commands/nand.c
+++ b/commands/nand.c
@@ -224,31 +224,37 @@ static struct file_operations nand_bb_ops = {
 int dev_add_bb_dev(char *path, const char *name)
 {
 	struct nand_bb *bb;
-	int ret;
+	int ret = -ENOMEM;
 	struct stat s;
 
 	bb = xzalloc(sizeof(*bb));
 	bb->devname = asprintf("/dev/%s", basename(path));
+	if (!bb->devname)
+		goto out1;
+
 	if (name)
 		bb->cdev.name = strdup(name);
 	else
 		bb->cdev.name = asprintf("%s.bb", basename(path));
 
+	if (!bb->cdev.name)
+		goto out2;
+
 	ret = stat(bb->devname, &s);
 	if (ret)
-		goto free_out;
+		goto out3;
 
 	bb->raw_size = s.st_size;
 
 	bb->fd = open(bb->devname, O_RDWR);
 	if (bb->fd < 0) {
 		ret = -ENODEV;
-		goto free_out;
+		goto out3;
 	}
 
 	ret = ioctl(bb->fd, MEMGETINFO, &bb->info);
 	if (ret)
-		goto free_out;
+		goto out4;
 
 	nand_bb_calc_size(bb);
 	bb->cdev.ops = &nand_bb_ops;
@@ -258,7 +264,13 @@ int dev_add_bb_dev(char *path, const char *name)
 
 	return 0;
 
-free_out:
+out4:
+	close(bb->fd);
+out3:
+	free(bb->cdev.name);
+out2:
+	free(bb->devname);
+out1:
 	free(bb);
 	return ret;
 }
-- 
1.6.5.2


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: possible memory leak in commands/nand.c?

[Robert P. J. Day]

On Mon, 21 Dec 2009, Sascha Hauer wrote:

... snip ...

> Yes, indeed, that's a memory hole here. The following should fix
> this. Thanks for noting.
>
> Sascha
>
>
> >From 4e4b03cd61808383a98cb1d10a47025e1909e0bd Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Mon, 21 Dec 2009 09:41:52 +0100
> Subject: [PATCH] commands/nand.c: Fix memory hole
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  commands/nand.c |   22 +++++++++++++++++-----
>  1 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/commands/nand.c b/commands/nand.c
> index cbf1058..55b89af 100644
> --- a/commands/nand.c
> +++ b/commands/nand.c
> @@ -224,31 +224,37 @@ static struct file_operations nand_bb_ops = {
>  int dev_add_bb_dev(char *path, const char *name)
>  {
>  	struct nand_bb *bb;
> -	int ret;
> +	int ret = -ENOMEM;
>  	struct stat s;
>
>  	bb = xzalloc(sizeof(*bb));
>  	bb->devname = asprintf("/dev/%s", basename(path));
> +	if (!bb->devname)
> +		goto out1;
> +
>  	if (name)
>  		bb->cdev.name = strdup(name);
>  	else
>  		bb->cdev.name = asprintf("%s.bb", basename(path));
>
> +	if (!bb->cdev.name)
> +		goto out2;
> +
>  	ret = stat(bb->devname, &s);
>  	if (ret)
> -		goto free_out;
> +		goto out3;
>
>  	bb->raw_size = s.st_size;
>
>  	bb->fd = open(bb->devname, O_RDWR);
>  	if (bb->fd < 0) {
>  		ret = -ENODEV;
> -		goto free_out;
> +		goto out3;
>  	}
>
>  	ret = ioctl(bb->fd, MEMGETINFO, &bb->info);
>  	if (ret)
> -		goto free_out;
> +		goto out4;
>
>  	nand_bb_calc_size(bb);
>  	bb->cdev.ops = &nand_bb_ops;
> @@ -258,7 +264,13 @@ int dev_add_bb_dev(char *path, const char *name)
>
>  	return 0;
>
> -free_out:
> +out4:
> +	close(bb->fd);
> +out3:
> +	free(bb->cdev.name);
> +out2:
> +	free(bb->devname);
> +out1:
>  	free(bb);
>  	return ret;
>  }

  i'm not sure this required distinguishing between every one of those
cases since the initial space was allocated with xzalloc(),
guaranteeing it would be zero-filled, and freeing a NULL pointer is
supposed to be a no-op.

  so it would have been simpler to just

  free(bb->devname);		# might be NULL, no problem
  free(bb->cdev.name);		# same here
  free(bb);

but, yes, the above will work.

  there's another memory leak i've found, i'll submit a patch for it,
for no other reason than i feel like getting a few patches with my
name on it into the barebox git log. :-)

rday
--


========================================================================
Robert P. J. Day                               Waterloo, Ontario, CANADA

            Linux Consulting, Training and Kernel Pedantry.

Web page:                                          http://crashcourse.ca
Twitter:                                       http://twitter.com/rpjday
========================================================================

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: possible memory leak in commands/nand.c?

[Sascha Hauer]

On Mon, Dec 21, 2009 at 04:17:29AM -0500, Robert P. J. Day wrote:
> On Mon, 21 Dec 2009, Sascha Hauer wrote:
> 
> ... snip ...
> 
> > Yes, indeed, that's a memory hole here. The following should fix
> > this. Thanks for noting.
> >
> > Sascha
> >
> >
> > >From 4e4b03cd61808383a98cb1d10a47025e1909e0bd Mon Sep 17 00:00:00 2001
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Date: Mon, 21 Dec 2009 09:41:52 +0100
> > Subject: [PATCH] commands/nand.c: Fix memory hole
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  commands/nand.c |   22 +++++++++++++++++-----
> >  1 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/commands/nand.c b/commands/nand.c
> > index cbf1058..55b89af 100644
> > --- a/commands/nand.c
> > +++ b/commands/nand.c
> > @@ -224,31 +224,37 @@ static struct file_operations nand_bb_ops = {
> >  int dev_add_bb_dev(char *path, const char *name)
> >  {
> >  	struct nand_bb *bb;
> > -	int ret;
> > +	int ret = -ENOMEM;
> >  	struct stat s;
> >
> >  	bb = xzalloc(sizeof(*bb));
> >  	bb->devname = asprintf("/dev/%s", basename(path));
> > +	if (!bb->devname)
> > +		goto out1;
> > +
> >  	if (name)
> >  		bb->cdev.name = strdup(name);
> >  	else
> >  		bb->cdev.name = asprintf("%s.bb", basename(path));
> >
> > +	if (!bb->cdev.name)
> > +		goto out2;
> > +
> >  	ret = stat(bb->devname, &s);
> >  	if (ret)
> > -		goto free_out;
> > +		goto out3;
> >
> >  	bb->raw_size = s.st_size;
> >
> >  	bb->fd = open(bb->devname, O_RDWR);
> >  	if (bb->fd < 0) {
> >  		ret = -ENODEV;
> > -		goto free_out;
> > +		goto out3;
> >  	}
> >
> >  	ret = ioctl(bb->fd, MEMGETINFO, &bb->info);
> >  	if (ret)
> > -		goto free_out;
> > +		goto out4;
> >
> >  	nand_bb_calc_size(bb);
> >  	bb->cdev.ops = &nand_bb_ops;
> > @@ -258,7 +264,13 @@ int dev_add_bb_dev(char *path, const char *name)
> >
> >  	return 0;
> >
> > -free_out:
> > +out4:
> > +	close(bb->fd);
> > +out3:
> > +	free(bb->cdev.name);
> > +out2:
> > +	free(bb->devname);
> > +out1:
> >  	free(bb);
> >  	return ret;
> >  }
> 
>   i'm not sure this required distinguishing between every one of those
> cases since the initial space was allocated with xzalloc(),
> guaranteeing it would be zero-filled, and freeing a NULL pointer is
> supposed to be a no-op.
> 
>   so it would have been simpler to just
> 
>   free(bb->devname);		# might be NULL, no problem
>   free(bb->cdev.name);		# same here
>   free(bb);

Yes, you're right. OTOH we probably do not save anything by removing
the different jump labels.

Sascha

> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox