[libvirt] [PATCH 7/8] conf: check for buffer errors before virBufferUse

John Ferlan jferlan at redhat.com
Wed Aug 2 14:21:30 UTC 2017


>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 7728321cb..4dc49fdb0 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>
>> [...]
>>
>>> @@ -23309,6 +23321,10 @@ static int
>>> virDomainPanicDefFormat(virBufferPtr buf,
>>>      virBufferAdjustIndent(&childrenBuf, indent + 2);
>>>      if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0)
>>>          return -1;
>>> +
>>> +    if (virBufferCheckError(&childrenBuf) < 0)
>>> +        return -1;
>>> +
>>
>> Seeing a virBufferFreeAndReset below this if condition first had me
>> thinking, well that's unnecessary; however, in actuality I think we have
>> a leak any time virBufferUse doesn't return a non zero value call
>> virBufferAddBuffer to consume the buffer.
> 
> I do not see the leak. If we made no attempt at all to use the buffer,
> nothing should have been allocated. If we tried to add something to it,
> and failed on OOM, virBufferSetError should free the content and set use
> to zero. The only possible leak would be when we try to extend the
> buffer without actually writing any content to it - but we have no
> reason to do that in an XML file, since there's always going to be
> at least the element name.
> 
> Jan
> 

Hmm.. right - I guess it's seeing the FreeAndReset in some places and
not others that got me to thinking about this and of course being
somehow convinced that there could be a leak.  Perhaps those places
where FreeAndReset is called unnecessarily could be cleaned up (they're
not wrong, but they do nothing as long as the AddBuffer was used).

John




More information about the libvir-list mailing list