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