Fix a memory leak in the i386 setup code

PATCH In review Catalin Marinas 4 participants 7 messages Jul 10, 2006 – Jul 15, 2006

lore

Share & tools

lore

Catalin Marinas

author

2006-07-10 22:13 UTC #0 lore raw

PATCH 1/1 Fix a memory leak in the i386 setup code

From: Catalin Marinas <[email protected]>

Signed-off-by Catalin Marinas <[email protected]>

---

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

arch/i386/kernel/setup.c +4 -1

index 08c00d2..d32d264 100644
@@ -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,

Rafael J. Wysocki

2006-07-10 22:25 UTC #1 lore raw

Hi,

Does x86_64 need a similar fix?

Quoted 15 lines · Catalin Marinas

From: Catalin Marinas <[email protected]>
Signed-off-by Catalin Marinas <[email protected]>
---
 arch/i386/kernel/setup.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
arch/i386/kernel/setup.c +0 -0
index 08c00d2..d32d264 100644
@@ -1327,7 +1327,10 @@ #endif
 		res->start = e820.map[i].addr;
 		res->end = res->start + e820.map[i].size - 1;

arch/i386/kernel/setup.c #endif

Quoted 10 lines

arch/i386/kernel/setup.c +4 -2

 		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

Catalin Marinas

author

2006-07-11 07:47 UTC #2 lore raw

Quoted 1 line · Rafael J. Wysocki

Does x86_64 need a similar fix?

Doesn't look like it needs this.

Quoted 10 lines · Catalin Marinas

Quoted 9 lines

arch/i386/kernel/setup.c +2 -1

@@ -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);

Quoted 8 lines

arch/i386/kernel/setup.c #endif · line 1336
Quoted 5 lines
arch/i386/kernel/setup.c +2 -0
+                     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.

Signature · 1 line

Catalin

Rafael J. Wysocki

2006-07-11 08:26 UTC #3 lore raw

Quoted 24 lines · Catalin Marinas

Quoted 1 line · Rafael J. Wysocki

Does x86_64 need a similar fix?

Doesn't look like it needs this.

Quoted 10 lines · Catalin Marinas

Quoted 9 lines

arch/i386/kernel/setup.c +2 -1

@@ -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);

Quoted 8 lines

arch/i386/kernel/setup.c #endif · line 1336
Quoted 5 lines
arch/i386/kernel/setup.c +2 -0
+                     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.

Quoted 3 lines

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

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

Michael Tokarev

2006-07-10 22:49 UTC #5 lore raw

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

Michael Tokarev wrote:
[]

Quoted 6 lines

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 ;)

Quoted 3 lines

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

[..]
/mjt

Dmitry Torokhov

2006-07-15 17:33 UTC #6 lore raw

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

In reply to Michael Tokarev

Quoted 6 lines · Michael Tokarev

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.

Signature · 1 line

Dmitry