1. Patchew
  2. Libvirt
  3. View series

[libvirt PATCH] fix build by NULL string handling

Mishal Shah posted 1 patch 4 years ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200406101936.31423-1-shahmishal1998@gmail.com
src/openvz/openvz_driver.c | 6 +++++-
tests/sockettest.c         | 8 ++++++--
2 files changed, 11 insertions(+), 3 deletions(-)
[libvirt PATCH] fix build by NULL string handling
Posted by Mishal Shah 4 years ago 
Fix libvirt build by taking care of NULL string handling

---
 src/openvz/openvz_driver.c | 6 +++++-
 tests/sockettest.c         | 8 ++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 1a189dbbe7..46f117cd00 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -2084,7 +2084,7 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
     virDomainDefPtr def = NULL;
     virDomainObjPtr vm = NULL;
     char *my_hostname = NULL;
-    const char *hostname = NULL;
+    char *hostname = NULL;
     virURIPtr uri = NULL;
     int ret = -1;
 
@@ -2149,6 +2149,10 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
         }
     }
 
+    if (hostname == NULL) {
+        goto error;
+    }
+
     *uri_out = g_strdup_printf("ssh://%s", hostname);
 
     ret = 0;
diff --git a/tests/sockettest.c b/tests/sockettest.c
index 29a565de40..f0a0815b88 100644
--- a/tests/sockettest.c
+++ b/tests/sockettest.c
@@ -188,8 +188,12 @@ static int testMaskNetwork(const char *addrstr,
         return -1;
 
     if (STRNEQ(networkstr, gotnet)) {
-        VIR_FREE(gotnet);
-        fprintf(stderr, "Expected %s, got %s\n", networkstr, gotnet);
+        if (gotnet == NULL) {
+            fprintf(stderr, "Expected %s, got empty string\n", networkstr);
+        } else {
+            fprintf(stderr, "Expected %s, got %s\n", networkstr, gotnet);
+            VIR_FREE(gotnet);
+        }
         return -1;
     }
     VIR_FREE(gotnet);
-- 
2.17.1
Re: [libvirt PATCH] fix build by NULL string handling
Posted by Daniel P. Berrangé 4 years ago 
On Mon, Apr 06, 2020 at 03:49:36PM +0530, Mishal Shah wrote:
> Fix libvirt build by taking care of NULL string handling

What build problems are you seeing without this patch ?  Our automated
CI is all working correctly and I don't think the changes make sense.

> ---
>  src/openvz/openvz_driver.c | 6 +++++-
>  tests/sockettest.c         | 8 ++++++--
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> index 1a189dbbe7..46f117cd00 100644
> --- a/src/openvz/openvz_driver.c
> +++ b/src/openvz/openvz_driver.c
> @@ -2084,7 +2084,7 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
>      virDomainDefPtr def = NULL;
>      virDomainObjPtr vm = NULL;
>      char *my_hostname = NULL;
> -    const char *hostname = NULL;
> +    char *hostname = NULL;

This string is just a copy of anoter string, so shouldn't have
const removed IMHO.

>      virURIPtr uri = NULL;
>      int ret = -1;
>  
> @@ -2149,6 +2149,10 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
>          }
>      }
>  
> +    if (hostname == NULL) {
> +        goto error;
> +    }

I don't think this is right either - what's missing is that in the
previous  if() block we should have a  "hostname = my_hostname;"
assignment.

>      *uri_out = g_strdup_printf("ssh://%s", hostname);
>  
>      ret = 0;
> diff --git a/tests/sockettest.c b/tests/sockettest.c
> index 29a565de40..f0a0815b88 100644
> --- a/tests/sockettest.c
> +++ b/tests/sockettest.c
> @@ -188,8 +188,12 @@ static int testMaskNetwork(const char *addrstr,
>          return -1;
>  
>      if (STRNEQ(networkstr, gotnet)) {
> -        VIR_FREE(gotnet);
> -        fprintf(stderr, "Expected %s, got %s\n", networkstr, gotnet);
> +        if (gotnet == NULL) {
> +            fprintf(stderr, "Expected %s, got empty string\n", networkstr);
> +        } else {
> +            fprintf(stderr, "Expected %s, got %s\n", networkstr, gotnet);
> +            VIR_FREE(gotnet);
> +        }
>          return -1;
>      }

This makese no sense either, since a few lines above we have

    if (!(gotnet = virSocketAddrFormat(&network)))
       return -1;

so it is impossible for "gotnet == NULL" to be true


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 :|
Re: [libvirt PATCH] fix build by NULL string handling
Posted by Mishal Shah 4 years ago 
On Mon, Apr 6, 2020 at 3:57 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Apr 06, 2020 at 03:49:36PM +0530, Mishal Shah wrote:
> > Fix libvirt build by taking care of NULL string handling
>
> What build problems are you seeing without this patch ?  Our automated
> CI is all working correctly and I don't think the changes make sense.
>

../../src/openvz/openvz_driver.c: In function
'openvzDomainMigratePrepare3Params':
../../src/util/glibcompat.h:33:26: error: '%s' directive argument is null
[-Werror=format-overflow=]
   33 | # define g_strdup_printf vir_g_strdup_printf
../../src/openvz/openvz_driver.c:2152:16: note: in expansion of macro
'g_strdup_printf'
 2152 |     *uri_out = g_strdup_printf("ssh://%s", hostname);
      |                ^~~~~~~~~~~~~~~

This is the error that comes on compiling build. I'm using Ubuntu 18.04 &
GCC version 9.2.1


>
> > ---
> >  src/openvz/openvz_driver.c | 6 +++++-
> >  tests/sockettest.c         | 8 ++++++--
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> > index 1a189dbbe7..46f117cd00 100644
> > --- a/src/openvz/openvz_driver.c
> > +++ b/src/openvz/openvz_driver.c
> > @@ -2084,7 +2084,7 @@ openvzDomainMigratePrepare3Params(virConnectPtr
> dconn,
> >      virDomainDefPtr def = NULL;
> >      virDomainObjPtr vm = NULL;
> >      char *my_hostname = NULL;
> > -    const char *hostname = NULL;
> > +    char *hostname = NULL;
>
> This string is just a copy of anoter string, so shouldn't have
> const removed IMHO.
>
> >      virURIPtr uri = NULL;
> >      int ret = -1;
> >
> > @@ -2149,6 +2149,10 @@ openvzDomainMigratePrepare3Params(virConnectPtr
> dconn,
> >          }
> >      }
> >
> > +    if (hostname == NULL) {
> > +        goto error;
> > +    }
>
> I don't think this is right either - what's missing is that in the
> previous  if() block we should have a  "hostname = my_hostname;"
> assignment.
>

Yes, I think this assignment should have been there, otherwise, the string
hostname stays NULL and throws an error in the build.


>
> >      *uri_out = g_strdup_printf("ssh://%s", hostname);
> >
> >      ret = 0;
> > diff --git a/tests/sockettest.c b/tests/sockettest.c
> > index 29a565de40..f0a0815b88 100644
> > --- a/tests/sockettest.c
> > +++ b/tests/sockettest.c
> > @@ -188,8 +188,12 @@ static int testMaskNetwork(const char *addrstr,
> >          return -1;
> >
> >      if (STRNEQ(networkstr, gotnet)) {
> > -        VIR_FREE(gotnet);
> > -        fprintf(stderr, "Expected %s, got %s\n", networkstr, gotnet);
> > +        if (gotnet == NULL) {
> > +            fprintf(stderr, "Expected %s, got empty string\n",
> networkstr);
> > +        } else {
> > +            fprintf(stderr, "Expected %s, got %s\n", networkstr,
> gotnet);
> > +            VIR_FREE(gotnet);
> > +        }
> >          return -1;
> >      }
>
> This makese no sense either, since a few lines above we have
>
>     if (!(gotnet = virSocketAddrFormat(&network)))
>        return -1;
>
> so it is impossible for "gotnet == NULL" to be true
>

Yes, but I think because VIR_FREE was called before the fprintf statement,
gotnet became NULL and was throwing an error in the build.

I can make the necessary changes if required.


>
>
> 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 :|
>
>
Re: [libvirt PATCH] fix build by NULL string handling
Posted by Daniel P. Berrangé 4 years ago 
On Mon, Apr 06, 2020 at 04:16:48PM +0530, Mishal Shah wrote:
> On Mon, Apr 6, 2020 at 3:57 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, Apr 06, 2020 at 03:49:36PM +0530, Mishal Shah wrote:
> > > Fix libvirt build by taking care of NULL string handling
> >
> > What build problems are you seeing without this patch ?  Our automated
> > CI is all working correctly and I don't think the changes make sense.
> >
> 
> ../../src/openvz/openvz_driver.c: In function
> 'openvzDomainMigratePrepare3Params':
> ../../src/util/glibcompat.h:33:26: error: '%s' directive argument is null
> [-Werror=format-overflow=]
>    33 | # define g_strdup_printf vir_g_strdup_printf
> ../../src/openvz/openvz_driver.c:2152:16: note: in expansion of macro
> 'g_strdup_printf'
>  2152 |     *uri_out = g_strdup_printf("ssh://%s", hostname);
>       |                ^~~~~~~~~~~~~~~
> 
> This is the error that comes on compiling build. I'm using Ubuntu 18.04 &
> GCC version 9.2.1

Have you made any local changes to libvirt code, or are you passing any
special flags to configure, or custom env ?

We test the build on Ubuntu 18.04 and don't see these warnings

eg this is the latest build log

  https://gitlab.com/libvirt/libvirt/-/jobs/497900982

> 
> 
> >
> > > ---
> > >  src/openvz/openvz_driver.c | 6 +++++-
> > >  tests/sockettest.c         | 8 ++++++--
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> > > index 1a189dbbe7..46f117cd00 100644
> > > --- a/src/openvz/openvz_driver.c
> > > +++ b/src/openvz/openvz_driver.c
> > > @@ -2084,7 +2084,7 @@ openvzDomainMigratePrepare3Params(virConnectPtr
> > dconn,
> > >      virDomainDefPtr def = NULL;
> > >      virDomainObjPtr vm = NULL;
> > >      char *my_hostname = NULL;
> > > -    const char *hostname = NULL;
> > > +    char *hostname = NULL;
> >
> > This string is just a copy of anoter string, so shouldn't have
> > const removed IMHO.
> >
> > >      virURIPtr uri = NULL;
> > >      int ret = -1;
> > >
> > > @@ -2149,6 +2149,10 @@ openvzDomainMigratePrepare3Params(virConnectPtr
> > dconn,
> > >          }
> > >      }
> > >
> > > +    if (hostname == NULL) {
> > > +        goto error;
> > > +    }
> >
> > I don't think this is right either - what's missing is that in the
> > previous  if() block we should have a  "hostname = my_hostname;"
> > assignment.
> >
> 
> Yes, I think this assignment should have been there, otherwise, the string
> hostname stays NULL and throws an error in the build.
> 
> 
> >
> > >      *uri_out = g_strdup_printf("ssh://%s", hostname);
> > >
> > >      ret = 0;
> > > diff --git a/tests/sockettest.c b/tests/sockettest.c
> > > index 29a565de40..f0a0815b88 100644
> > > --- a/tests/sockettest.c
> > > +++ b/tests/sockettest.c
> > > @@ -188,8 +188,12 @@ static int testMaskNetwork(const char *addrstr,
> > >          return -1;
> > >
> > >      if (STRNEQ(networkstr, gotnet)) {
> > > -        VIR_FREE(gotnet);
> > > -        fprintf(stderr, "Expected %s, got %s\n", networkstr, gotnet);
> > > +        if (gotnet == NULL) {
> > > +            fprintf(stderr, "Expected %s, got empty string\n",
> > networkstr);
> > > +        } else {
> > > +            fprintf(stderr, "Expected %s, got %s\n", networkstr,
> > gotnet);
> > > +            VIR_FREE(gotnet);
> > > +        }
> > >          return -1;
> > >      }
> >
> > This makese no sense either, since a few lines above we have
> >
> >     if (!(gotnet = virSocketAddrFormat(&network)))
> >        return -1;
> >
> > so it is impossible for "gotnet == NULL" to be true
> >
> 
> Yes, but I think because VIR_FREE was called before the fprintf statement,
> gotnet became NULL and was throwing an error in the build.
> 
> I can make the necessary changes if required.

Oh yes, the order needs to be reversed


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 :|
Re: [libvirt PATCH] fix build by NULL string handling
Posted by Mishal Shah 4 years ago 
On Mon, Apr 6, 2020 at 4:39 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Apr 06, 2020 at 04:16:48PM +0530, Mishal Shah wrote:
> > On Mon, Apr 6, 2020 at 3:57 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Mon, Apr 06, 2020 at 03:49:36PM +0530, Mishal Shah wrote:
> > > > Fix libvirt build by taking care of NULL string handling
> > >
> > > What build problems are you seeing without this patch ?  Our automated
> > > CI is all working correctly and I don't think the changes make sense.
> > >
> >
> > ../../src/openvz/openvz_driver.c: In function
> > 'openvzDomainMigratePrepare3Params':
> > ../../src/util/glibcompat.h:33:26: error: '%s' directive argument is null
> > [-Werror=format-overflow=]
> >    33 | # define g_strdup_printf vir_g_strdup_printf
> > ../../src/openvz/openvz_driver.c:2152:16: note: in expansion of macro
> > 'g_strdup_printf'
> >  2152 |     *uri_out = g_strdup_printf("ssh://%s", hostname);
> >       |                ^~~~~~~~~~~~~~~
> >
> > This is the error that comes on compiling build. I'm using Ubuntu 18.04 &
> > GCC version 9.2.1
>
> Have you made any local changes to libvirt code, or are you passing any
> special flags to configure, or custom env ?
>
> We test the build on Ubuntu 18.04 and don't see these warnings
>
> eg this is the latest build log
>
>   https://gitlab.com/libvirt/libvirt/-/jobs/497900982


No, I haven't passed any special flags. The issue is caused because of the
GCC version and not Ubuntu version IMO.

I found two links to support:

   - Similar issue: https://bugzilla.redhat.com/show_bug.cgi?id=1670377
   - Fix: https://gitlab.gnome.org/GNOME/glib/-/merge_requests/626


>
> >
> >
> > >
> > > > ---
> > > >  src/openvz/openvz_driver.c | 6 +++++-
> > > >  tests/sockettest.c         | 8 ++++++--
> > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> > > > index 1a189dbbe7..46f117cd00 100644
> > > > --- a/src/openvz/openvz_driver.c
> > > > +++ b/src/openvz/openvz_driver.c
> > > > @@ -2084,7 +2084,7 @@ openvzDomainMigratePrepare3Params(virConnectPtr
> > > dconn,
> > > >      virDomainDefPtr def = NULL;
> > > >      virDomainObjPtr vm = NULL;
> > > >      char *my_hostname = NULL;
> > > > -    const char *hostname = NULL;
> > > > +    char *hostname = NULL;
> > >
> > > This string is just a copy of anoter string, so shouldn't have
> > > const removed IMHO.
> > >
> > > >      virURIPtr uri = NULL;
> > > >      int ret = -1;
> > > >
> > > > @@ -2149,6 +2149,10 @@
> openvzDomainMigratePrepare3Params(virConnectPtr
> > > dconn,
> > > >          }
> > > >      }
> > > >
> > > > +    if (hostname == NULL) {
> > > > +        goto error;
> > > > +    }
> > >
> > > I don't think this is right either - what's missing is that in the
> > > previous  if() block we should have a  "hostname = my_hostname;"
> > > assignment.
> > >
> >
> > Yes, I think this assignment should have been there, otherwise, the
> string
> > hostname stays NULL and throws an error in the build.
> >
> >
> > >
> > > >      *uri_out = g_strdup_printf("ssh://%s", hostname);
> > > >
> > > >      ret = 0;
> > > > diff --git a/tests/sockettest.c b/tests/sockettest.c
> > > > index 29a565de40..f0a0815b88 100644
> > > > --- a/tests/sockettest.c
> > > > +++ b/tests/sockettest.c
> > > > @@ -188,8 +188,12 @@ static int testMaskNetwork(const char *addrstr,
> > > >          return -1;
> > > >
> > > >      if (STRNEQ(networkstr, gotnet)) {
> > > > -        VIR_FREE(gotnet);
> > > > -        fprintf(stderr, "Expected %s, got %s\n", networkstr,
> gotnet);
> > > > +        if (gotnet == NULL) {
> > > > +            fprintf(stderr, "Expected %s, got empty string\n",
> > > networkstr);
> > > > +        } else {
> > > > +            fprintf(stderr, "Expected %s, got %s\n", networkstr,
> > > gotnet);
> > > > +            VIR_FREE(gotnet);
> > > > +        }
> > > >          return -1;
> > > >      }
> > >
> > > This makese no sense either, since a few lines above we have
> > >
> > >     if (!(gotnet = virSocketAddrFormat(&network)))
> > >        return -1;
> > >
> > > so it is impossible for "gotnet == NULL" to be true
> > >
> >
> > Yes, but I think because VIR_FREE was called before the fprintf
> statement,
> > gotnet became NULL and was throwing an error in the build.
> >
> > I can make the necessary changes if required.
>
> Oh yes, the order needs to be reversed
>
>
> 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 :|
>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.