Hello!
There seem to be memleak convert_2digits_to_char() function that is triggered
during normal operations.
Also I think there are some memleaks on error exit paths
ebda_rsrc_controller()
All of this is addressed by below patch.
2.5 seems to have totally different version of this code (and no
convert_2digits_to_char() function at all for example).
Found with help of smatch + enhanced unfree script.
Bye,
Oleg
===== drivers/hotplug/ibmphp_ebda.c 1.6 vs edited =====
--- 1.6/drivers/hotplug/ibmphp_ebda.c Fri Sep 13 21:56:25 2002
+++ edited/drivers/hotplug/ibmphp_ebda.c Thu Mar 13 23:40:29 2003
@@ -597,8 +597,8 @@
char *str1;
str = (char *) kmalloc (3, GFP_KERNEL);
- memset (str, 0, 3);
- str1 = (char *) kmalloc (2, GFP_KERNEL);
+ if (!str)
+ return NULL;
memset (str, 0, 3);
bit = (int)(var / 10);
switch (bit) {
@@ -608,13 +608,20 @@
return str;
default:
//2 digits number
+ str1 = (char *) kmalloc (2, GFP_KERNEL);
+ if (!str1) {
+ break;
+ }
+ memset (str, 0, 3);
*str1 = (char)(bit + 48);
strncpy (str, str1, 1);
memset (str1, 0, 3);
*str1 = (char)((var % 10) + 48);
strcat (str, str1);
+ kfree(str1);
return str;
- }
+ }
+ kfree(str);
return NULL;
}
@@ -1022,6 +1029,10 @@
bus_info_ptr1 = ibmphp_find_same_bus_num (hpc_ptr->slots[index].slot_bus_num);
if (!bus_info_ptr1) {
iounmap (io_mem);
+ kfree (hp_slot_ptr->name);
+ kfree (hp_slot_ptr->info);
+ kfree (hp_slot_ptr->private);
+ kfree (hp_slot_ptr);
return -ENODEV;
}
((struct slot *) hp_slot_ptr->private)->bus_on = bus_info_ptr1;
@@ -1036,12 +1047,20 @@
rc = ibmphp_hpc_fillhpslotinfo (hp_slot_ptr);
if (rc) {
iounmap (io_mem);
+ kfree (hp_slot_ptr->name);
+ kfree (hp_slot_ptr->info);
+ kfree (hp_slot_ptr->private);
+ kfree (hp_slot_ptr);
return rc;
}
rc = ibmphp_init_devno ((struct slot **) &hp_slot_ptr->private);
if (rc) {
iounmap (io_mem);
+ kfree (hp_slot_ptr->name);
+ kfree (hp_slot_ptr->info);
+ kfree (hp_slot_ptr->private);
+ kfree (hp_slot_ptr);
return rc;
}
hp_slot_ptr->ops = &ibmphp_hotplug_slot_ops;
On Thu, Mar 13, 2003 at 11:45:56PM +0300, Oleg Drokin wrote:
> Hello!
>
> There seem to be memleak convert_2digits_to_char() function that is triggered
> during normal operations.
> Also I think there are some memleaks on error exit paths
> ebda_rsrc_controller()
> All of this is addressed by below patch.
> 2.5 seems to have totally different version of this code (and no
> convert_2digits_to_char() function at all for example).
Yes, the 2.5 version should be backported to 2.4. There have been a
number of error patch fixes in the 2.5 version, care to make up a patch?
> Found with help of smatch + enhanced unfree script.
The 2.5 changes were found with smatch too :)
thanks,
greg k-h
On Thursday 13 March 2003 21.45, Oleg Drokin wrote:
Below, why allocating 2 bytes on heap (str1,) only to non-conditionally free
it a few lines further down? Why not keep the two bytes on stack instead? It
also seems like a bad idea to strncopy/strcat 1 byte long strings.
_
/Bjorn.
> ===== drivers/hotplug/ibmphp_ebda.c 1.6 vs edited =====
> --- 1.6/drivers/hotplug/ibmphp_ebda.c Fri Sep 13 21:56:25 2002
> +++ edited/drivers/hotplug/ibmphp_ebda.c Thu Mar 13 23:40:29 2003
> @@ -608,13 +608,20 @@
> return str;
> default:
> //2 digits number
> + str1 = (char *) kmalloc (2, GFP_KERNEL);
> + if (!str1) {
> + break;
> + }
> + memset (str, 0, 3);
> *str1 = (char)(bit + 48);
> strncpy (str, str1, 1);
> memset (str1, 0, 3);
> *str1 = (char)((var % 10) + 48);
> strcat (str, str1);
> + kfree(str1);
> return str;
> - }
> + }
> + kfree(str);
> return NULL;
> }
>
> @@ -1022,6 +1029,10 @@
> bus_info_ptr1 = ibmphp_find_same_bus_num
> (hpc_ptr->slots[index].slot_bus_num); if (!bus_info_ptr1) {
> iounmap (io_mem);
> + kfree (hp_slot_ptr->name);
> + kfree (hp_slot_ptr->info);
> + kfree (hp_slot_ptr->private);
> + kfree (hp_slot_ptr);
> return -ENODEV;
> }
> ((struct slot *) hp_slot_ptr->private)->bus_on = bus_info_ptr1;
> @@ -1036,12 +1047,20 @@
> rc = ibmphp_hpc_fillhpslotinfo (hp_slot_ptr);
> if (rc) {
> iounmap (io_mem);
> + kfree (hp_slot_ptr->name);
> + kfree (hp_slot_ptr->info);
> + kfree (hp_slot_ptr->private);
> + kfree (hp_slot_ptr);
> return rc;
> }
>
> rc = ibmphp_init_devno ((struct slot **) &hp_slot_ptr->private);
> if (rc) {
> iounmap (io_mem);
> + kfree (hp_slot_ptr->name);
> + kfree (hp_slot_ptr->info);
> + kfree (hp_slot_ptr->private);
> + kfree (hp_slot_ptr);
> return rc;
> }
> hp_slot_ptr->ops = &ibmphp_hotplug_slot_ops;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Fri, Mar 14, 2003 at 09:34:44AM +0100, Bj?rn Fahller wrote:
> On Thursday 13 March 2003 21.45, Oleg Drokin wrote:
>
> Below, why allocating 2 bytes on heap (str1,) only to non-conditionally free
> it a few lines further down? Why not keep the two bytes on stack instead? It
> also seems like a bad idea to strncopy/strcat 1 byte long strings.
Like I previously said, this whole function is a bad dream. Look at
what is now in 2.5, all of this nonsense is now gone.
thanks,
greg k-h
On 13 March 2003 22:45, Oleg Drokin wrote:
> Hello!
>
> There seem to be memleak convert_2digits_to_char() function that
> is triggered during normal operations.
> Also I think there are some memleaks on error exit paths
> ebda_rsrc_controller()
> All of this is addressed by below patch.
> 2.5 seems to have totally different version of this code (and no
> convert_2digits_to_char() function at all for example).
> Found with help of smatch + enhanced unfree script.
>
> Bye,
> Oleg
Oleg, you are doing great job. Thanks!
Well.. I just looked into that function. Do we have a kernel
obfuscated C contest? ;)
> ===== drivers/hotplug/ibmphp_ebda.c 1.6 vs edited =====
> --- 1.6/drivers/hotplug/ibmphp_ebda.c Fri Sep 13 21:56:25 2002
> +++ edited/drivers/hotplug/ibmphp_ebda.c Thu Mar 13 23:40:29 2003
> @@ -597,8 +597,8 @@
> char *str1;
>
> str = (char *) kmalloc (3, GFP_KERNEL);
BTW, that char* str is not local. It is not even static.
It is global symbol: "char *chassis_str, *rxe_str, *str;"
> - memset (str, 0, 3);
> - str1 = (char *) kmalloc (2, GFP_KERNEL);
A bit weird to have 3 and 2 bytes allocated in two separate kmalloc()
> + if (!str)
> + return NULL;
> memset (str, 0, 3);
This was fun, right? "Lets add second memset just in case
first failed" ;) ;)
> bit = (int)(var / 10);
> switch (bit) {
> @@ -608,13 +608,20 @@
> return str;
> default:
> //2 digits number
> + str1 = (char *) kmalloc (2, GFP_KERNEL);
> + if (!str1) {
> + break;
> + }
> + memset (str, 0, 3);
> + memset (str, 0, 3);
> *str1 = (char)(bit + 48);
> strncpy (str, str1, 1);
> memset (str1, 0, 3);
Wow! *str1 is 2 bytes long, not 3!
Anyway, here is the diff against some old 2.5 (sorry don't have latest
tree here at the moment). Also here are old and new functions
for easy visual diff. Completely untested:
static char *convert_2digits_to_char (int var)
{
int bit;
char *str1;
str = (char *) kmalloc (3, GFP_KERNEL);
memset (str, 0, 3);
str1 = (char *) kmalloc (2, GFP_KERNEL);
memset (str, 0, 3);
bit = (int)(var / 10);
switch (bit) {
case 0:
//one digit number
*str = (char)(var + 48);
return str;
default:
//2 digits number
*str1 = (char)(bit + 48);
strncpy (str, str1, 1);
memset (str1, 0, 3);
*str1 = (char)((var % 10) + 48);
strcat (str, str1);
return str;
}
return NULL;
}
static char *convert_2digits_to_char (int var)
{
int bit;
char *str = (char *) kmalloc (3, GFP_KERNEL);
if (!str)
return NULL;
bit = (int)(var / 10);
switch (bit) {
case 0:
//one digit number
str[0] = (char)(var + '0');
str[1] = 0;
break;
default:
//2 digits number
str[0] = (char)(bit + '0');
str[1] = (char)((var % 10) + '0');
str[2] = 0;
break;
}
return str;
}
--
vda
--- ibmphp_ebda.c Mon Nov 11 05:28:05 2002
+++ ibmphp_ebda.new.c Fri Mar 14 11:46:28 2003
@@ -65,7 +65,7 @@
static LIST_HEAD (opt_lo_head);
static void *io_mem;
-char *chassis_str, *rxe_str, *str;
+char *chassis_str, *rxe_str;
/* Local functions */
static int ebda_rsrc_controller (void);
@@ -594,28 +594,25 @@
static char *convert_2digits_to_char (int var)
{
int bit;
- char *str1;
- str = (char *) kmalloc (3, GFP_KERNEL);
- memset (str, 0, 3);
- str1 = (char *) kmalloc (2, GFP_KERNEL);
- memset (str, 0, 3);
+ char *str = (char *) kmalloc (3, GFP_KERNEL);
+ if (!str)
+ return NULL;
bit = (int)(var / 10);
switch (bit) {
case 0:
//one digit number
- *str = (char)(var + 48);
- return str;
+ str[0] = (char)(var + '0');
+ str[1] = 0;
+ break;
default:
//2 digits number
- *str1 = (char)(bit + 48);
- strncpy (str, str1, 1);
- memset (str1, 0, 3);
- *str1 = (char)((var % 10) + 48);
- strcat (str, str1);
- return str;
- }
- return NULL;
+ str[0] = (char)(bit + '0');
+ str[1] = (char)((var % 10) + '0');
+ str[2] = 0;
+ break;
+ }
+ return str;
}
/* Since we don't know the max slot number per each chassis, hence go
Hello!
On Fri, Mar 14, 2003 at 11:54:40AM +0200, Denis Vlasenko wrote:
> > + if (!str)
> > + return NULL;
> > memset (str, 0, 3);
> This was fun, right? "Lets add second memset just in case
> first failed" ;) ;)
Ah, I was half asleep when doing the change, so I missed
other obvious stuff it seems, like memset of three bytes to
two byte variabe :)
memset on wrong thing (second memset should be applied to str1 of course.)
But as Greg KH said already, whis stuff should be replaced by version from 2.5 instead ;)
> > bit = (int)(var / 10);
> > switch (bit) {
> > @@ -608,13 +608,20 @@
> > return str;
> > default:
> > //2 digits number
> > + str1 = (char *) kmalloc (2, GFP_KERNEL);
> > + if (!str1) {
> > + break;
> > + }
> > + memset (str, 0, 3);
> > + memset (str, 0, 3);
> > *str1 = (char)(bit + 48);
> > strncpy (str, str1, 1);
> > memset (str1, 0, 3);
> Wow! *str1 is 2 bytes long, not 3!
yup.
> Anyway, here is the diff against some old 2.5 (sorry don't have latest
> tree here at the moment). Also here are old and new functions
> for easy visual diff. Completely untested:
New 2.5 code does not have this function at all.
I will look into porting 2.5 changes back to 2.4 tonight. This seems to be proper solution
in this case.
Bye,
Oleg
Hello!
On Thu, Mar 13, 2003 at 04:31:44PM -0800, Greg KH wrote:
> > There seem to be memleak convert_2digits_to_char() function that is triggered
> > during normal operations.
> > Also I think there are some memleaks on error exit paths
> > ebda_rsrc_controller()
> > All of this is addressed by below patch.
> > 2.5 seems to have totally different version of this code (and no
> > convert_2digits_to_char() function at all for example).
> Yes, the 2.5 version should be backported to 2.4. There have been a
> number of error patch fixes in the 2.5 version, care to make up a patch?
Ok, see below.
Note there is no credit for me. Your 2 patches from 2.5 applied to 2.4
just fine even without offsets, and stuff compiled (I have no hardware to test
how it works, but I presume it will work ok).
Comments for patches were:
C IBM PCI Hotplug driver: Clean up the slot filename generation logic a lot.
C IBM PCI Hotplug: fix a load of memory leak errors found by the checker project
.
Bye,
Oleg
===== drivers/hotplug/ibmphp_ebda.c 1.7 vs 1.8 =====
--- 1.7/drivers/hotplug/ibmphp_ebda.c Sat Nov 23 05:00:44 2002
+++ 1.8/drivers/hotplug/ibmphp_ebda.c Wed Feb 5 19:56:25 2003
@@ -65,8 +65,6 @@
static LIST_HEAD (opt_lo_head);
static void *io_mem;
-char *chassis_str, *rxe_str, *str;
-
/* Local functions */
static int ebda_rsrc_controller (void);
static int ebda_rsrc_rsrc (void);
@@ -591,32 +589,6 @@
return 0;
}
-static char *convert_2digits_to_char (int var)
-{
- int bit;
- char *str1;
-
- str = (char *) kmalloc (3, GFP_KERNEL);
- memset (str, 0, 3);
- str1 = (char *) kmalloc (2, GFP_KERNEL);
- memset (str, 0, 3);
- bit = (int)(var / 10);
- switch (bit) {
- case 0:
- //one digit number
- *str = (char)(var + 48);
- return str;
- default:
- //2 digits number
- *str1 = (char)(bit + 48);
- strncpy (str, str1, 1);
- memset (str1, 0, 3);
- *str1 = (char)((var % 10) + 48);
- strcat (str, str1);
- return str;
- }
- return NULL;
-}
/* Since we don't know the max slot number per each chassis, hence go
* through the list of all chassis to find out the range
@@ -701,7 +673,7 @@
{
struct opt_rio *opt_vg_ptr = NULL;
struct opt_rio_lo *opt_lo_ptr = NULL;
- char *ptr_chassis_num, *ptr_rxe_num, *ptr_slot_num;
+ static char str[30];
int which = 0; /* rxe = 1, chassis = 0 */
u8 number = 1; /* either chassis or rxe # */
u8 first_slot = 1;
@@ -715,19 +687,7 @@
slot_num = slot_cur->number;
- chassis_str = (char *) kmalloc (30, GFP_KERNEL);
- memset (chassis_str, 0, 30);
- rxe_str = (char *) kmalloc (30, GFP_KERNEL);
- memset (rxe_str, 0, 30);
- ptr_chassis_num = (char *) kmalloc (3, GFP_KERNEL);
- memset (ptr_chassis_num, 0, 3);
- ptr_rxe_num = (char *) kmalloc (3, GFP_KERNEL);
- memset (ptr_rxe_num, 0, 3);
- ptr_slot_num = (char *) kmalloc (3, GFP_KERNEL);
- memset (ptr_slot_num, 0, 3);
-
- strcpy (chassis_str, "chassis");
- strcpy (rxe_str, "rxe");
+ memset (str, 0, sizeof(str));
if (rio_table_ptr) {
if (rio_table_ptr->ver_num == 3) {
@@ -772,31 +732,10 @@
}
}
- switch (which) {
- case 0:
- /* Chassis */
- *ptr_chassis_num = (char)(number + 48);
- strcat (chassis_str, ptr_chassis_num);
- kfree (ptr_chassis_num);
- strcat (chassis_str, "slot");
- ptr_slot_num = convert_2digits_to_char (slot_num - first_slot + 1);
- strcat (chassis_str, ptr_slot_num);
- kfree (ptr_slot_num);
- return chassis_str;
- break;
- case 1:
- /* RXE */
- *ptr_rxe_num = (char)(number + 48);
- strcat (rxe_str, ptr_rxe_num);
- kfree (ptr_rxe_num);
- strcat (rxe_str, "slot");
- ptr_slot_num = convert_2digits_to_char (slot_num - first_slot + 1);
- strcat (rxe_str, ptr_slot_num);
- kfree (ptr_slot_num);
- return rxe_str;
- break;
- }
- return NULL;
+ sprintf(str, "%s%dslot%d",
+ which == 0 ? "chassis" : "rxe",
+ number, slot_num - first_slot + 1);
+ return str;
}
static struct pci_driver ibmphp_driver;
@@ -1060,10 +999,6 @@
slot_cur = list_entry (list, struct slot, ibm_slot_list);
snprintf (slot_cur->hotplug_slot->name, 30, "%s", create_file_name (slot_cur));
- if (chassis_str)
- kfree (chassis_str);
- if (rxe_str)
- kfree (rxe_str);
pci_hp_register (slot_cur->hotplug_slot);
}