Re: [libvirt] [PATCH 1/2] qemu: Fix possible infinite loop and segfault on error path.

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