[2.4] init/do_mounts.c::rd_load_image() memleak

[2.4] init/do_mounts.c::rd_load_image() memleak

[Oleg Drokin]

Hello!

   rd_load_image() leaks some memory if it cannot determine source device size,
   if it cannot close or open source for ramdisk device.

   Probably this is not all that critical, since we most likely panic after
   failure to load initrd, but still there is chance that we have valid
   root device too, from which we can try to continue to boot.

   Found with help of smatch + enhanced unfree script.

Bye,
    Oleg

===== init/do_mounts.c 1.35 vs edited =====
--- 1.35/init/do_mounts.c	Wed Jan 15 09:42:29 2003
+++ edited/init/do_mounts.c	Thu Mar 13 23:56:18 2003
@@ -551,7 +551,7 @@
 	int in_fd, out_fd;
 	unsigned long rd_blocks, devblocks;
 	int nblocks, i;
-	char *buf;
+	char *buf = 0;
 	unsigned short rotate = 0;
 #if !defined(CONFIG_ARCH_S390) && !defined(CONFIG_PPC_ISERIES)
 	char rotator[4] = { '|' , '/' , '-' , '\\' };
@@ -648,7 +648,6 @@
 #endif
 	}
 	printk("done.\n");
-	kfree(buf);
 
 successful_load:
 	res = 1;
@@ -656,6 +655,8 @@
 	close(in_fd);
 noclose_input:
 	close(out_fd);
+	if (buf)
+		kfree(buf);
 out:
 	sys_unlink("/dev/ram");
 #endif

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[Russell King]

On Fri, Mar 14, 2003 at 12:01:44AM +0300, Oleg Drokin wrote:
> +	if (buf)
> +		kfree(buf);

kfree(NULL); is valid - you don't need this check.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[Oleg Drokin]

Hello!

On Thu, Mar 13, 2003 at 10:03:08PM +0000, Russell King wrote:

> > +	if (buf)
> > +		kfree(buf);
> kfree(NULL); is valid - you don't need this check.

Almost every place I can think of does just this, so I do not see why this
particular piece of code should be different.

Bye,
    Oleg

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[Jens Axboe]

On Fri, Mar 14 2003, Oleg Drokin wrote:
> Hello!
> 
> On Thu, Mar 13, 2003 at 10:03:08PM +0000, Russell King wrote:
> 
> > > +	if (buf)
> > > +		kfree(buf);
> > kfree(NULL); is valid - you don't need this check.
> 
> Almost every place I can think of does just this, so I do not see why this
> particular piece of code should be different.

Since when has that been a valid argument? :)

-- 
Jens Axboe

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[Oleg Drokin]

Hello!

On Fri, Mar 14, 2003 at 08:59:57AM +0100, Jens Axboe wrote:

> > > > +	if (buf)
> > > > +		kfree(buf);
> > > kfree(NULL); is valid - you don't need this check.
> > Almost every place I can think of does just this, so I do not see why this
> > particular piece of code should be different.
> Since when has that been a valid argument? :)

Well, my argument is code uniformness which was always valid as long as it does not
introduce any bugs, I think.
Do you propose somebody should go and fix all
if ( something )
	kfree(something);
pieces of code to read just
kfree(something); ?

Bye,
    Oleg

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[Zwane Mwaikambo]

On Fri, 14 Mar 2003, Oleg Drokin wrote:

> Well, my argument is code uniformness which was always valid as long as it does not
> introduce any bugs, I think.
> Do you propose somebody should go and fix all
> if ( something )
> 	kfree(something);
> pieces of code to read just
> kfree(something); ?

It's defined that kfree(NULL) is valid. I tried the above mentioned 
'cleanup' a while ago, way too noisy.

	Zwane

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[Jens Axboe]

On Fri, Mar 14 2003, Oleg Drokin wrote:
> Hello!
> 
> On Fri, Mar 14, 2003 at 08:59:57AM +0100, Jens Axboe wrote:
> 
> > > > > +	if (buf)
> > > > > +		kfree(buf);
> > > > kfree(NULL); is valid - you don't need this check.
> > > Almost every place I can think of does just this, so I do not see why this
> > > particular piece of code should be different.
> > Since when has that been a valid argument? :)
> 
> Well, my argument is code uniformness which was always valid as long
> as it does not introduce any bugs, I think.

I agree with that.

> Do you propose somebody should go and fix all
> if ( something )
> 	kfree(something);
> pieces of code to read just
> kfree(something); ?

No that would just be another pointless exercise in causing more
annoyance for someone who has to look through patches finding that one
hunk that breaks stuff. The recent spelling changes come to mind.

But just because you don't seem to have seen any kfree(NULL) in the
kernel does not mean they are not there. And should a good trend not
allow to grow?

-- 
Jens Axboe

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[Denis Vlasenko]

On 14 March 2003 10:09, Jens Axboe wrote:
> No that would just be another pointless exercise in causing more
> annoyance for someone who has to look through patches finding that
> one hunk that breaks stuff. The recent spelling changes come to mind.

How we should do such global small cleanups?
Maybe grep the source and bring the list of affected files
to maintainers' attention, letting the to gradually push
changes to Linus...

I suspect "bring the list to maintainers' attention"
will be a trickier part ;)

> But just because you don't seem to have seen any kfree(NULL) in the
> kernel does not mean they are not there. And should a good trend not
> allow to grow?

"if(p) free(p)" => "free(p)" is mostly ok, less code.

But free is called now unconditionally. Make an exception
for performance-critical places where p is almost always 0.
--
vda

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[John Alvord]

On Fri, 14 Mar 2003 12:05:40 +0200, Denis Vlasenko
<vda@port.imtp.ilyichevsk.odessa.ua> wrote:

>On 14 March 2003 10:09, Jens Axboe wrote:
>> No that would just be another pointless exercise in causing more
>> annoyance for someone who has to look through patches finding that
>> one hunk that breaks stuff. The recent spelling changes come to mind.
>
>How we should do such global small cleanups?
>Maybe grep the source and bring the list of affected files
>to maintainers' attention, letting the to gradually push
>changes to Linus...
>
>I suspect "bring the list to maintainers' attention"
>will be a trickier part ;)
>
>> But just because you don't seem to have seen any kfree(NULL) in the
>> kernel does not mean they are not there. And should a good trend not
>> allow to grow?
>
>"if(p) free(p)" => "free(p)" is mostly ok, less code.
>
>But free is called now unconditionally. Make an exception
>for performance-critical places where p is almost always 0.

The one implementation I looked at carefully (SAS/C) looked like this:

 	free(p);
	---------
	if (p)
		malloc(p);

so 
	if (p) free(p);
	-------------
	if (p)
	     if (p)
		malloc(p);

which seems fairly worthless.

john

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[Horst von Brand]

Jens Axboe <axboe@suse.de> said:
> On Fri, Mar 14 2003, Oleg Drokin wrote:

[...]

> > Do you propose somebody should go and fix all
> > if ( something )
> > 	kfree(something);
> > pieces of code to read just
> > kfree(something); ?

> No that would just be another pointless exercise in causing more
> annoyance for someone who has to look through patches finding that one
> hunk that breaks stuff. The recent spelling changes come to mind.

By that standard, most janitorial patches should be ditched. That way, no
cruft will ever be removed.

Instead of this extremist position, some way should be found that minimizes
this kind of friction.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [2.4] init/do_mounts.c::rd_load_image() memleak

[Horst von Brand]

Oleg Drokin <green@namesys.com> said:
> On Thu, Mar 13, 2003 at 10:03:08PM +0000, Russell King wrote:
> > > +	if (buf)
> > > +		kfree(buf);

> > kfree(NULL); is valid - you don't need this check.

> Almost every place I can think of does just this, so I do not see why this
> particular piece of code should be different.

Then the other code should be fixed. This is bloat.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513