[libvirt] [PATCH 0/4] Fixes for segmentation faults
This patch series fixes some segmentation faults. Marc Hartmayer (4): qemu: Check if virQEMUCapsNewCopy(...) has failed conf: Fix libvirtd free() segfault if virDomainChrSourceDefNew(...) fails util: reset the counters to zero rpc: Fix potentially segfaults src/conf/domain_conf.c | 2 +- src/qemu/qemu_capabilities.c | 4 ++++ src/rpc/virnetserverservice.c | 20 +++++++++++--------- src/util/virnetdevip.c | 2 ++ 4 files changed, 18 insertions(+), 10 deletions(-) -- 2.5.5
Check if virQEMUCapsNewCopy(...) has failed, thus a segmentation fault in virQEMUCapsFilterByMachineType(...) will be avoided. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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; } -- 2.5.5
On Thu, Feb 09, 2017 at 03:13 PM +0100, Marc Hartmayer <mhartmay@linux.vnet.ibm.com> wrote:
Check if virQEMUCapsNewCopy(...) has failed, thus a segmentation fault in virQEMUCapsFilterByMachineType(...) will be avoided.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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; } -- 2.5.5
Another solution would be to add a cleanup path - not sure which one is better. -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 02/09/2017 09:13 AM, Marc Hartmayer wrote:
Check if virQEMUCapsNewCopy(...) has failed, thus a segmentation fault in virQEMUCapsFilterByMachineType(...) will be avoided.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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; }
ACK (and to respond to your self-reply, I don't think a cleanup: label is necessary. The function is too simple to bother cluttering it up). I'll push these all in a minute (except 4/4, where I'm waiting for your approval to a small change)
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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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) -- 2.5.5
On 02/09/2017 09:13 AM, Marc Hartmayer wrote:
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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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)
ACK
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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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; } -- 2.5.5
On 02/09/2017 09:13 AM, Marc Hartmayer wrote:
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).
ACK, and I take full responsibility for introducing the bug :-/ (This shows the danger of believing that merely moving a chunk of code into a subordinate function that's called in place of the original code won't lead to a regression; previously it wasn't possible to call it twice on the same object, but now it is)
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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 Thu, Feb 09, 2017 at 03:13 PM +0100, Marc Hartmayer <mhartmay@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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)?
+ }
#if WITH_GNUTLS virObjectUnref(svc->tls); -- 2.5.5
-- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 02/09/2017 09:21 AM, Marc Hartmayer wrote:
On Thu, Feb 09, 2017 at 03:13 PM +0100, Marc Hartmayer <mhartmay@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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).
+ }
#if WITH_GNUTLS virObjectUnref(svc->tls); -- 2.5.5
-- Beste Grüße / Kind regards Marc Hartmayer
IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 09, 2017 at 08:17 PM +0100, Laine Stump <laine@laine.org> wrote:
On 02/09/2017 09:21 AM, Marc Hartmayer wrote:
On Thu, Feb 09, 2017 at 03:13 PM +0100, Marc Hartmayer <mhartmay@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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).
No worries from my side so you can change it - thanks. -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 02/09/2017 09:13 AM, Marc Hartmayer 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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- 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); + }
re: your other message - we shouldn't need to check for svc->socks != NULL here (see my response to your self-response). And I *think* we don't need to worry about settinc svc->nsocks = 0 here, since the dispose function can only be called once per object. Do you see any problem with me removing the "if (svc->socks)" around the for loop and VIR_FREE()? If not, I'll do that and push the result. (ACK, assuming that change is okay with you)
participants (2)
-
Laine Stump -
Marc Hartmayer