[lm-sensors] [PATCH 04/12] hwmon: (mc13783-adc) Increase size of name string

[lm-sensors] [PATCH 04/12] hwmon: (mc13783-adc) Increase size of name string

[Guenter Roeck]

smatch complains about

mc13783_adc_probe() error: snprintf() chops off the last chars of
'id->name': 20 +vs 10

Use PLATFORM_NAME_SIZE instead of '10' as size when declaring
the name variable.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/mc13783-adc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/mc13783-adc.c b/drivers/hwmon/mc13783-adc.c
index 982d862..ae00e60 100644
--- a/drivers/hwmon/mc13783-adc.c
+++ b/drivers/hwmon/mc13783-adc.c
@@ -37,7 +37,7 @@
 struct mc13783_adc_priv {
 	struct mc13xxx *mc13xxx;
 	struct device *hwmon_dev;
-	char name[10];
+	char name[PLATFORM_NAME_SIZE];
 };
 
 static ssize_t mc13783_adc_show_name(struct device *dev, struct device_attribute
-- 
1.7.9.7


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 04/12] hwmon: (mc13783-adc) Increase size of name string

[Jean Delvare]

On Sat, 14 Sep 2013 01:33:27 -0700, Guenter Roeck wrote:
> smatch complains about
> 
> mc13783_adc_probe() error: snprintf() chops off the last chars of
> 'id->name': 20 +vs 10
> 
> Use PLATFORM_NAME_SIZE instead of '10' as size when declaring
> the name variable.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Note that Dan Carpenter had been sending the same patch 19 months ago:

http://lists.lm-sensors.org/pipermail/lm-sensors/2012-February/035178.html

I wasn't sure what to do with it and finally forgot about it, sorry.

> ---
>  drivers/hwmon/mc13783-adc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/mc13783-adc.c b/drivers/hwmon/mc13783-adc.c
> index 982d862..ae00e60 100644
> --- a/drivers/hwmon/mc13783-adc.c
> +++ b/drivers/hwmon/mc13783-adc.c
> @@ -37,7 +37,7 @@
>  struct mc13783_adc_priv {
>  	struct mc13xxx *mc13xxx;
>  	struct device *hwmon_dev;
> -	char name[10];
> +	char name[PLATFORM_NAME_SIZE];
>  };
>  
>  static ssize_t mc13783_adc_show_name(struct device *dev, struct device_attribute

I admit I find it a bit sad to allocate more memory than we need just
to make a static analyzer happy. Static analyzers are supposed to help
developers improve their code, not make it less efficient.

That being said, I tried to rewrite the code in a way which was more
obviously correct (to a static analyzer), but what I came up with was
not very appealing.

So if everybody is OK with the simple buffer size fix above, then so am
I.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 04/12] hwmon: (mc13783-adc) Increase size of name string

[Guenter Roeck]

On 09/14/2013 11:00 AM, Jean Delvare wrote:
> On Sat, 14 Sep 2013 01:33:27 -0700, Guenter Roeck wrote:
>> smatch complains about
>>
>> mc13783_adc_probe() error: snprintf() chops off the last chars of
>> 'id->name': 20 +vs 10
>>
>> Use PLATFORM_NAME_SIZE instead of '10' as size when declaring
>> the name variable.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> Note that Dan Carpenter had been sending the same patch 19 months ago:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2012-February/035178.html
>
> I wasn't sure what to do with it and finally forgot about it, sorry.
>
>> ---
>>   drivers/hwmon/mc13783-adc.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/mc13783-adc.c b/drivers/hwmon/mc13783-adc.c
>> index 982d862..ae00e60 100644
>> --- a/drivers/hwmon/mc13783-adc.c
>> +++ b/drivers/hwmon/mc13783-adc.c
>> @@ -37,7 +37,7 @@
>>   struct mc13783_adc_priv {
>>   	struct mc13xxx *mc13xxx;
>>   	struct device *hwmon_dev;
>> -	char name[10];
>> +	char name[PLATFORM_NAME_SIZE];
>>   };
>>
>>   static ssize_t mc13783_adc_show_name(struct device *dev, struct device_attribute
>
> I admit I find it a bit sad to allocate more memory than we need just
> to make a static analyzer happy. Static analyzers are supposed to help
> developers improve their code, not make it less efficient.
>
> That being said, I tried to rewrite the code in a way which was more
> obviously correct (to a static analyzer), but what I came up with was
> not very appealing.
>
For me there is always a trade-off. In this case, using an arbitrary size of 10
adds at least some risk. Anyone ever messing with the variable might inadvertently
introduce a bug. So I thought it was worth changing it to use a well defined
constant instead.

There are other cases where static analyzers create noise - for example,
smatch complains about ignored return values in detect functions which are
replaced with -ENODEV. But on the other side it caught a number of
inconsistencies and at least one real bug, so I am perfectly happy
with the false positives.

Overall my approach is that I try to make static analyzers happy if it
doesn't create undue pain, simply because I think that they are
valuable tools to have around, and I don't want real problems
to disappear in the noise.

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors