[libvirt PATCH] fix build by NULL string handling

[Mishal Shah]

On Mon, Apr 6, 2020 at 4:39 PM Daniel P. Berrangé <berrange at 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 at 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 :|
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200406/981f39e7/attachment-0001.htm>