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

[Michal Privoznik]

On 12.06.2016 12:30, 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. I fix it.
> 
> Signed-off-by: Wang Yufei <james.wangyufei at huawei.com>
> ---
>  src/conf/virdomainobjlist.c |  29 ++++++++--
>  src/conf/virdomainobjlist.h |   2 +
>  src/libvirt_private.syms    |   1 +
>  src/libxl/libxl_domain.c    |  18 ++-----
>  src/libxl/libxl_domain.h    |   5 +-
>  src/libxl/libxl_driver.c    | 127 ++++++++++++++++----------------------------
>  src/libxl/libxl_migration.c |  14 ++---
>  7 files changed, 86 insertions(+), 110 deletions(-)
> 
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index 27590c8..485671e 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -112,24 +112,45 @@ static int virDomainObjListSearchID(const void *payload,
>      return want;
>  }
>  
> -
> -virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
> -                                         int id)
> +static virDomainObjPtr
> +virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
> +                                 int id,
> +                                 bool ref)
>  {
>      virDomainObjPtr obj;
>      virObjectLock(doms);
>      obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id);
> +    if (ref) {
> +        virObjectRef(obj);
> +        virObjectUnlock(doms);
> +    }
>      if (obj) {
>          virObjectLock(obj);
>          if (obj->removing) {
>              virObjectUnlock(obj);
> +            if (ref)
> +                virObjectUnref(obj);
>              obj = NULL;
>          }
>      }
> -    virObjectUnlock(doms);
> +    if (!ref)
> +        virObjectUnlock(doms);
>      return obj;
>  }
>  
> +virDomainObjPtr
> +virDomainObjListFindByID(virDomainObjListPtr doms,
> +                         int id)
> +{
> +    return virDomainObjListFindByIDInternal(doms, id, false);
> +}
> +
> +virDomainObjPtr
> +virDomainObjListFindByIDRef(virDomainObjListPtr doms,
> +                            int id)
> +{
> +    return virDomainObjListFindByIDInternal(doms, id, true);
> +}
>  

Okay, I've checked callers and it looks like this will not introduce ref
counting problem.

>  static virDomainObjPtr
>  virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
> diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
> index c0f856c..3c59bd3 100644
> --- a/src/conf/virdomainobjlist.h
> +++ b/src/conf/virdomainobjlist.h
> @@ -34,6 +34,8 @@ virDomainObjListPtr virDomainObjListNew(void);
>  
>  virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
>                                           int id);
> +virDomainObjPtr virDomainObjListFindByIDRef(virDomainObjListPtr doms,
> +                                            int id);
>  virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms,
>                                             const unsigned char *uuid);
>  virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 85b9cd1..b42e2cd 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -888,6 +888,7 @@ virDomainObjListCollect;
>  virDomainObjListConvert;
>  virDomainObjListExport;
>  virDomainObjListFindByID;
> +virDomainObjListFindByIDRef;
>  virDomainObjListFindByName;
>  virDomainObjListFindByUUID;
>  virDomainObjListFindByUUIDRef;
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 4a21f68..ec1ff51 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -123,8 +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",
>                    libxlDomainJobTypeToString(job));
> @@ -157,7 +155,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
>          virReportSystemError(errno,
>                               "%s", _("cannot acquire job mutex"));
>  
> -    virObjectUnref(obj);
>      return -1;
>  }

Finally. It just hasn't felt right for BeginJob & EndJob apis to grab a
reference when it should have been in fact driver API impl who did that.
Thank you for posting this patch.

>  
> @@ -171,7 +168,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 +180,6 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
>  
>      libxlDomainObjResetJob(priv);
>      virCondSignal(&priv->job.cond);
> -
> -    return virObjectUnref(obj);
>  }
>  

ACKed and pushed.

Michal