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

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(a)huawei.com&gt; > --- > .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; > } > .