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

[Peter Krempa]

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

>>> +    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;


Peter



>
> Daniel