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

[Daniel P. Berrange]

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>
> >
> >If the function lxcSetupLoopDevices(def,&nloopDevs,&loopDevs) failed,
> >the variable loopDevs will keep a initial NULL value, however, the
> >function VIR_FORCE_CLOSE(loopDevs[i]) will directly deref it.
> >
> >* rc/lxc/lxc_controller.c: fixed a null pointer dereference.
> >
> >Signed-off-by: Alex Jia<ajia at redhat.com>
> >---
> >  src/lxc/lxc_controller.c |    7 +++++--
> >  1 files changed, 5 insertions(+), 2 deletions(-)
> >
> >diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> >index c4e7832..024756d 100644
> >--- a/src/lxc/lxc_controller.c
> >+++ b/src/lxc/lxc_controller.c
> >@@ -1017,8 +1017,11 @@ cleanup:
> >      VIR_FORCE_CLOSE(containerhandshake[0]);
> >      VIR_FORCE_CLOSE(containerhandshake[1]);
> >
> >-    for (i = 0 ; i<  nloopDevs ; i++)
> >-        VIR_FORCE_CLOSE(loopDevs[i]);
> 
> 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

> >+    if (loopDevs) {
> >+        for (i = 0 ; i<  nloopDevs ; i++)
> >+            VIR_FORCE_CLOSE(loopDevs[i]);
> >+    }
> >+
> >      VIR_FREE(loopDevs);
> >
> >      if (container>  1) {
> 
> 
> ACK. I squashed in a fix for seting the device counter to 0 if this
> happens. (Well it will be fixed on two places at once, as
> lxcSetupLoopDevices is called only from here).
> 
> 
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 024756d..7603bc7 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -208,6 +208,7 @@ 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;
> 
> 
> 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


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 :|