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

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,