[libvirt] [PATCH 1/2] qemu: Fix possible infinite loop and segfault on error path.

Peter Krempa pkrempa at redhat.com
Thu Aug 30 15:04:34 UTC 2012


On 08/30/12 16:06, Daniel Veillard wrote:
> On Thu, Aug 30, 2012 at 03:51:54PM +0200, Peter Krempa wrote:
>> virDomainVcpuPinDefCopy when the control flow reaches out of memory
>> cleanup code, the flow would end in a infinite loop as the loop variable
>> wasn't decremented.
>>
>> Also a dereference of NULL pointers was possible if allocation of the
>> Vcpu pinning definiton structure failed.
>> ---
>>   src/conf/domain_conf.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 224aec5..2dad64d 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -1496,7 +1496,7 @@ virDomainVcpuPinDefPtr *
>>   virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, int nvcpupin)
>>   {
>>       int i = 0;
>> -    virDomainVcpuPinDefPtr *ret;
>> +    virDomainVcpuPinDefPtr *ret = NULL;
>>
>>       if (VIR_ALLOC_N(ret, nvcpupin) < 0) {
>>           goto no_memory;
>> @@ -1514,11 +1514,15 @@ virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, int nvcpupin)
>>       return ret;
>>
>>   no_memory:
>> -    while (i >= 0) {
>> -        VIR_FREE(ret[i]->cpumask);
>> -        VIR_FREE(ret[i]);
>> +    if (ret) {
>> +        for ( ; i >= 0; --i) {
>> +            if (ret[i]) {
>> +                VIR_FREE(ret[i]->cpumask);
>> +                VIR_FREE(ret[i]);
>> +            }
>> +        }
>> +        VIR_FREE(ret);
>>       }
>> -    VIR_FREE(ret);
>>       virReportOOMError();
>>
>>       return NULL;
>
>    Whoops, ACK ! Please push
>
> BTW I'm unclear what our prefered style is for
>
> if (ret) {
>      ...
>      VIR_FREE(ret);

This alternative saves the one function call and the identical check, so 
I tend to use it rather than the other one

> }
>
> vs
>
> if (ret) {
>      ...
> }
> VIR_FREE(ret);
>
> because that pattern appears twice in the replacement code.
> To me that's fine, but since we avoid if (ret) VIR_FREE(ret);
> I'm asking :-)
>
> Daniel
>

Pushed. Thanks!

Peter




More information about the libvir-list mailing list