sizeof(ptr) or sizeof(*ptr)?

sizeof(ptr) or sizeof(*ptr)?

[pmarques]

Last week a bug was detected in n_tty.c where an array of char was replaced by a
char pointer making a "(len > sizeof(buf))" condition test for len > 4 (or 8)
bytes, instead of the original array size.

I decided to tweak sparse to give warnings on sizeof(pointer), so that I could
check for other cases like this. The tweak was a very crude hack that I'm not
proud of, and I am still trying to make it more reliable.

So far I found 2 interesting cases (in 2.6.11-rc5). I'm not sure they are bugs,
but they sure look suspicious.

1: drivers/usb/storage/usb.c:788

	/*
	 * Since this is a new device, we need to register a SCSI
	 * host definition with the higher SCSI layers.
	 */
	us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(us));
	if (!us->host) {
		printk(KERN_WARNING USB_STORAGE
			"Unable to allocate the scsi host\n");
		return -EBUSY;
	}

"us" is a "struct us_data *". It seems pretty weird that we're allocating
something the size of a pointer, and then waste a pointer to keep the address
where it is allocated. So maybe this should be:

	us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(*us));


2: sound/core/control.c:936

	ue = kcalloc(1, sizeof(struct user_element) + private_size + extra_size,
GFP_KERNEL);
	if (ue == NULL)
		return -ENOMEM;
	ue->info = info;
	ue->elem_data = (char *)ue + sizeof(ue);
	ue->elem_data_size = private_size;
	if (extra_size) {
		ue->priv_data = (char *)ue + sizeof(ue) + private_size;
		ue->priv_data_size = extra_size;
		if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) {
			if (copy_from_user(ue->priv_data, *(char __user
**)info.value.enumerated.name, extra_size))
				return -EFAULT;
		}
	}

If we're allocating "sizeof(struct user_element) + private_size + extra_size" it
seems that in the instructions below we would be wanting to use that space, so
both "sizeof(ue)" there should in fact be "sizeof(*ue)"

I'm not *really* sure that these are bugs, but they look suspicious. I'm CC'ing
the maintainers of both these files so that they can check these out.

I'll probably bring more examples in the future, as the tool improves. Right now
it gives about 550 false positives, so I think it is better to improve it
before checking every case :)

--
Paulo Marques - www.grupopie.com
 
All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)

Re: sizeof(ptr) or sizeof(*ptr)?

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

On Sun, Feb 27, 2005 at 08:25:04PM +0000, pmarques@grupopie.com wrote:
> I decided to tweak sparse to give warnings on sizeof(pointer), so that I could
> check for other cases like this. The tweak was a very crude hack that I'm not
> proud of, and I am still trying to make it more reliable.
> 
> So far I found 2 interesting cases (in 2.6.11-rc5). I'm not sure they are bugs,
> but they sure look suspicious.
> 
> 1: drivers/usb/storage/usb.c:788
> 
> 	/*
> 	 * Since this is a new device, we need to register a SCSI
> 	 * host definition with the higher SCSI layers.
> 	 */
> 	us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(us));
> 	if (!us->host) {
> 		printk(KERN_WARNING USB_STORAGE
> 			"Unable to allocate the scsi host\n");
> 		return -EBUSY;
> 	}
> 
> "us" is a "struct us_data *". It seems pretty weird that we're allocating
> something the size of a pointer, and then waste a pointer to keep the address
> where it is allocated. So maybe this should be:
> 
> 	us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(*us));

This is actually correct as-is.  We're allocating a host, and asking for
the sizeof(pointer) in the 'extra storage' section.  We then store the
pointer (not what it points to) in the extra storage section a few lines down.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

S:  Another stupid question?
G:  There's no such thing as a stupid question, only stupid people.
					-- Stef and Greg
User Friendly, 7/15/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: sizeof(ptr) or sizeof(*ptr)?

[pmarques]

Quoting Matthew Dharm <mdharm-kernel@one-eyed-alien.net>:
> [...]
> 	us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(*us));
> 
> This is actually correct as-is.  We're allocating a host, and asking for
> the sizeof(pointer) in the 'extra storage' section.  We then store the
> pointer (not what it points to) in the extra storage section a few lines
> down.

Thanks for clarifying that. I guess the weekend effect got me, because at a
certain point I was starting to read the scsi_host_alloc as if it were a
kmalloc or something... :P

Anyway, after improving the tool and checking for false positives, there is only
one more suspicious piece of code in drivers/acpi/video.c:561

	status = acpi_video_device_lcd_query_levels(device, &obj);

	if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count >= 2) {
		int count = 0;
		union acpi_object *o;

		br = kmalloc(sizeof &br, GFP_KERNEL);
		if (!br) {
			printk(KERN_ERR "can't allocate memory\n");
		} else {
			memset(br, 0, sizeof &br);
			br->levels = kmalloc(obj->package.count * sizeof &br->levels, GFP_KERNEL);
			if (!br->levels)
				goto out;

"br" if of type "struct acpi_video_device_brightness *". 

"sizeof &br" doesn't make much sense there (besides the unconventional use of
sizeof without parenthesis) because the rest of the code seem to suggest that a
whole structure should have been allocated. This is the last case I've seen,
and I've added the maintainer to the cc list, so that he can check the code for
correctness.

Well, sorry about bothering you with a false positive (that I should've
recognised on my own, anyway), and thanks for the help.

--
Paulo Marques - www.grupopie.com
 
All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)

Re: sizeof(ptr) or sizeof(*ptr)?

[Andrew Morton]

"" <pmarques@grupopie.com> wrote:
>
> Anyway, after improving the tool and checking for false positives, there is only
>  one more suspicious piece of code in drivers/acpi/video.c:561
> 
>  	status = acpi_video_device_lcd_query_levels(device, &obj);
> 
>  	if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count >= 2) {
>  		int count = 0;
>  		union acpi_object *o;
> 
>  		br = kmalloc(sizeof &br, GFP_KERNEL);

yup, bug.

>  		if (!br) {
>  			printk(KERN_ERR "can't allocate memory\n");
>  		} else {
>  			memset(br, 0, sizeof &br);
>  			br->levels = kmalloc(obj->package.count * sizeof &br->levels, GFP_KERNEL);

And another one, although it happens to work out OK.

I'll get these all fixed up, thanks.

Re: sizeof(ptr) or sizeof(*ptr)?

[Paulo Marques]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

Andrew Morton wrote:
> "" <pmarques@grupopie.com> wrote:
> 
>>Anyway, after improving the tool and checking for false positives, there is only
>> one more suspicious piece of code in drivers/acpi/video.c:561
>>
>> 	status = acpi_video_device_lcd_query_levels(device, &obj);
>>
>> 	if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count >= 2) {
>> 		int count = 0;
>> 		union acpi_object *o;
>>
>> 		br = kmalloc(sizeof &br, GFP_KERNEL);
> 
> 
> yup, bug.
> 
> 
>> 		if (!br) {
>> 			printk(KERN_ERR "can't allocate memory\n");
>> 		} else {
>> 			memset(br, 0, sizeof &br);
>> 			br->levels = kmalloc(obj->package.count * sizeof &br->levels, GFP_KERNEL);
> 
> 
> And another one, although it happens to work out OK.
> 
> I'll get these all fixed up, thanks.

I just checked the 2.6.11-mm1 release and this is only half-fixed there, 
and it is the worst of both halves: the kmalloc only mallocs the size of 
a pointer, but the memset is fixed, so it memset's the size of a 
structure (oops). This is partially my fault for not sending the patch 
in the first place, together with the bug report.

The attached patch against 2.6.11-mm1 should fix the kmalloc.

By the way, I haven't got any response from an alsa developer about the 
bug in sound/core/control.c, but this is already fixed in 2.6.11-mm1, 
along with several other changes to that file. So the status is: it was 
a bug, but it is already fixed :)

-- 
Paulo Marques - www.grupopie.com

All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)

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

--- ./drivers/acpi/video.c.orig	2005-03-08 13:07:42.000000000 +0000
+++ ./drivers/acpi/video.c	2005-03-08 13:09:05.000000000 +0000
@@ -564,11 +564,11 @@ acpi_video_device_find_cap (struct acpi_
 		int count = 0;
 		union acpi_object *o;
 		
-		br = kmalloc(sizeof &br, GFP_KERNEL);
+		br = kmalloc(sizeof(*br), GFP_KERNEL);
 		if (!br) {
 			printk(KERN_ERR "can't allocate memory\n");
 		} else {
-			memset(br, 0, sizeof *br);
+			memset(br, 0, sizeof(*br));
 			br->levels = kmalloc(obj->package.count *
 					sizeof *(br->levels), GFP_KERNEL);
 			if (!br->levels)