[PATCH] Fix a memory leak in the i386 setup code

[PATCH] Fix a memory leak in the i386 setup code

[Catalin Marinas]

From: Catalin Marinas <catalin.marinas@gmail.com>

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---

 arch/i386/kernel/setup.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/i386/kernel/setup.c b/arch/i386/kernel/setup.c
index 08c00d2..d32d264 100644
--- a/arch/i386/kernel/setup.c
+++ b/arch/i386/kernel/setup.c
@@ -1327,7 +1327,10 @@ #endif
 		res->start = e820.map[i].addr;
 		res->end = res->start + e820.map[i].size - 1;
 		res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-		request_resource(&iomem_resource, res);
+		if (request_resource(&iomem_resource, res)) {
+			kfree(res);
+			continue;
+		}
 		if (e820.map[i].type == E820_RAM) {
 			/*
 			 *  We don't know which RAM region contains kernel data,

Re: [PATCH] Fix a memory leak in the i386 setup code

[Rafael J. Wysocki]

Hi,

Does x86_64 need a similar fix?

On Tuesday 11 July 2006 00:13, Catalin Marinas wrote:
> From: Catalin Marinas <catalin.marinas@gmail.com>
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
> ---
> 
>  arch/i386/kernel/setup.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/i386/kernel/setup.c b/arch/i386/kernel/setup.c
> index 08c00d2..d32d264 100644
> --- a/arch/i386/kernel/setup.c
> +++ b/arch/i386/kernel/setup.c
> @@ -1327,7 +1327,10 @@ #endif
>  		res->start = e820.map[i].addr;
>  		res->end = res->start + e820.map[i].size - 1;
>  		res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> -		request_resource(&iomem_resource, res);
> +		if (request_resource(&iomem_resource, res)) {
> +			kfree(res);
> +			continue;
> +		}
>  		if (e820.map[i].type == E820_RAM) {
>  			/*
>  			 *  We don't know which RAM region contains kernel data,
> -

Evidently res is used if e820.map[i].type == E820_RAM, so it should
be freed later on, it seems.

Greetings,
Rafael

functions returning 0 on success [was: [PATCH] Fix a memory leak in the i386 setup code]

[Michael Tokarev]

Catalin Marinas wrote:
> From: Catalin Marinas <catalin.marinas@gmail.com>
[]
> -		request_resource(&iomem_resource, res);
> +		if (request_resource(&iomem_resource, res)) {
> +			kfree(res);
...

Just a small nitpick, or, rather, a question.
Quite alot of functions return 0 on success, or !=0 on failure.
There are other functions, which, say, return NULL on failure.
Or when 0 is valid result, something <0 is returned.. etc.

Without looking at the function description or code, it's
impossible to say wich of the above it is.  But.

When reading that kind of code as the quoted patch adds,
I for one always think it's somehow incorrect or backwards.
When used like that, request_resource() seems like a boolean,
and the whole thing becomes:

  if (do_something_successeful())
    fail();

Ofcourse, later you understand that do_something() returns 0
on failure, and the code is correct.  But the first impression
(again, for me anyway) is that it's wrong.

In such cases when a routine returns 0 on error, I usually write
it this way:

   if (request_resource() != 0)
     fail()

This way it becomes obvious what does it do, and compiler
generates EXACTLY the same instructions.

Yes it's redundrand, that "!= 0" tail.  But it makes the
whole stuff readable without a need to re-think what does it
return, and, which is especially important, logically correct
when reading.

Alternatively, it can be named something like

  request_resource_failed()

instead of just request_resource(). Compare:

 if (request_resource(..))
    fail();

(current).  And

 if (request_resource_failed())
   fail();

The latter seems more logical... ;)

But that "!= 0" tail achieves almost the same effect.
Yet again, from my point of view anyway... ;)

Thanks.

/mjt

Re: functions returning 0 on success [was: [PATCH] Fix a memory leak in the i386 setup code]

[Michael Tokarev]

Michael Tokarev wrote:
[]
> Ofcourse, later you understand that do_something() returns 0
> on failure, and the code is correct.  But the first impression
> (again, for me anyway) is that it's wrong.
> 
> In such cases when a routine returns 0 on error, I usually write
> it this way:
....
s/error/success/g.  Blah ;)
> 
>    if (request_resource() != 0)
>      fail()
[..]
/mjt

Re: [PATCH] Fix a memory leak in the i386 setup code

[Catalin Marinas]

On 10/07/06, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Does x86_64 need a similar fix?

Doesn't look like it needs this.

> On Tuesday 11 July 2006 00:13, Catalin Marinas wrote:
> > --- a/arch/i386/kernel/setup.c
> > +++ b/arch/i386/kernel/setup.c
> > @@ -1327,7 +1327,10 @@ #endif
> >               res->start = e820.map[i].addr;
> >               res->end = res->start + e820.map[i].size - 1;
> >               res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> > -             request_resource(&iomem_resource, res);
> > +             if (request_resource(&iomem_resource, res)) {
> > +                     kfree(res);
> > +                     continue;
> > +             }
> >               if (e820.map[i].type == E820_RAM) {
> >                       /*
> >                        *  We don't know which RAM region contains kernel data,
>
> Evidently res is used if e820.map[i].type == E820_RAM, so it should
> be freed later on, it seems.

The "if" block I added has a "continue" and therefore the E820_RAM
case is skipped. There is no point in requesting a resource with "res"
as parent when "res" couldn't be successfully acquired.

-- 
Catalin

Re: [PATCH] Fix a memory leak in the i386 setup code

[Rafael J. Wysocki]

On Tuesday 11 July 2006 09:47, Catalin Marinas wrote:
> On 10/07/06, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Does x86_64 need a similar fix?
> 
> Doesn't look like it needs this.
> 
> > On Tuesday 11 July 2006 00:13, Catalin Marinas wrote:
> > > --- a/arch/i386/kernel/setup.c
> > > +++ b/arch/i386/kernel/setup.c
> > > @@ -1327,7 +1327,10 @@ #endif
> > >               res->start = e820.map[i].addr;
> > >               res->end = res->start + e820.map[i].size - 1;
> > >               res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> > > -             request_resource(&iomem_resource, res);
> > > +             if (request_resource(&iomem_resource, res)) {
> > > +                     kfree(res);
> > > +                     continue;
> > > +             }
> > >               if (e820.map[i].type == E820_RAM) {
> > >                       /*
> > >                        *  We don't know which RAM region contains kernel data,
> >
> > Evidently res is used if e820.map[i].type == E820_RAM, so it should
> > be freed later on, it seems.
> 
> The "if" block I added has a "continue" and therefore the E820_RAM
> case is skipped. There is no point in requesting a resource with "res"
> as parent when "res" couldn't be successfully acquired.

Ah, right, it's freed when request_resource(&iomem_resource, res) fails.
Sorry for the noise.

Rafael

Re: functions returning 0 on success [was: [PATCH] Fix a memory leak in the i386 setup code]

[Dmitry Torokhov]

On Monday 10 July 2006 18:45, Michael Tokarev wrote:
> In such cases when a routine returns 0 on error, I usually write
> it this way:
> 
>    if (request_resource() != 0)
>      fail()
> 


Many write:

	error = do_something();
	if (error) {
		...
	}

And then it really obvious.

-- 
Dmitry