Re: [libvirt PATCH] fix build by NULL string handling

On Mon, Apr 6, 2020 at 3:57 PM Daniel P. Berrangé <berrange(a)redhat.com&gt; 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.