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

[Daniel P. Berrange]

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

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 at redhat.com>
> 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 :|