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

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Friday, September 05, 2014 07:17:57 PM Darren Hart wrote:
> On Thu, Sep 04, 2014 at 09:08:08AM +0200, Paul Bolle 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@gmail.com>
> > > ---
> > >  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:
> 
> Thanks for resending.
> 
> > 
> > ------------>8------------
> > From: Paul Bolle <pebolle@tiscali.nl>
> > 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.
> > 
> 
> Seems reasonable.
> 
> > 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@tiscali.nl>
> > ---
> >  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)
> 
> I suppose it's harmless, but it would be more explicit to reuse rv here instead
> of value.
> 
> >  		return -EIO;
> 
> And as with Frans' version, I suggest propogating the error. We're talking about
> a missing/invalid ACPI control method name here, ENODEV seems approprirate.
> 
> Rafael, do you have a strong preference about what to return in such an event?

No, I don't, although -ENXIO could be used here too.

Rafael