[libvirt] [PATCH 2/3] libxl: fix segfault in libxlReconnectDomain

[Joao Martins]

On 07/27/2016 02:42 PM, Cédric Bosdonnat wrote:
> In case of error, libxlReconnectDomain may call
> virDomainObjListRemoveLocked. However it has no local reference on
> the domain object, leading to segfault. Get a reference to the domain
> object at the start of the function and release it at the end to avoid
> problems.
> 
> This commit also factorizes code between the error and normal ends.
Also noticed in the rest of the patches, but I think you forgot to include
the SoB tag (Signed-off-by).

> ---
>  src/libxl/libxl_driver.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index cb501cf..5008c00 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -373,11 +373,13 @@ libxlReconnectDomain(virDomainObjPtr vm,
>      uint8_t *data = NULL;
>      virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
>      unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI;
> +    int ret = -1;
>  
>  #ifdef LIBXL_HAVE_PVUSB
>      hostdev_flags |= VIR_HOSTDEV_SP_USB;
>  #endif
>  
> +    virObjectRef(vm);
>      virObjectLock(vm);
>  
>      libxl_dominfo_init(&d_info);
> @@ -385,18 +387,18 @@ libxlReconnectDomain(virDomainObjPtr vm,
>      /* Does domain still exist? */
>      rc = libxl_domain_info(cfg->ctx, &d_info, vm->def->id);
>      if (rc == ERROR_INVAL) {
> -        goto out;
> +        goto error;
>      } else if (rc != 0) {
>          VIR_DEBUG("libxl_domain_info failed (code %d), ignoring domain %d",
>                    rc, vm->def->id);
> -        goto out;
> +        goto error;
>      }
>  
>      /* Is this a domain that was under libvirt control? */
>      if (libxl_userdata_retrieve(cfg->ctx, vm->def->id,
>                                  "libvirt-xml", &data, &len)) {
>          VIR_DEBUG("libxl_userdata_retrieve failed, ignoring domain %d", vm->def->id);
> -        goto out;
> +        goto error;
>      }
>  
>      /* Update domid in case it changed (e.g. reboot) while we were gone? */
> @@ -405,7 +407,7 @@ libxlReconnectDomain(virDomainObjPtr vm,
>      /* Update hostdev state */
>      if (virHostdevUpdateActiveDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
>                                              vm->def, hostdev_flags) < 0)
> -        goto out;
> +        goto error;
>  
>      if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
>          driver->inhibitCallback(true, driver->inhibitOpaque);
> @@ -413,21 +415,23 @@ libxlReconnectDomain(virDomainObjPtr vm,
>      /* Enable domain death events */
>      libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW);
After this patch it will always returns an error. Although
patch 3 of this series does add the "ret = 0" here. I think it should a part of this
patch.

>  
> + cleanup:
>      libxl_dominfo_dispose(&d_info);
>      virObjectUnlock(vm);
> +    virObjectUnref(vm);
>      virObjectUnref(cfg);
> -    return 0;
> +    return ret;
>  
> - out:
> -    libxl_dominfo_dispose(&d_info);
> + error:
>      libxlDomainCleanup(driver, vm);
> -    if (!vm->persistent)
> +    if (!vm->persistent) {
>          virDomainObjListRemoveLocked(driver->domains, vm);
> -    else
> -        virObjectUnlock(vm);
> -    virObjectUnref(cfg);
>  
> -    return -1;
> +        /* virDomainObjListRemoveLocked leaves the object unlocked,
> +         * lock it again to factorize more code. */
> +        virObjectLock(vm);
> +    }
> +    goto cleanup;
>  }
>  
>  static void
>