[libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes

[Daniel P. Berrangé]

On Thu, Jan 28, 2021 at 02:03:25PM +0100, Michal Privoznik wrote:
> On 1/28/21 1:47 PM, Daniel P. Berrangé wrote:
> > On Thu, Jan 28, 2021 at 01:18:07PM +0100, Michal Privoznik wrote:
> > > On 1/28/21 11:44 AM, Peter Krempa wrote:
> > > > On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
> > > > > If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr"
> > > > > would return "0", indicating success, without writing to "value".
> > > > > 
> > > > > This was found by clang-tidy's
> > > > > "clang-analyzer-core.UndefinedBinaryOperatorResult" check in
> > > > > function "udevProcessCCW", flagging a read on the potentially
> > > > > uninitialized variable "online".
> > > > > 
> > > > > Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
> > > > > ---
> > > > >    src/node_device/node_device_udev.c | 5 ++++-
> > > > >    1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > > > > index 55a2731681..d5a12bab0e 100644
> > > > > --- a/src/node_device/node_device_udev.c
> > > > > +++ b/src/node_device/node_device_udev.c
> > > > > @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
> > > > >        str = udevGetDeviceSysfsAttr(udev_device, attr_name);
> > > > > -    if (str && virStrToLong_i(str, NULL, base, value) < 0) {
> > > > > +    if (!str)
> > > > > +        return -1;
> > > > 
> > > > In this case an error wouldn't be reported any more.
> > > 
> > > I think it's quite the opposite actually. Previously, if str == NULL then a
> > > zero was returned (without any error) from this function. Now you get -1.
> > > 
> > > I think we want to keep return 0 in case of !str. Callers use the following
> > > pattern:
> > > 
> > > var = -1; /* default */
> > > udevGetIntSysfsAttr(device, "attribute", &var, 10);
> > > 
> > > If "attribute" exists, @var is updated; if it doesn't it's left untouched
> > > with the default value (-1 in this case).
> > 
> > There should not be any code with this pattern because that leads us to
> > ignore genuine errors.
> 
> I was a bit too harsh in my reply. Of course we check for
> udevGetIntSysfsAttr() retval. The pattern is like this:
> 
> var = -1;
> if (udevGetIntSysfsAttr(device, "attribute", &var, 10) < 0)
>   goto error;
> 
> And I think this okay.
> 
> > 
> > If we want to degrade when an attribute isn't present, we must be explict
> > about that and test existance of the sysfs file
> 
> Well, the above example would then look like this:
> 
> var = -1;
> str = udev_device_get_sysattr_value(device, "attribute");
> if (str && virStrToLong_i(str, NULL, &var, 10) < 0) {
>   /* error */
> }
> 
> Which is exactly what udevGetIntSysfsAttr() does, except it's open coded.

Ok, so the real bug here is idevProcessCCW not initializing the
"online" variable before calling the method.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|