On Thu, Oct 27, 2011 at 10:34:36AM +0200, Peter Krempa wrote: > On 10/27/2011 10:18 AM, Daniel P. Berrange wrote: > >On Thu, Oct 27, 2011 at 10:06:09AM +0200, Peter Krempa wrote: > >>On 10/27/2011 09:18 AM, ajia@xxxxxxxxxx wrote: > >>>From: Alex Jia<ajia@xxxxxxxxxx> > >>> > >> > >>Indeed, this situation might happen if memory reallocation fails > >>after some iterations of the loop inside of lxcSetupLoopDevices, > >>leaving nloopDevs assigned to some value, but loopDevs being NULL. > > > >No it can't. If VIR_REALLOC_N fails, it guarentees that the original > >pointer is left at its original value. It can't become NULL. This is > >critical, because we need to release any FDs that were stored into > >loopDevs in earlier iterations of the loop > > > > Uh :/ The retention of the original pointer is in fact one of the primary reasons why we invented the VIR_REALLOC_N macro, as a replacement for realloc(). If you're interested, I describe the design in some more detail here http://berrange.com/posts/2008/05/23/better-living-through-api-design-low-level-memory-allocation/ > > >>>+ if (loopDevs) { > >>>+ for (i = 0 ; i< nloopDevs ; i++) > >>>+ VIR_FORCE_CLOSE(loopDevs[i]); > >> > >> > >>and pushed. > > > >This is actually *wrong*. You now leak any file descriptors that > >were stored in loopDevs prior to the VIR_REALLOC_N failure. Please > >revert this hunk > > > > Yes, if the pointer is retained, it'll definitely leak the FD's :(. > > I'm sorry for not noticing that. :( > > Reverted with: > > commit 95d3b4de714049e4b6b2033e2db9151ae11d6742 > Author: Peter Krempa <pkrempa@xxxxxxxxxx> > Date: Thu Oct 27 10:24:30 2011 +0200 > > lxc: Revert zeroing count of allocated items if VIR_REALLOC_N fails > > Previous commit clears number of items alocated in lxcSetupLoopDevices > if VIR_REALLOC_N fails. In that case, the pointer is not NULL, and > causes leaking FDs that have been allocated. > > * src/lxc/lxc_controller.c: revert zeroing array size > > diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c > index 7603bc7..024756d 100644 > --- a/src/lxc/lxc_controller.c > +++ b/src/lxc/lxc_controller.c > @@ -208,7 +208,6 @@ static int lxcSetupLoopDevices(virDomainDefPtr > def, size_t *nloopDevs, int **loo > > VIR_DEBUG("Saving loop fd %d", fd); > if (VIR_REALLOC_N(*loopDevs, *nloopDevs+1) < 0) { > - *nloopDevs = 0; > VIR_FORCE_CLOSE(fd); > virReportOOMError(); > goto cleanup; Thanks for pushing this quickly ! Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list