[libvirt] [PATCH] libxl: fix vm lock overwritten bug

[Jim Fehlig]

On 05/10/2016 12:18 AM, Wang Yufei wrote:
> In libxl driver we do virObjectRef in libxlDomainObjBeginJob,
> If virCondWaitUntil failed, it goes to error, do virObjectUnref,
> There's a chance that someone undefine the vm at the same time,
> and refs unref to zero, vm is freed in libxlDomainObjBeginJob.
> But the vm outside function is not Null, we do virObjectUnlock(vm).
> That's how we overwrite the vm memory after it's freed. Because the
> coding amount is much, I fix it partly in libxlDomainCreateWithFlags.
> If my opinion is right and there're no problems, I'll fix them all
> later.

I haven't looked at this patch in detail yet, but I posted a similar patch a
while back

https://www.redhat.com/archives/libvir-list/2015-June/msg00711.html

Later, Martin had some good review comments

https://www.redhat.com/archives/libvir-list/2015-July/msg00557.html

I was working on a V2 to address his comments, but found that it introduced a
race between saving and destroying a domain. The work got interrupted and sadly
I haven't found time to resume that.

Regards,
Jim

>
> Signed-off-by: Wang Yufei <james.wangyufei at huawei.com>
> ---
>  .gnulib                  | 1 -
>  src/libxl/libxl_domain.c | 6 +-----
>  src/libxl/libxl_domain.h | 2 +-
>  src/libxl/libxl_driver.c | 5 ++---
>  4 files changed, 4 insertions(+), 10 deletions(-)
>  delete mode 160000 .gnulib
>
> diff --git a/.gnulib b/.gnulib
> deleted file mode 160000
> index 6cc32c6..0000000
> --- a/.gnulib
> +++ /dev/null
> @@ -1 +0,0 @@
> -Subproject commit 6cc32c63e80bc1a30c521b2f07f2b54909b59892
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 14a900c..a90ce53 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -123,7 +123,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
>          return -1;
>      then = now + LIBXL_JOB_WAIT_TIME;
>  
> -    virObjectRef(obj);
>  
>      while (priv->job.active) {
>          VIR_DEBUG("Wait normal job condition for starting job: %s",
> @@ -157,7 +156,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
>          virReportSystemError(errno,
>                               "%s", _("cannot acquire job mutex"));
>  
> -    virObjectUnref(obj);
>      return -1;
>  }
>  
> @@ -171,7 +169,7 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
>   * non-zero, false if the reference count has dropped to zero
>   * and obj is disposed.
>   */
> -bool
> +void
>  libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
>                       virDomainObjPtr obj)
>  {
> @@ -183,8 +181,6 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
>  
>      libxlDomainObjResetJob(priv);
>      virCondSignal(&priv->job.cond);
> -
> -    return virObjectUnref(obj);
>  }
>  
>  int
> diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
> index 1c1eba3..ce28944 100644
> --- a/src/libxl/libxl_domain.h
> +++ b/src/libxl/libxl_domain.h
> @@ -85,7 +85,7 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
>                         enum libxlDomainJob job)
>      ATTRIBUTE_RETURN_CHECK;
>  
> -bool
> +void
>  libxlDomainObjEndJob(libxlDriverPrivatePtr driver,
>                       virDomainObjPtr obj)
>      ATTRIBUTE_RETURN_CHECK;
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index bf97c9c..a46994a 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -294,7 +294,7 @@ libxlDomObjFromDomain(virDomainPtr dom)
>      libxlDriverPrivatePtr driver = dom->conn->privateData;
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>  
> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
> +    vm = virDomainObjListFindByUUIDRef(driver->domains, dom->uuid);
>      if (!vm) {
>          virUUIDFormat(dom->uuid, uuidstr);
>          virReportError(VIR_ERR_NO_DOMAIN,
> @@ -2691,8 +2691,7 @@ libxlDomainCreateWithFlags(virDomainPtr dom,
>          vm = NULL;
>  
>   cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>      return ret;
>  }
>