conf: Add check to avoid a NULL compare for SysfsPath

[libvirt] [PATCH] conf: Add check to avoid a NULL compare for SysfsPath

Posted by Cheng Lin 5 years, 3 months ago

If the two sysfs_path are both NULL, there may be an incorrect
object returned for virNodeDeviceObjListFindBySysfsPath().

This check exists in old interface virNodeDeviceFindBySysfsPath().
e.g.
virNodeDeviceFindBySysfsPath(virNodeDeviceObjListPtr devs,
                             const char *sysfs_path)
{
    ...
        if ((devs->objs[i]->def->sysfs_path != NULL) &&
            (STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) {
    ...
}

Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
---
 src/conf/virnodedeviceobj.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index c8ad131..2e40de5 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -207,7 +207,8 @@ virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
     int want = 0;
 
     virObjectLock(obj);
-    if (STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path))
+    if ((obj->def->sysfs_path != NULL) &&
+        (STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path)))
         want = 1;
     virObjectUnlock(obj);
     return want;
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: Add check to avoid a NULL compare for SysfsPath

Posted by Cole Robinson 5 years, 3 months ago

On 01/18/2019 02:49 AM, Cheng Lin wrote:
> If the two sysfs_path are both NULL, there may be an incorrect
> object returned for virNodeDeviceObjListFindBySysfsPath().
> 
> This check exists in old interface virNodeDeviceFindBySysfsPath().
> e.g.
> virNodeDeviceFindBySysfsPath(virNodeDeviceObjListPtr devs,
>                              const char *sysfs_path)
> {
>     ...
>         if ((devs->objs[i]->def->sysfs_path != NULL) &&
>             (STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) {
>     ...
> }
> 
> Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> ---
>  src/conf/virnodedeviceobj.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index c8ad131..2e40de5 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -207,7 +207,8 @@ virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
>      int want = 0;
>  
>      virObjectLock(obj);
> -    if (STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path))
> +    if ((obj->def->sysfs_path != NULL) &&
> +        (STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path)))
>          want = 1;
>      virObjectUnlock(obj);
>      return want;
> 

Thanks, I reformatted to this and pushed:

@@ -207,7 +207,8 @@ virNodeDeviceObjListFindBySysfsPathCallback(const
void *payload,
     int want = 0;

     virObjectLock(obj);
-    if (STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path))
+    if (obj->def->sysfs_path &&
+        STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path))
         want = 1;
     virObjectUnlock(obj);
     return want;

What kind of devices are you seeing that don't have a sysfs_path I?
Seems like there's lots of places internally we expect it to be there,
but some others where we don't, so people must have hit similar issues
before, but I don't think I see an example in the udev output on my machine

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt][PATCH] conf: Add check to avoid a NULL compare forSysfsPath

Posted by cheng.lin130@zte.com.cn 5 years, 2 months ago

> On 01/18/2019 02:49 AM, Cheng Lin wrote:
> > If the two sysfs_path are both NULL, there may be an incorrect
> > object returned for virNodeDeviceObjListFindBySysfsPath().
> >
> > This check exists in old interface virNodeDeviceFindBySysfsPath().
> > e.g.
> > virNodeDeviceFindBySysfsPath(virNodeDeviceObjListPtr devs,
> >                              const char *sysfs_path)
> > {
> >     ...
> >         if ((devs->objs[i]->def->sysfs_path != NULL) &&
> >             (STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) {
> >     ...
> > }
> >
> > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
> > ---
> >  src/conf/virnodedeviceobj.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> > index c8ad131..2e40de5 100644
> > --- a/src/conf/virnodedeviceobj.c
> > +++ b/src/conf/virnodedeviceobj.c
> > @@ -207,7 +207,8 @@ virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
> >      int want = 0;
> >
> >      virObjectLock(obj);
> -    if (STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path))
> > +    if ((obj->def->sysfs_path != NULL) &&
> > +        (STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path)))
> >          want = 1;
> >      virObjectUnlock(obj);
> >      return want;
> >
> 
> Thanks, I reformatted to this and pushed:
> 
> @@ -207,7 +207,8 @@ virNodeDeviceObjListFindBySysfsPathCallback(const
> void *payload,
> int want = 0;
> 
> virObjectLock(obj);
> -    if (STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path))
> +    if (obj->def->sysfs_path &&
> +        STREQ_NULLABLE(obj->def->sysfs_path, sysfs_path))
> want = 1;
> virObjectUnlock(obj);
> return want;
> 
> What kind of devices are you seeing that don't have a sysfs_path I?
> Seems like there's lots of places internally we expect it to be there,
> but some others where we don't, so people must have hit similar issues
> before, but I don't think I see an example in the udev output on my machine

The new interface handles null parameters differently from the old one. 
Potential risks may be introduced.

> - Cole--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list