Check if virQEMUCapsNewCopy(...) has failed, thus a segmentation fault in virQEMUCapsFilterByMachineType(...) will be avoided. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3247d25..399e314 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4928,6 +4928,10 @@ virQEMUCapsCacheLookupCopy(virCapsPtr caps, ret = virQEMUCapsNewCopy(qemuCaps); virObjectUnref(qemuCaps); + + if (!ret) + return NULL; + virQEMUCapsFilterByMachineType(ret, machineType); return ret; }

If virDomainChrSourceDefNew(xmlopt) fails, it will lead to free()ing the uninitialized pointer bus. The fix for this is to initialize bus with NULL. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c06b128..ef883b5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13307,7 +13307,7 @@ virDomainRedirdevDefParseXML(virDomainXMLOptionPtr xmlopt, { xmlNodePtr cur; virDomainRedirdevDefPtr def; - char *bus, *type = NULL; + char *bus = NULL, *type = NULL; int remaining; if (VIR_ALLOC(def) < 0)

After freeing the data structures we have to reset the counters to zero. This fixes a segmentation fault when virNetDevIPInfoClear is called twice (e.g. this is possible in virDomainNetDefParseXML() if virDomainNetIPInfoParseXML(...) fails with ret < 0 (this leads to the first call of 'virNetDevIPInfoClear(&def->guestIP)') and the resulting call of virDomainNetDefFree(def) in the error path of virDomainNetDefParseXML() (this leads to the second call of virNetDevIPInfoClear(&def->guestIP), and finally to the segmentation fault).

Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; --- src/util/virnetdevip.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index d159760..42fbba1 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -882,10 +882,12 @@ virNetDevIPInfoClear(virNetDevIPInfoPtr ip) for (i = 0; i < ip->nips; i++) VIR_FREE(ip->ips[i]); VIR_FREE(ip->ips); + ip->nips = 0; for (i = 0; i < ip->nroutes; i++) virNetDevIPRouteFree(ip->routes[i]); VIR_FREE(ip->routes); + ip->nroutes = 0; }

We have to allocate first and if, and only if, it was successful we can set the count. A segfault has occurred in virNetServerServiceNewPostExecRestart() when VIR_ALLOC_N(svc->socks, n) has failed, but svc->nsocsk = n was already set. Thus virObejectUnref(svc) was called and therefore it was possible that virNetServerServiceDispose was called => segmentation fault. For safeness NULL pointer check were added in virNetServerServiceDispose(). Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; --- src/rpc/virnetserverservice.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 1ef0636..006d041 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -228,9 +228,9 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, svc->tls = virObjectRef(tls); #endif - svc->nsocks = 1; - if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0) + if (VIR_ALLOC_N(svc->socks, 1) < 0) goto error; + svc->nsocks = 1; if (virNetSocketNewListenUNIX(path, mask, @@ -289,9 +289,9 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, svc->tls = virObjectRef(tls); #endif - svc->nsocks = 1; - if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0) + if (VIR_ALLOC_N(svc->socks, 1) < 0) goto error; + svc->nsocks = 1; if (virNetSocketNewListenFD(fd, &svc->socks[0]) < 0) @@ -367,9 +367,9 @@ virNetServerServicePtr virNetServerServiceNewPostExecRestart(virJSONValuePtr obj goto error; } - svc->nsocks = n; - if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0) + if (VIR_ALLOC_N(svc->socks, n) < 0) goto error; + svc->nsocks = n; for (i = 0; i < svc->nsocks; i++) { virJSONValuePtr child = virJSONValueArrayGet(socks, i); @@ -492,9 +492,11 @@ void virNetServerServiceDispose(void *obj) virNetServerServicePtr svc = obj; size_t i; - for (i = 0; i < svc->nsocks; i++) - virObjectUnref(svc->socks[i]); - VIR_FREE(svc->socks); + if (svc->socks) { + for (i = 0; i < svc->nsocks; i++) + virObjectUnref(svc->socks[i]); + VIR_FREE(svc->socks);

+ } #if WITH_GNUTLS virObjectUnref(svc->tls); -- 2.5.5

On 02/09/2017 09:21 AM, Marc Hartmayer wrote: > On Thu, Feb 09, 2017 at 03:13 PM +0100, Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; wrote: >> We have to allocate first and if, and only if, it was successful we >> can set the count. A segfault has occurred in >> virNetServerServiceNewPostExecRestart() when VIR_ALLOC_N(svc->socks, >> n) has failed, but svc->nsocsk = n was already set. Thus >> virObejectUnref(svc) was called and therefore it was possible that >> virNetServerServiceDispose was called => segmentation fault. For >> safeness NULL pointer check were added in >> virNetServerServiceDispose(). >> >> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; >> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; >> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; >> --- >> src/rpc/virnetserverservice.c | 20 +++++++++++--------- >> 1 file changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c >> index 1ef0636..006d041 100644 >> --- a/src/rpc/virnetserverservice.c >> +++ b/src/rpc/virnetserverservice.c >> @@ -228,9 +228,9 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, >> svc->tls = virObjectRef(tls); >> #endif >> >> - svc->nsocks = 1; >> - if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0) >> + if (VIR_ALLOC_N(svc->socks, 1) < 0) >> goto error; >> + svc->nsocks = 1; >> >> if (virNetSocketNewListenUNIX(path, >> mask, >> @@ -289,9 +289,9 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, >> svc->tls = virObjectRef(tls); >> #endif >> >> - svc->nsocks = 1; >> - if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0) >> + if (VIR_ALLOC_N(svc->socks, 1) < 0) >> goto error; >> + svc->nsocks = 1; >> >> if (virNetSocketNewListenFD(fd, >> &svc->socks[0]) < 0) >> @@ -367,9 +367,9 @@ virNetServerServicePtr virNetServerServiceNewPostExecRestart(virJSONValuePtr obj >> goto error; >> } >> >> - svc->nsocks = n; >> - if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0) >> + if (VIR_ALLOC_N(svc->socks, n) < 0) >> goto error; >> + svc->nsocks = n; >> >> for (i = 0; i < svc->nsocks; i++) { >> virJSONValuePtr child = virJSONValueArrayGet(socks, i); >> @@ -492,9 +492,11 @@ void virNetServerServiceDispose(void *obj) >> virNetServerServicePtr svc = obj; >> size_t i; >> >> - for (i = 0; i < svc->nsocks; i++) >> - virObjectUnref(svc->socks[i]); >> - VIR_FREE(svc->socks); >> + if (svc->socks) { >> + for (i = 0; i < svc->nsocks; i++) >> + virObjectUnref(svc->socks[i]); >> + VIR_FREE(svc->socks); > Should we set svc->nsocks = 0 instead of using a NULL-pointer check (or > do both)? For consistency with other code, we should set svc->nsocks = 0. svc->socks should also be NULL if it's not pointing to anything valid, but as long as it is initialized to NULL (it is) and always freed with VIR_FREE() (it should be), then that will always be the case. When checking at vir*Free() time, we don't need the "if (svc->socks)", since nsocks == 0 will prevent the for loop from being executed, and VIR_FREE() is a NOP if the pointer is NULL. I think your patch is fine after removing the "if (svc->socks)" (the original problem was just that nsocks was set before we tried to allocate socks). Unless you see some other issue, I will make that one change and push it (I'll wait for word back from you though).