Re: [KJ] patch to fix initialization code of edd.c

[Jaco Kroon <jaco@kroon.co.za>]

[-- Attachment #1.1: Type: text/plain, Size: 1752 bytes --]

Om. wrote:
> On 7/29/06, Neil Horman <nhorman@tuxdriver.com> wrote:
> 
>>On Sat, Jul 29, 2006 at 06:31:49PM -0700, Om. wrote:
>>
>>>Hi,
>>>BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
>>>the init function causes memory leaks in src/drivers/firmware/edd.c
>>>This is my first patch submission. It is generated against 2.6.18-rc2.
>>>
>>>Review comments welcome.
>>>Regards,
>>>Om.
>>>
>>
>>Looks good, but instead of adding a stack variable failno to free the allocated
>>devices on failure, why not just use the same i variable and count backwards in
>>the loop under alloc_fail?
> 
> I tried that first. But there would be some ambiguity between i=0 as a
> failure and i=0 as a success.
> What I meant is,
> 	int i;
> ...
> 	for (i = 0; i < edd_num_devices(); i++) {
> 		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
> 		if (!edev) {
> 			rc = -ENOMEM;
> 			goto alloc_fail;
> 		}
> ....
> 		edd_devices[i] = edev;
> 	}
> ...
> alloc_fail:
> 	for (; i >= 0; i--) {
> 		edd_device_unregister (edd_devices[i]);
> 		kfree (edd_devices[i]);
> 	}

i is unsigned and this code would crash for _all_ error values of i.
I'm thinking something like this might work better:

while(i-- > 0) {
        edd_device_unregister(edd_devices[i]);
        kfree(edd_devices[i]);
}

The reasoning here is that the device indicated by i is the one that
failed, in other words, it already didn't allocate, so if we get here
with i == 0 then we don't really have anything to de-allocate.  If we
get here with i == 1 then it means edd_devices[1] has _not_ been
allocated, but we do want to free for i == 0.

Comments?

Jaco
-- 
There are only 10 kinds of people in this world,
  those that understand binary and those that don't.
http://www.kroon.co.za/

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3233 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors