[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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,

[...]

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]