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

Dmitry Guryanov dguryanov at odin.com
Thu May 21 07:29:10 UTC 2015


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 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