2006-12-07 05:13:23

by Amit Choudhary

[permalink] [raw]
Subject: [PATCH 2.6.19] drivers/media/video/cpia2/cpia2_usb.c: Free previously allocated memory (in array elements) if kmalloc() returns NULL.

Description: Free previously allocated memory (in array elements) if kmalloc() returns NULL, in function submit_urbs(), in file drivers/media/video/cpia2/cpia2_usb.c. If the system is low on memory, then previously allocated memory in the same array should be freed up to help the system recover.

Signed-off-by: Amit Choudhary <[email protected]>

diff --git a/drivers/media/video/cpia2/cpia2_usb.c b/drivers/media/video/cpia2/cpia2_usb.c
index 28dc6a1..c938638 100644
--- a/drivers/media/video/cpia2/cpia2_usb.c
+++ b/drivers/media/video/cpia2/cpia2_usb.c
@@ -640,6 +640,10 @@ static int submit_urbs(struct camera_dat
cam->sbuf[i].data =
kmalloc(FRAMES_PER_DESC * FRAME_SIZE_PER_DESC, GFP_KERNEL);
if (!cam->sbuf[i].data) {
+ for (--i; i >= 0; i--) {
+ kfree(cam->sbuf[i].data);
+ cam->sbuf[i].data = NULL;
+ }
return -ENOMEM;
}
}


2006-12-07 23:51:01

by Mariusz Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2.6.19] drivers/media/video/cpia2/cpia2_usb.c: Free previously allocated memory (in array elements) if kmalloc() returns NULL.

Hello,

> --- a/drivers/media/video/cpia2/cpia2_usb.c
> +++ b/drivers/media/video/cpia2/cpia2_usb.c
> @@ -640,6 +640,10 @@ static int submit_urbs(struct camera_dat
> cam->sbuf[i].data =
> kmalloc(FRAMES_PER_DESC * FRAME_SIZE_PER_DESC, GFP_KERNEL);
> if (!cam->sbuf[i].data) {
> + for (--i; i >= 0; i--) {
> + kfree(cam->sbuf[i].data);
> + cam->sbuf[i].data = NULL;
> + }
> return -ENOMEM;
> }
> }

Just for future. Shorter and more readable version of your for(...) thing:

while (i--) {
...
}

--
Regards,

Mariusz Kozlowski

2006-12-08 00:04:31

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 2.6.19] drivers/media/video/cpia2/cpia2_usb.c: Free previously allocated memory (in array elements) if kmalloc() returns NULL.

On Fri, 8 Dec 2006, Mariusz Kozlowski wrote:

> > --- a/drivers/media/video/cpia2/cpia2_usb.c
> > +++ b/drivers/media/video/cpia2/cpia2_usb.c
> > @@ -640,6 +640,10 @@ static int submit_urbs(struct camera_dat
> > cam->sbuf[i].data =
> > kmalloc(FRAMES_PER_DESC * FRAME_SIZE_PER_DESC, GFP_KERNEL);
> > if (!cam->sbuf[i].data) {
> > + for (--i; i >= 0; i--) {
> > + kfree(cam->sbuf[i].data);
> > + cam->sbuf[i].data = NULL;
> > + }
> > return -ENOMEM;
> > }
> > }
>
> Just for future. Shorter and more readable version of your for(...) thing:
>
> while (i--) {
> ...
> }
>

No, that is not equivalent.

You want
while (i-- >= 0) {
...
}

2006-12-08 00:09:34

by Mariusz Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2.6.19] drivers/media/video/cpia2/cpia2_usb.c: Free previously allocated memory (in array elements) if kmalloc() returns NULL.

Hello,

> > Just for future. Shorter and more readable version of your for(...) thing:
> >
> > while (i--) {
> > ...
> > }
> >
>
> No, that is not equivalent.
>
> You want
> while (i-- >= 0) {
> ...
> }
>

Not really. That will stop at -1 not 0.

--
Regards,

Mariusz Kozlowski

2006-12-08 00:36:37

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 2.6.19] drivers/media/video/cpia2/cpia2_usb.c: Free previously allocated memory (in array elements) if kmalloc() returns NULL.

On Fri, 8 Dec 2006, Mariusz Kozlowski wrote:

> > > Just for future. Shorter and more readable version of your for(...) thing:
> > >
> > > while (i--) {
> > > ...
> > > }
> > >
> >
> > No, that is not equivalent.
> >
> > You want
> > while (i-- >= 0) {
> > ...
> > }
> >
>
> Not really. That will stop at -1 not 0.
>

It depends on your intent. Generally speaking,

while (i--) {
...
}

is never what you want. Adding the check for being greater than 0 stops
potential bugs from signed int i being negative. The only drawback on x86
is that it sets a byte based on the greater condition with setg and tests
it later with testb for every iteration. This use of testb will _always_
use the same addressable byte registers for both its operands.

Based on this particular patch, I agree that

while (i-- > 0) {
...
}

will do the job. This is equivalent to the original for loop and ensures
that a negative value of i is never iterated on (even though it admittedly
would never be negative in this instance to begin with).

David