mdev: Fix mingw build by adding a check for non-NULL pointer

[libvirt] [PATCH] mdev: Fix mingw build by adding a check for non-NULL pointer

Posted by Erik Skultety 6 years, 11 months ago

This patch fixes the following MinGW error (although actually being a
false positive):

../../src/util/virmdev.c: In function 'virMediatedDeviceListMarkDevices':
../../src/util/virmdev.c:453:21: error: potential null pointer
dereference [-Werror=null-dereference]
          const char *mdev_path = mdev->path;
                      ^~~~~~~~~

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
Pushed under the build breaker rule.

Erik

 src/util/virmdev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index c861d21..174f48c 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -449,9 +449,13 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst,
 
     virObjectLock(dst);
     for (i = 0; i < count; i++) {
+        const char *mdev_path = NULL;
         virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i);
-        const char *mdev_path = mdev->path;
 
+        if (!mdev)
+            goto cleanup;
+
+        mdev_path = mdev->path;
         if (virMediatedDeviceIsUsed(mdev, dst) ||
             virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0)
             goto cleanup;
-- 
2.9.3

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

Re: [libvirt] [PATCH] mdev: Fix mingw build by adding a check for non-NULL pointer

Posted by Peter Krempa 6 years, 11 months ago

On Thu, May 04, 2017 at 13:32:35 +0200, Erik Skultety wrote:
> This patch fixes the following MinGW error (although actually being a
> false positive):
> 
> ../../src/util/virmdev.c: In function 'virMediatedDeviceListMarkDevices':
> ../../src/util/virmdev.c:453:21: error: potential null pointer
> dereference [-Werror=null-dereference]
>           const char *mdev_path = mdev->path;
>                       ^~~~~~~~~
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
> Pushed under the build breaker rule.
> 
> Erik
> 
>  src/util/virmdev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index c861d21..174f48c 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -449,9 +449,13 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst,
>  
>      virObjectLock(dst);
>      for (i = 0; i < count; i++) {
> +        const char *mdev_path = NULL;
>          virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i);

This does not set an error, ...

> -        const char *mdev_path = mdev->path;
>  
> +        if (!mdev)
> +            goto cleanup;

so this function will sometimes report an error and sometimes will not.

> +
> +        mdev_path = mdev->path;
>          if (virMediatedDeviceIsUsed(mdev, dst) ||
>              virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0)

E.g.: Both of these report an error.

>              goto cleanup;
> -- 
> 2.9.3
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] mdev: Fix mingw build by adding a check for non-NULL pointer

Posted by Erik Skultety 6 years, 11 months ago

On Thu, May 04, 2017 at 01:41:19PM +0200, Peter Krempa wrote:
> On Thu, May 04, 2017 at 13:32:35 +0200, Erik Skultety wrote:
> > This patch fixes the following MinGW error (although actually being a
> > false positive):
> >
> > ../../src/util/virmdev.c: In function 'virMediatedDeviceListMarkDevices':
> > ../../src/util/virmdev.c:453:21: error: potential null pointer
> > dereference [-Werror=null-dereference]
> >           const char *mdev_path = mdev->path;
> >                       ^~~~~~~~~
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> > Pushed under the build breaker rule.
> >
> > Erik
> >
> >  src/util/virmdev.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> > index c861d21..174f48c 100644
> > --- a/src/util/virmdev.c
> > +++ b/src/util/virmdev.c
> > @@ -449,9 +449,13 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst,
> >
> >      virObjectLock(dst);
> >      for (i = 0; i < count; i++) {
> > +        const char *mdev_path = NULL;
> >          virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i);
>
> This does not set an error, ...
>
> > -        const char *mdev_path = mdev->path;
> >
> > +        if (!mdev)
> > +            goto cleanup;
>
> so this function will sometimes report an error and sometimes will not.
>

Sigh..Although, as I wrote, it's a false positive. I could send another patch,
but I'm not sure I want to pollute the repo shortly before a release. Besides,
per a private conversation, it might eventually be better to move the DEBUG
message up, before actually adding the device into the list, so that we still
have a valid pointer. That way, we don't need the local variable I've
introduced only recently and it also builds with mingw, although, not sure why,
since debug is enabled, so it should fail within the DEBUG macro as well.

Erik

> > +
> > +        mdev_path = mdev->path;
> >          if (virMediatedDeviceIsUsed(mdev, dst) ||
> >              virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0)
>
> E.g.: Both of these report an error.
>
> >              goto cleanup;
> > --
> > 2.9.3
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list



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

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