Don't forget to unlock domain on error path Signed-off-by: Maxim Nestratov <mnestratov(a)parallels.com&gt; --- src/parallels/parallels_sdk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 786e0ad..faae1ae 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1443,8 +1443,10 @@ prlsdkAddDomain(parallelsConnPtr privconn, const unsigned char *uuid) } sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid); - if (sdkdom == PRL_INVALID_HANDLE) + if (sdkdom == PRL_INVALID_HANDLE) { + virObjectUnlock(dom); return NULL; + } dom = prlsdkLoadDomain(privconn, sdkdom, NULL); PrlHandle_Free(sdkdom);

It is better to get all necessary parameters and check them on newly created configuration before actually creating a domain with them or applying them to an existing domain.

Signed-off-by: Maxim Nestratov <mnestratov(a)parallels.com&gt; --- src/parallels/parallels_sdk.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index faae1ae..5c15e94 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1312,6 +1312,9 @@ prlsdkLoadDomain(parallelsConnPtr privconn, *s = '\0'; } + pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart); + prlsdkCheckRetGoto(pret, error); + if (virDomainDefAddImplicitControllers(def) < 0) goto error; @@ -1349,15 +1352,6 @@ prlsdkLoadDomain(parallelsConnPtr privconn, dom->privateDataFreeFunc = prlsdkDomObjFreePrivate; dom->persistent = 1; - if (prlsdkGetDomainState(sdkdom, &domainState) < 0) - goto error; - - if (prlsdkConvertDomainState(domainState, envId, dom) < 0) - goto error; - - pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart); - prlsdkCheckRetGoto(pret, error); - switch (autostart) { case PAO_VM_START_ON_LOAD: dom->autostart = 1; @@ -1371,6 +1365,12 @@ prlsdkLoadDomain(parallelsConnPtr privconn, goto error; } + if (prlsdkGetDomainState(sdkdom, &domainState) < 0) + goto error; +

+ if (prlsdkConvertDomainState(domainState, envId, dom) < 0) + goto error; + if (!pdom->sdkdom) { pret = PrlHandle_AddRef(sdkdom); prlsdkCheckRetGoto(pret, error);

On 05/18/2015 05:44 PM, Maxim Nestratov wrote: > Cleanup code in prlsdkLoadDomain doesn't take into account the fact > if private domain freeing function is assigned or not. In case it is, > we shouldn't call it manually because virDomainObjListRemove calls it. > Also, allocated def structure should be freed only if it's not > assigned to domain. Otherwise it will be called twice one time by > virDomainObjListRemove and the second by prlsdkLoadDomain itself. Libvirt now can assign this pointer inside virDomainObjListAdd. I think you just need to set virDomainXMLOptionPtr->privateData->free to our prlsdkDomObjFreePrivate.

You can give virDomainXMLPrivateDataCallbacks structure as second argument of virDomainXMLOptionNew, and it will create appropriate virDomainXMLOption structure. > > Signed-off-by: Maxim Nestratov <mnestratov(a)parallels.com&gt; > --- > src/parallels/parallels_sdk.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/src/parallels/parallels_sdk.c > b/src/parallels/parallels_sdk.c > index 5c15e94..2f2574e 100644 > --- a/src/parallels/parallels_sdk.c > +++ b/src/parallels/parallels_sdk.c > @@ -1379,10 +1379,21 @@ prlsdkLoadDomain(parallelsConnPtr privconn, > return dom; > error: > - if (dom && !olddom) > + if (dom && !olddom) { > + /* Domain isn't persistent means that we haven't yet set > + * prlsdkDomObjFreePrivate and should call it manually > + */ > + if (!dom->persistent) > + prlsdkDomObjFreePrivate(pdom); > + > virDomainObjListRemove(privconn->domains, dom); > - virDomainDefFree(def); > - prlsdkDomObjFreePrivate(pdom); > + } > + /* Delete newly allocated def only if we haven't assigned it to > domain > + * Otherwise we will end up with domain having invalid def > within it > + */ > + if (!dom) > + virDomainDefFree(def); > + > return NULL; > }

Sorry for top-posting, Check code of virDomainObjListAdd. It assigns xmlopt->privateData->free to dom->privateDataFreeFunc.

________________________________________ От: Maxim Nestratov Отправлено: 21 мая 2015 г. 10:17 Кому: Dmitry Guryanov; Maxim Nestratov; libvir-list(a)redhat.com; Dmitry Guryanov Тема: Re: [PATCH 3/3] parallels: fix possible crash in case of errors in prlsdkLoadDomain 21.05.2015 10:04, Dmitry Guryanov пишет: > > On 05/18/2015 05:44 PM, Maxim Nestratov wrote: >> Cleanup code in prlsdkLoadDomain doesn't take into account the fact >> if private domain freeing function is assigned or not. In case it is, >> we shouldn't call it manually because virDomainObjListRemove calls it. >> Also, allocated def structure should be freed only if it's not >> assigned to domain. Otherwise it will be called twice one time by >> virDomainObjListRemove and the second by prlsdkLoadDomain itself. > Libvirt now can assign this pointer inside virDomainObjListAdd. I > think you just need to set virDomainXMLOptionPtr->privateData->free to > our prlsdkDomObjFreePrivate. I didn't get you. We already assign this function to dom->privateDataFreeFunc, and seems to the right place for this function. > You can give virDomainXMLPrivateDataCallbacks structure as second > argument of virDomainXMLOptionNew, and it will create appropriate > virDomainXMLOption structure. > >> Signed-off-by: Maxim Nestratov <mnestratov(a)parallels.com&gt; >> --- >> src/parallels/parallels_sdk.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/src/parallels/parallels_sdk.c >> b/src/parallels/parallels_sdk.c >> index 5c15e94..2f2574e 100644 >> --- a/src/parallels/parallels_sdk.c >> +++ b/src/parallels/parallels_sdk.c >> @@ -1379,10 +1379,21 @@ prlsdkLoadDomain(parallelsConnPtr privconn, >> return dom; >> error: >> - if (dom && !olddom) >> + if (dom && !olddom) { >> + /* Domain isn't persistent means that we haven't yet set >> + * prlsdkDomObjFreePrivate and should call it manually >> + */ >> + if (!dom->persistent) >> + prlsdkDomObjFreePrivate(pdom); >> + >> virDomainObjListRemove(privconn->domains, dom); >> - virDomainDefFree(def); >> - prlsdkDomObjFreePrivate(pdom); >> + } >> + /* Delete newly allocated def only if we haven't assigned it to >> domain >> + * Otherwise we will end up with domain having invalid def >> within it >> + */ >> + if (!dom) >> + virDomainDefFree(def); >> + >> return NULL; >> }