Re: [CHECKER] 32 Memory Leaks on Error Paths

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Jörn Engel]

On Mon, 15 September 2003 21:35:46 -0700, David Yu Chen wrote:
> 
> [FILE:  2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c]
> [FUNC:  cfi_staa_setup]
> [LINES: 191-211]
> [VAR:   mtd]
>  186:	struct mtd_info *mtd;
>  187:	unsigned long offset = 0;
>  188:	int i,j;
>  189:	unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
>  190:
> START -->
>  191:	mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
>  192:	//printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
>  193:
>  194:	if (!mtd) {
>  195:		printk(KERN_ERR "Failed to allocate memory for MTD device\n");
>  196:		kfree(cfi->cmdset_priv);
>         ... DELETED 9 lines ...
>  206:	mtd->eraseregions = kmalloc(sizeof(struct mtd_erase_region_info) 
>  207:			* mtd->numeraseregions, GFP_KERNEL);
>  208:	if (!mtd->eraseregions) { 
>  209:		printk(KERN_ERR "Failed to allocate memory for MTD erase region info\n");
>  210:		kfree(cfi->cmdset_priv);
> END -->
>  211:		return NULL;
>  212:	}
>  213:	
>  214:	for (i=0; i<cfi->cfiq->NumEraseRegions; i++) {
>  215:		unsigned long ernum, ersize;
>  216:		ersize = ((cfi->cfiq->EraseRegionInfo[i] >> 8) & ~0xff) * cfi->interleave;

Valid.

> [FILE:  2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c]
> [FUNC:  cfi_staa_setup]
> [LINES: 191-235]
> [VAR:   mtd]
>  186:	struct mtd_info *mtd;
>  187:	unsigned long offset = 0;
>  188:	int i,j;
>  189:	unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
>  190:
> START -->
>  191:	mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
>  192:	//printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
>  193:
>  194:	if (!mtd) {
>  195:		printk(KERN_ERR "Failed to allocate memory for MTD device\n");
>  196:		kfree(cfi->cmdset_priv);
>         ... DELETED 33 lines ...
>  230:		if (offset != devsize) {
>  231:			/* Argh */
>  232:			printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize);
>  233:			kfree(mtd->eraseregions);
>  234:			kfree(cfi->cmdset_priv);
> END -->
>  235:			return NULL;
>  236:		}
>  237:
>  238:		for (i=0; i<mtd->numeraseregions;i++){
>  239:			printk(KERN_DEBUG "%d: offset=0x%x,size=0x%x,blocks=%d\n",
>  240:			       i,mtd->eraseregions[i].offset,

Also valid.

> looks like checking for mtdblks instead of mtdblk
> [FILE:  2.6.0-test5/drivers/mtd/mtdblock.c]
> [FUNC:  mtdblock_open]
> [LINES: 277-279]
> [VAR:   mtdblk]
>  272:		mtdblks[dev]->count++;
>  273:		return 0;
>  274:	}
>  275:	
>  276:	/* OK, it's not open. Create cache info for it */
> START -->
>  277:	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
>  278:	if (!mtdblks)
> END -->
>  279:		return -ENOMEM;
>  280:
>  281:	memset(mtdblk, 0, sizeof(*mtdblk));
>  282:	mtdblk->count = 1;
>  283:	mtdblk->mtd = mtd;
>  284:

Invalid.  This is quite an obvious false positive, at least if your
algorithm checks for possible value ranges.

> [FILE:  2.6.0-test5/fs/jffs2/scan.c]
> [FUNC:  jffs2_scan_medium]
> [LINES: 98-109]
> [VAR:   flashbuf]
>   93:			buf_size = c->sector_size;
>   94:		else
>   95:			buf_size = PAGE_SIZE;
>   96:
>   97:		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
> START -->
>   98:		flashbuf = kmalloc(buf_size, GFP_KERNEL);
>   99:		if (!flashbuf)
>  100:			return -ENOMEM;
>  101:	}
>  102:
>  103:	for (i=0; i<c->nr_blocks; i++) {
>  104:		struct jffs2_eraseblock *jeb = &c->blocks[i];
>  105:
>  106:		ret = jffs2_scan_eraseblock(c, jeb, buf_size?flashbuf:(flashbuf+jeb->offset), buf_size);
>  107:
>  108:		if (ret < 0)
> END -->
>  109:			return ret;
>  110:
>  111:		ACCT_PARANOIA_CHECK(jeb);
>  112:
>  113:		/* Now decide which list to put it on */
>  114:		switch(ret) {

Valid.  And not trivial to fix.

> [FILE:  2.6.0-test5/fs/jffs2/scan.c]
> [FUNC:  jffs2_scan_medium]
> [LINES: 98-233]
> [VAR:   flashbuf]
>   93:			buf_size = c->sector_size;
>   94:		else
>   95:			buf_size = PAGE_SIZE;
>   96:
>   97:		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
> START -->
>   98:		flashbuf = kmalloc(buf_size, GFP_KERNEL);
>   99:		if (!flashbuf)
>  100:			return -ENOMEM;
>  101:	}
>  102:
>  103:	for (i=0; i<c->nr_blocks; i++) {
>         ... DELETED 124 lines ...
>  228:	}
>  229:	if (c->nr_erasing_blocks) {
>  230:		if ( !c->used_size && ((empty_blocks+bad_blocks)!= c->nr_blocks || bad_blocks == c->nr_blocks) ) {
>  231:			printk(KERN_NOTICE "Cowardly refusing to erase blocks on filesystem with no valid JFFS2 nodes\n");
>  232:			printk(KERN_NOTICE "empty_blocks %d, bad_blocks %d, c->nr_blocks %d\n",empty_blocks,bad_blocks,c->nr_blocks);
> END -->
>  233:			return -EIO;
>  234:		}
>  235:		jffs2_erase_pending_trigger(c);
>  236:	}
>  237:	if (buf_size)
>  238:		kfree(flashbuf);

Same one, basically.

Jörn

-- 
Geld macht nicht glücklich.
Glück macht nicht satt.

[PATCH] fix memleak in fs/jffs2/scan.c (was: re: [CHECKER] 32 Memory Leaks on Error Paths)

[Jörn Engel]

On Tue, 16 September 2003 08:55:53 +0200, Jörn Engel wrote:
> 
> > [FILE:  2.6.0-test5/fs/jffs2/scan.c]
> > [FUNC:  jffs2_scan_medium]
> > [LINES: 98-109]
> > [VAR:   flashbuf]
> >   93:			buf_size = c->sector_size;
> >   94:		else
> >   95:			buf_size = PAGE_SIZE;
> >   96:
> >   97:		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
> > START -->
> >   98:		flashbuf = kmalloc(buf_size, GFP_KERNEL);
> >   99:		if (!flashbuf)
> >  100:			return -ENOMEM;
> >  101:	}
> >  102:
> >  103:	for (i=0; i<c->nr_blocks; i++) {
> >  104:		struct jffs2_eraseblock *jeb = &c->blocks[i];
> >  105:
> >  106:		ret = jffs2_scan_eraseblock(c, jeb, buf_size?flashbuf:(flashbuf+jeb->offset), buf_size);
> >  107:
> >  108:		if (ret < 0)
> > END -->
> >  109:			return ret;
> >  110:
> >  111:		ACCT_PARANOIA_CHECK(jeb);
> >  112:
> >  113:		/* Now decide which list to put it on */
> >  114:		switch(ret) {
> 
> Valid.  And not trivial to fix.

But at least trivial to band-aid around it.  This doesn't make the
function any nicer, but it should get rid of the leaks.

David, consider this to be public domain. :)

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

--- linux-2.6.0-test3/fs/jffs2/scan.c~jffs2_memleak	2003-07-05 23:59:33.000000000 +0200
+++ linux-2.6.0-test3/fs/jffs2/scan.c	2003-09-16 09:16:30.000000000 +0200
@@ -106,7 +106,7 @@
 		ret = jffs2_scan_eraseblock(c, jeb, buf_size?flashbuf:(flashbuf+jeb->offset), buf_size);
 
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		ACCT_PARANOIA_CHECK(jeb);
 
@@ -230,7 +230,8 @@
 		if ( !c->used_size && ((empty_blocks+bad_blocks)!= c->nr_blocks || bad_blocks == c->nr_blocks) ) {
 			printk(KERN_NOTICE "Cowardly refusing to erase blocks on filesystem with no valid JFFS2 nodes\n");
 			printk(KERN_NOTICE "empty_blocks %d, bad_blocks %d, c->nr_blocks %d\n",empty_blocks,bad_blocks,c->nr_blocks);
-			return -EIO;
+			ret = -EIO;
+			goto out;
 		}
 		jffs2_erase_pending_trigger(c);
 	}
@@ -241,6 +242,10 @@
 		c->mtd->unpoint(c->mtd, flashbuf, 0, c->mtd->size);
 #endif
 	return 0;
+out:
+	if (buf_size)
+		kfree(flashbuf);
+	return ret;
 }
 
 static int jffs2_fill_scan_buf (struct jffs2_sb_info *c, unsigned char *buf,

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Jörn Engel]

On Tue, 16 September 2003 08:55:53 +0200, Jörn Engel wrote:
> On Mon, 15 September 2003 21:35:46 -0700, David Yu Chen wrote:
> > 
> > [FILE:  2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c]
> > [FUNC:  cfi_staa_setup]
> > [LINES: 191-211]
> > [VAR:   mtd]
> >  186:	struct mtd_info *mtd;
> >  187:	unsigned long offset = 0;
> >  188:	int i,j;
> >  189:	unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
> >  190:
> > START -->
> >  191:	mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
> >  192:	//printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
> >  193:
> >  194:	if (!mtd) {
> >  195:		printk(KERN_ERR "Failed to allocate memory for MTD device\n");
> >  196:		kfree(cfi->cmdset_priv);
> >         ... DELETED 9 lines ...
> >  206:	mtd->eraseregions = kmalloc(sizeof(struct mtd_erase_region_info) 
> >  207:			* mtd->numeraseregions, GFP_KERNEL);
> >  208:	if (!mtd->eraseregions) { 
> >  209:		printk(KERN_ERR "Failed to allocate memory for MTD erase region info\n");
> >  210:		kfree(cfi->cmdset_priv);
> > END -->
> >  211:		return NULL;
> >  212:	}
> >  213:	
> >  214:	for (i=0; i<cfi->cfiq->NumEraseRegions; i++) {
> >  215:		unsigned long ernum, ersize;
> >  216:		ersize = ((cfi->cfiq->EraseRegionInfo[i] >> 8) & ~0xff) * cfi->interleave;
> 
> Valid.

This should be fixed by finally taking the time and merging the
command sets 0001 and 0020.  Maybe someone who cares will come up with
a patch.

Jörn

-- 
Simplicity is prerequisite for reliability.
-- Edsger W. Dijkstra

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Jörn Engel]

On Tue, 16 September 2003 08:55:53 +0200, Jörn Engel wrote:
> On Mon, 15 September 2003 21:35:46 -0700, David Yu Chen wrote:
> > 
> > looks like checking for mtdblks instead of mtdblk
> > [FILE:  2.6.0-test5/drivers/mtd/mtdblock.c]
> > [FUNC:  mtdblock_open]
> > [LINES: 277-279]
> > [VAR:   mtdblk]
> >  272:		mtdblks[dev]->count++;
> >  273:		return 0;
> >  274:	}
> >  275:	
> >  276:	/* OK, it's not open. Create cache info for it */
> > START -->
> >  277:	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
> >  278:	if (!mtdblks)
> > END -->
> >  279:		return -ENOMEM;
> >  280:
> >  281:	memset(mtdblk, 0, sizeof(*mtdblk));
> >  282:	mtdblk->count = 1;
> >  283:	mtdblk->mtd = mtd;
> >  284:
> 
> Invalid.  This is quite an obvious false positive, at least if your
> algorithm checks for possible value ranges.

Actually, it *is* valid, as Wade pointed out to me.

David, please apply!

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

--- linux-2.6.0-test3/drivers/mtd/mtdblock.c~mtdblock_leak	2003-07-05 23:59:30.000000000 +0200
+++ linux-2.6.0-test3/drivers/mtd/mtdblock.c	2003-09-16 10:47:58.000000000 +0200
@@ -275,7 +275,7 @@
 	
 	/* OK, it's not open. Create cache info for it */
 	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
-	if (!mtdblks)
+	if (!mtdblk)
 		return -ENOMEM;
 
 	memset(mtdblk, 0, sizeof(*mtdblk));

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Timothy Miller]

>> 276:	/* OK, it's not open. Create cache info for it */
>>START -->
>> 277:	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
>> 278:	if (!mtdblks)
>>END -->
>> 279:		return -ENOMEM;

> 
> Invalid.  This is quite an obvious false positive, at least if your
> algorithm checks for possible value ranges.

Wait... one is "mtdblk", and the other is "mtdblks".  One has an extra 
's' on it.  Unless there is some kind of aliasing going on, they would 
appear to be different variables.  Naturally, I didn't check the 
original code, so I could be full of it.  :)

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

On Tue, 16 Sep 2003 10:52:06 EDT, Timothy Miller said:
> 
> >> 276:	/* OK, it's not open. Create cache info for it */
> >>START -->
> >> 277:	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
> >> 278:	if (!mtdblks)
> >>END -->
> >> 279:		return -ENOMEM;
> 
> > 
> > Invalid.  This is quite an obvious false positive, at least if your
> > algorithm checks for possible value ranges.
> 
> Wait... one is "mtdblk", and the other is "mtdblks".  One has an extra 
> 's' on it.  Unless there is some kind of aliasing going on, they would 
> appear to be different variables.  Naturally, I didn't check the 
> original code, so I could be full of it.  :)

On the other hand, even if the report was invalid (which another posting says isn't
the case), this code is just *begging* for somebody to "fix the typo", due to how
much the kernel uses
	foo = function(..)
	if (!foo)
Yeah, they're different variables - but we alloc mktblk and then return -ENOMEM
if the OTHER variable is null?? ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Nick Piggin]

Timothy Miller wrote:

>
>>> 276:    /* OK, it's not open. Create cache info for it */
>>> START -->
>>> 277:    mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
>>> 278:    if (!mtdblks)
>>> END -->
>>> 279:        return -ENOMEM;
>>
>
>>
>> Invalid.  This is quite an obvious false positive, at least if your
>> algorithm checks for possible value ranges.
>
>
> Wait... one is "mtdblk", and the other is "mtdblks".  One has an extra 
> 's' on it.  Unless there is some kind of aliasing going on, they would 
> appear to be different variables.  Naturally, I didn't check the 
> original code, so I could be full of it.  :)


Yes its a bug from glancing at the source code.