Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 4, 2014 at 9:08 AM, Paul Bolle <pebolle@xxxxxxxxxx> wrote:
> On Thu, 2014-09-04 at 00:53 +0200, Frans Klaver wrote:
>> In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
>> fails, 'value' remains possibly uninitialized. In that case 'value'
>> shouldn't be used to produce the store_sys_acpi()s return value.
>>
>> Only test the return value of set_acpi() if we can actually call it.
>> Return rv otherwise.
>>
>> Signed-off-by: Frans Klaver <fransklaver@xxxxxxxxx>
>> ---
>>  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
>> index bd533c2..41f12ba 100644
>> --- a/drivers/platform/x86/eeepc-laptop.c
>> +++ b/drivers/platform/x86/eeepc-laptop.c
>> @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>>       int rv, value;
>>
>>       rv = parse_arg(buf, count, &value);
>> -     if (rv > 0)
>> -             value = set_acpi(eeepc, cm, value);
>> -     if (value < 0)
>> -             return -EIO;
>> +     if (rv > 0) {
>> +             if (set_acpi(eeepc, cm, value) < 0)
>> +                     return -EIO;
>> +     }
>>       return rv;
>>  }
>>
>
> The warning that this code (currently) generated triggered me to submit
> https://lkml.org/lkml/2014/7/1/150 , which uses a different approach to
> get rid of it. I received no reactions so far. Here's that patch again:
>
> ------------>8------------
> From: Paul Bolle <pebolle@xxxxxxxxxx>
> Subject: [PATCH] eeepc-laptop: simplify parse_arg()
>
> parse_arg() has three possible return values:
>     -EINVAL if sscanf(), in short, fails;
>     zero if "count" is zero; and
>     "count" in all other cases
>
> But "count" will never be zero. See, parse_arg() is called by the
> various store functions. And the callchain of these functions starts
> with sysfs_kf_write(). And that function checks for a zero "count". So
> we can stop checking for a zero "count", drop the "count" argument
> entirely, and transform parse_arg() into a function that returns zero on
> success or a negative error. That, in turn, allows to make those store
> functions just return "count" on success. The net effect is that the
> code becomes a bit easier to understand.
>
> A nice side effect is that this GCC warning is silenced too:
>     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
>     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>       int rv, value;
>
> Which is, of course, the reason to have a look at parse_arg().
>
> Signed-off-by: Paul Bolle <pebolle@xxxxxxxxxx>

Curious. I hadn't found this one when searching for earlier patches.

Thanks,
Frans


> ---
>  drivers/platform/x86/eeepc-laptop.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index bd533c22be57..78515b850165 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -263,13 +263,11 @@ static int acpi_setter_handle(struct eeepc_laptop *eeepc, int cm,
>  /*
>   * Sys helpers
>   */
> -static int parse_arg(const char *buf, unsigned long count, int *val)
> +static int parse_arg(const char *buf, int *val)
>  {
> -       if (!count)
> -               return 0;
>         if (sscanf(buf, "%i", val) != 1)
>                 return -EINVAL;
> -       return count;
> +       return 0;
>  }
>
>  static ssize_t store_sys_acpi(struct device *dev, int cm,
> @@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>         struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
>         int rv, value;
>
> -       rv = parse_arg(buf, count, &value);
> -       if (rv > 0)
> -               value = set_acpi(eeepc, cm, value);
> +       rv = parse_arg(buf, &value);
> +       if (rv < 0)
> +               return rv;
> +       value = set_acpi(eeepc, cm, value);
>         if (value < 0)
>                 return -EIO;
> -       return rv;
> +       return count;
>  }
>
>  static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
> @@ -377,13 +376,13 @@ static ssize_t store_cpufv(struct device *dev,
>                 return -EPERM;
>         if (get_cpufv(eeepc, &c))
>                 return -ENODEV;
> -       rv = parse_arg(buf, count, &value);
> +       rv = parse_arg(buf, &value);
>         if (rv < 0)
>                 return rv;
> -       if (!rv || value < 0 || value >= c.num)
> +       if (value < 0 || value >= c.num)
>                 return -EINVAL;
>         set_acpi(eeepc, CM_ASL_CPUFV, value);
> -       return rv;
> +       return count;
>  }
>
>  static ssize_t show_cpufv_disabled(struct device *dev,
> @@ -402,7 +401,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
>         struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
>         int rv, value;
>
> -       rv = parse_arg(buf, count, &value);
> +       rv = parse_arg(buf, &value);
>         if (rv < 0)
>                 return rv;
>
> @@ -412,7 +411,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
>                         pr_warn("cpufv enabled (not officially supported "
>                                 "on this model)\n");
>                 eeepc->cpufv_disabled = false;
> -               return rv;
> +               return count;
>         case 1:
>                 return -EPERM;
>         default:
> @@ -1042,10 +1041,11 @@ static ssize_t store_sys_hwmon(void (*set)(int), const char *buf, size_t count)
>  {
>         int rv, value;
>
> -       rv = parse_arg(buf, count, &value);
> -       if (rv > 0)
> -               set(value);
> -       return rv;
> +       rv = parse_arg(buf, &value);
> +       if (rv < 0)
> +               return rv;
> +       set(value);
> +       return count;
>  }
>
>  static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
> --
>
>
> Paul Bolle
>
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux