[libvirt] [PATCH 0/2] Fix segfault and infinite loop when doing VCPU pinning
See patch comments for details. Peter Krempa (2): qemu: Fix possible infinite loop and segfault on error path. vcpupin: Fix returning of arrays from virDomainVcpuPinAdd src/conf/domain_conf.c | 22 +++++++++++++--------- src/conf/domain_conf.h | 2 +- src/libxl/libxl_driver.c | 2 +- src/qemu/qemu_driver.c | 6 +++--- src/xen/xend_internal.c | 2 +- 5 files changed, 19 insertions(+), 15 deletions(-) -- 1.7.12
virDomainVcpuPinDefCopy when the control flow reaches out of memory cleanup code, the flow would end in a infinite loop as the loop variable wasn't decremented. Also a dereference of NULL pointers was possible if allocation of the Vcpu pinning definiton structure failed. --- src/conf/domain_conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 224aec5..2dad64d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1496,7 +1496,7 @@ virDomainVcpuPinDefPtr * virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, int nvcpupin) { int i = 0; - virDomainVcpuPinDefPtr *ret; + virDomainVcpuPinDefPtr *ret = NULL; if (VIR_ALLOC_N(ret, nvcpupin) < 0) { goto no_memory; @@ -1514,11 +1514,15 @@ virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, int nvcpupin) return ret; no_memory: - while (i >= 0) { - VIR_FREE(ret[i]->cpumask); - VIR_FREE(ret[i]); + if (ret) { + for ( ; i >= 0; --i) { + if (ret[i]) { + VIR_FREE(ret[i]->cpumask); + VIR_FREE(ret[i]); + } + } + VIR_FREE(ret); } - VIR_FREE(ret); virReportOOMError(); return NULL; -- 1.7.12
On Thu, Aug 30, 2012 at 03:51:54PM +0200, Peter Krempa wrote:
virDomainVcpuPinDefCopy when the control flow reaches out of memory cleanup code, the flow would end in a infinite loop as the loop variable wasn't decremented.
Also a dereference of NULL pointers was possible if allocation of the Vcpu pinning definiton structure failed. --- src/conf/domain_conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 224aec5..2dad64d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1496,7 +1496,7 @@ virDomainVcpuPinDefPtr * virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, int nvcpupin) { int i = 0; - virDomainVcpuPinDefPtr *ret; + virDomainVcpuPinDefPtr *ret = NULL;
if (VIR_ALLOC_N(ret, nvcpupin) < 0) { goto no_memory; @@ -1514,11 +1514,15 @@ virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, int nvcpupin) return ret;
no_memory: - while (i >= 0) { - VIR_FREE(ret[i]->cpumask); - VIR_FREE(ret[i]); + if (ret) { + for ( ; i >= 0; --i) { + if (ret[i]) { + VIR_FREE(ret[i]->cpumask); + VIR_FREE(ret[i]); + } + } + VIR_FREE(ret); } - VIR_FREE(ret); virReportOOMError();
return NULL;
Whoops, ACK ! Please push BTW I'm unclear what our prefered style is for if (ret) { ... VIR_FREE(ret); } vs if (ret) { ... } VIR_FREE(ret); because that pattern appears twice in the replacement code. To me that's fine, but since we avoid if (ret) VIR_FREE(ret); I'm asking :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/30/12 16:06, Daniel Veillard wrote:
On Thu, Aug 30, 2012 at 03:51:54PM +0200, Peter Krempa wrote:
virDomainVcpuPinDefCopy when the control flow reaches out of memory cleanup code, the flow would end in a infinite loop as the loop variable wasn't decremented.
Also a dereference of NULL pointers was possible if allocation of the Vcpu pinning definiton structure failed. --- src/conf/domain_conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 224aec5..2dad64d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1496,7 +1496,7 @@ virDomainVcpuPinDefPtr * virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, int nvcpupin) { int i = 0; - virDomainVcpuPinDefPtr *ret; + virDomainVcpuPinDefPtr *ret = NULL;
if (VIR_ALLOC_N(ret, nvcpupin) < 0) { goto no_memory; @@ -1514,11 +1514,15 @@ virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, int nvcpupin) return ret;
no_memory: - while (i >= 0) { - VIR_FREE(ret[i]->cpumask); - VIR_FREE(ret[i]); + if (ret) { + for ( ; i >= 0; --i) { + if (ret[i]) { + VIR_FREE(ret[i]->cpumask); + VIR_FREE(ret[i]); + } + } + VIR_FREE(ret); } - VIR_FREE(ret); virReportOOMError();
return NULL;
Whoops, ACK ! Please push
BTW I'm unclear what our prefered style is for
if (ret) { ... VIR_FREE(ret);
This alternative saves the one function call and the identical check, so I tend to use it rather than the other one
}
vs
if (ret) { ... } VIR_FREE(ret);
because that pattern appears twice in the replacement code. To me that's fine, but since we avoid if (ret) VIR_FREE(ret); I'm asking :-)
Daniel
Pushed. Thanks! Peter
virDomainVcpuPinAdd does a realloc on vcpupin_list if the new vcpu pin definition doesn't fit into the array. The list is an array of pointers but the function definition didn't support returning the changed pointer to the caller if it was realloced. This caused segfaults if realloc would change the base pointer. --- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 2 +- src/libxl/libxl_driver.c | 2 +- src/qemu/qemu_driver.c | 6 +++--- src/xen/xend_internal.c | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2dad64d..554298d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11037,7 +11037,7 @@ cleanup: return bitmap; } -int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr *vcpupin_list, +int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, int *nvcpupin, unsigned char *cpumap, int maplen, @@ -11052,7 +11052,7 @@ int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr *vcpupin_list, if ((cpumask = bitmapFromBytemap(cpumap, maplen)) == NULL) return -1; - vcpupin = virDomainVcpuPinFindByVcpu(vcpupin_list, + vcpupin = virDomainVcpuPinFindByVcpu(*vcpupin_list, *nvcpupin, vcpu); if (vcpupin) { @@ -11073,14 +11073,14 @@ int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr *vcpupin_list, vcpupin->cpumask = cpumask; - if (VIR_REALLOC_N(vcpupin_list, *nvcpupin + 1) < 0) { + if (VIR_REALLOC_N(*vcpupin_list, *nvcpupin + 1) < 0) { virReportOOMError(); VIR_FREE(cpumask); VIR_FREE(vcpupin); return -1; } - vcpupin_list[(*nvcpupin)++] = vcpupin; + (*vcpupin_list)[(*nvcpupin)++] = vcpupin; return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9ee57e1..dfdae49 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1885,7 +1885,7 @@ int virDomainCpuSetParse(const char *str, char *virDomainCpuSetFormat(char *cpuset, int maxcpu); -int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr *vcpupin_list, +int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, int *nvcpupin, unsigned char *cpumap, int maplen, diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d8ecf13..1638314 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2461,7 +2461,7 @@ libxlDomainPinVcpu(virDomainPtr dom, unsigned int vcpu, unsigned char *cpumap, } vm->def->cputune.nvcpupin = 0; } - if (virDomainVcpuPinAdd(vm->def->cputune.vcpupin, + if (virDomainVcpuPinAdd(&vm->def->cputune.vcpupin, &vm->def->cputune.nvcpupin, cpumap, maplen, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7c0a5c3..5670ca0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3782,7 +3782,7 @@ qemudDomainPinVcpuFlags(virDomainPtr dom, newVcpuPinNum = 0; } - if (virDomainVcpuPinAdd(newVcpuPin, &newVcpuPinNum, cpumap, maplen, vcpu) < 0) { + if (virDomainVcpuPinAdd(&newVcpuPin, &newVcpuPinNum, cpumap, maplen, vcpu) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update vcpupin")); virDomainVcpuPinDefFree(newVcpuPin, newVcpuPinNum); @@ -3849,7 +3849,7 @@ qemudDomainPinVcpuFlags(virDomainPtr dom, } persistentDef->cputune.nvcpupin = 0; } - if (virDomainVcpuPinAdd(persistentDef->cputune.vcpupin, + if (virDomainVcpuPinAdd(&persistentDef->cputune.vcpupin, &persistentDef->cputune.nvcpupin, cpumap, maplen, @@ -4042,7 +4042,7 @@ qemudDomainPinEmulator(virDomainPtr dom, newVcpuPinNum = 0; } - if (virDomainVcpuPinAdd(newVcpuPin, &newVcpuPinNum, cpumap, maplen, -1) < 0) { + if (virDomainVcpuPinAdd(&newVcpuPin, &newVcpuPinNum, cpumap, maplen, -1) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update vcpupin")); virDomainVcpuPinDefFree(newVcpuPin, newVcpuPinNum); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 99def42..984f040 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2303,7 +2303,7 @@ xenDaemonDomainPinVcpu(virDomainPtr domain, unsigned int vcpu, } def->cputune.nvcpupin = 0; } - if (virDomainVcpuPinAdd(def->cputune.vcpupin, + if (virDomainVcpuPinAdd(&def->cputune.vcpupin, &def->cputune.nvcpupin, cpumap, maplen, -- 1.7.12
On Thu, Aug 30, 2012 at 03:51:55PM +0200, Peter Krempa wrote:
virDomainVcpuPinAdd does a realloc on vcpupin_list if the new vcpu pin definition doesn't fit into the array. The list is an array of pointers but the function definition didn't support returning the changed pointer to the caller if it was realloced. This caused segfaults if realloc would change the base pointer. --- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 2 +- src/libxl/libxl_driver.c | 2 +- src/qemu/qemu_driver.c | 6 +++--- src/xen/xend_internal.c | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2dad64d..554298d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11037,7 +11037,7 @@ cleanup: return bitmap; }
-int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr *vcpupin_list, +int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, int *nvcpupin, unsigned char *cpumap, int maplen, @@ -11052,7 +11052,7 @@ int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr *vcpupin_list, if ((cpumask = bitmapFromBytemap(cpumap, maplen)) == NULL) return -1;
- vcpupin = virDomainVcpuPinFindByVcpu(vcpupin_list, + vcpupin = virDomainVcpuPinFindByVcpu(*vcpupin_list, *nvcpupin, vcpu); if (vcpupin) { @@ -11073,14 +11073,14 @@ int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr *vcpupin_list, vcpupin->cpumask = cpumask;
- if (VIR_REALLOC_N(vcpupin_list, *nvcpupin + 1) < 0) { + if (VIR_REALLOC_N(*vcpupin_list, *nvcpupin + 1) < 0) { virReportOOMError(); VIR_FREE(cpumask); VIR_FREE(vcpupin); return -1; }
- vcpupin_list[(*nvcpupin)++] = vcpupin; + (*vcpupin_list)[(*nvcpupin)++] = vcpupin;
return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9ee57e1..dfdae49 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1885,7 +1885,7 @@ int virDomainCpuSetParse(const char *str, char *virDomainCpuSetFormat(char *cpuset, int maxcpu);
-int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr *vcpupin_list, +int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, int *nvcpupin, unsigned char *cpumap, int maplen, diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d8ecf13..1638314 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2461,7 +2461,7 @@ libxlDomainPinVcpu(virDomainPtr dom, unsigned int vcpu, unsigned char *cpumap, } vm->def->cputune.nvcpupin = 0; } - if (virDomainVcpuPinAdd(vm->def->cputune.vcpupin, + if (virDomainVcpuPinAdd(&vm->def->cputune.vcpupin, &vm->def->cputune.nvcpupin, cpumap, maplen, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7c0a5c3..5670ca0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3782,7 +3782,7 @@ qemudDomainPinVcpuFlags(virDomainPtr dom, newVcpuPinNum = 0; }
- if (virDomainVcpuPinAdd(newVcpuPin, &newVcpuPinNum, cpumap, maplen, vcpu) < 0) { + if (virDomainVcpuPinAdd(&newVcpuPin, &newVcpuPinNum, cpumap, maplen, vcpu) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update vcpupin")); virDomainVcpuPinDefFree(newVcpuPin, newVcpuPinNum); @@ -3849,7 +3849,7 @@ qemudDomainPinVcpuFlags(virDomainPtr dom, } persistentDef->cputune.nvcpupin = 0; } - if (virDomainVcpuPinAdd(persistentDef->cputune.vcpupin, + if (virDomainVcpuPinAdd(&persistentDef->cputune.vcpupin, &persistentDef->cputune.nvcpupin, cpumap, maplen, @@ -4042,7 +4042,7 @@ qemudDomainPinEmulator(virDomainPtr dom, newVcpuPinNum = 0; }
- if (virDomainVcpuPinAdd(newVcpuPin, &newVcpuPinNum, cpumap, maplen, -1) < 0) { + if (virDomainVcpuPinAdd(&newVcpuPin, &newVcpuPinNum, cpumap, maplen, -1) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update vcpupin")); virDomainVcpuPinDefFree(newVcpuPin, newVcpuPinNum); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 99def42..984f040 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2303,7 +2303,7 @@ xenDaemonDomainPinVcpu(virDomainPtr domain, unsigned int vcpu, } def->cputune.nvcpupin = 0; } - if (virDomainVcpuPinAdd(def->cputune.vcpupin, + if (virDomainVcpuPinAdd(&def->cputune.vcpupin, &def->cputune.nvcpupin, cpumap, maplen,
Nice catch ! ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/30/12 16:09, Daniel Veillard wrote:
On Thu, Aug 30, 2012 at 03:51:55PM +0200, Peter Krempa wrote:
virDomainVcpuPinAdd does a realloc on vcpupin_list if the new vcpu pin definition doesn't fit into the array. The list is an array of pointers but the function definition didn't support returning the changed pointer to the caller if it was realloced. This caused segfaults if realloc would change the base pointer. --- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 2 +- src/libxl/libxl_driver.c | 2 +- src/qemu/qemu_driver.c | 6 +++--- src/xen/xend_internal.c | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-)
Nice catch ! ACK,
Daniel
Pushed. Thanks! Peter
participants (2)
-
Daniel Veillard -
Peter Krempa