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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list