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

[Ján Tomko]

On Tue, Aug 01, 2017 at 05:45:10PM -0400, John Ferlan wrote:
[...]

>> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>> index 0f99f3096..db7efffdf 100644
>> --- a/src/conf/capabilities.c
>> +++ b/src/conf/capabilities.c
>> @@ -930,6 +930,11 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>>                                bank->controls[j]->max_allocation);
>>          }
>>
>
>One could move the VIR_FREE(cpus_str) to right here and avoid the two
>

One has pushed that as a separate patch:
https://www.redhat.com/archives/libvir-list/2017-August/msg00088.html

>It also seems this particular function uses virBufferAdjustIndent on buf
>instead of on the child buf (or in this case controlBuf)...
>

While inconsistent, that does not concern me.

>> +        if (virBufferCheckError(&controlBuf) < 0) {
>> +            VIR_FREE(cpus_str);
>> +            return -1;
>> +        }
>> +
>>          if (virBufferUse(&controlBuf)) {
>>              virBufferAddLit(buf, ">\n");
>>              virBufferAddBuffer(buf, &controlBuf);
>
>[...]
>
>> 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

>
>Not necessarily something to stop this patch, but perhaps a leak for:
>
>virDomainDiskDefFormat
>virDomainControllerDriverFormat
>virDomainControllerDefFormat
>virDomainFSDefFormat
>virDomainMemballoonDefFormat
>virDomainRNGDefFormat
>virDomainVideoDefFormat
>virDomainInputDefFormat
>virDomainIOMMUDefFormat
>virDomainDiskDefFormat
>
>
>>      if (virBufferUse(&childrenBuf)) {
>>          virBufferAddLit(buf, ">\n");
>>          virBufferAddBuffer(buf, &childrenBuf);
>> @@ -23655,6 +23671,9 @@ virDomainInputDefFormat(virBufferPtr buf,
>
>[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170802/1d6e2275/attachment-0001.sig>