Re: [libvirt] [PATCH] lxc: avoid null deref on lxcSetupLoopDevices failure

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(a)redhat.com wrote: >>>From: Alex Jia<ajia(a)redhat.com> >>> >> >>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 :/

>>>+ 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(a)redhat.com&gt; 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;