[libvirt] [PATCH 3/3] parallels: fix possible crash in case of errors in prlsdkLoadDomain

Maxim Nestratov mnestratov at virtuozzo.com
Thu May 21 13:09:28 UTC 2015


21.05.2015 10:29, Dmitry Guryanov пишет:
> Sorry for top-posting,
> Check code of virDomainObjListAdd. It assigns xmlopt->privateData->free to dom->privateDataFreeFunc.
I don't see why we can't do it manually, while problem the patch fixes 
is a bit different. We create three objects within this function: 'def', 
'privdom', and 'dom'. On error path depending on the place where we 
failed, we need to use different scenarios to cleanup: delete all we 
allocated and not to do it twice. The code without the patch frees 'def' 
and 'pdom 'structures twice.
In other words, if we didn't manage to assign 'pdom' to 'dom' because we 
failed earlier we should call privateDataFree function manully, and if 
we didn't set 'def' to 'dom', either new or existed one, we should free 
it manually too. Otherwise cleanup will be made by 
virDomainObjListRemove. Using xmlopt can't fix described problems.
>
> ________________________________________
> От: Maxim Nestratov
> Отправлено: 21 мая 2015 г. 10:17
> Кому: Dmitry Guryanov; Maxim Nestratov; libvir-list at 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 at parallels.com>
>>> ---
>>>    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;
>>>    }




More information about the libvir-list mailing list