After an OOM error, virBuffer* APIs set buf->use to zero.
Adding a buffer to the parent buffer only if use is non-zero
would quitely drop data on error.
Check the error beforehand to make sure buf->use is zero
because we have not attempted to add anything to it.
---
src/conf/capabilities.c | 5 +++++
src/conf/cpu_conf.c | 4 ++++
src/conf/domain_conf.c | 31 +++++++++++++++++++++++++++++--
3 files changed, 38 insertions(+), 2 deletions(-)
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);
}
+ if (virBufferCheckError(&controlBuf) < 0) {
+ VIR_FREE(cpus_str);
+ return -1;
+ }
+
if (virBufferUse(&controlBuf)) {
virBufferAddLit(buf, ">\n");
virBufferAddBuffer(buf, &controlBuf);
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index da40e9ba9..065b4df99 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -646,6 +646,10 @@ virCPUDefFormatBufFull(virBufferPtr buf,
if (virDomainNumaDefCPUFormat(&childrenBuf, numa) < 0)
goto cleanup;
+ if (virBufferCheckError(&attributeBuf) < 0 ||
+ virBufferCheckError(&childrenBuf) < 0)
+ goto cleanup;
+
/* Put it all together */
if (virBufferUse(&attributeBuf) || virBufferUse(&childrenBuf)) {
virBufferAddLit(buf, "<cpu");
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
@@ -21560,6 +21560,9 @@ virDomainDiskDefFormat(virBufferPtr buf,
virDomainVirtioOptionsFormat(&driverBuf, def->virtio);
+ if (virBufferCheckError(&driverBuf) < 0)
+ return -1;
+
if (virBufferUse(&driverBuf)) {
virBufferAddLit(buf, "<driver");
virBufferAddBuffer(buf, &driverBuf);
@@ -21744,7 +21747,7 @@ virDomainControllerDriverFormat(virBufferPtr buf,
virDomainVirtioOptionsFormat(&driverBuf, def->virtio);
- if (virBufferUse(&driverBuf)) {
+ if (virBufferError(&driverBuf) != 0 || virBufferUse(&driverBuf)) {
virBufferAddLit(buf, "<driver");
virBufferAddBuffer(buf, &driverBuf);
virBufferAddLit(buf, "/>\n");
@@ -21891,6 +21894,9 @@ virDomainControllerDefFormat(virBufferPtr buf,
"pcihole64>\n", def->opts.pciopts.pcihole64size);
}
+ if (virBufferCheckError(&childBuf) < 0)
+ return -1;
+
if (virBufferUse(&childBuf)) {
virBufferAddLit(buf, ">\n");
virBufferAddBuffer(buf, &childBuf);
@@ -21962,6 +21968,9 @@ virDomainFSDefFormat(virBufferPtr buf,
virDomainVirtioOptionsFormat(&driverBuf, def->virtio);
+ if (virBufferCheckError(&driverBuf) < 0)
+ return -1;
+
if (virBufferUse(&driverBuf)) {
virBufferAddLit(buf, "<driver");
virBufferAddBuffer(buf, &driverBuf);
@@ -23223,6 +23232,9 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
}
}
+ if (virBufferCheckError(&childrenBuf) < 0)
+ return -1;
+
if (!virBufferUse(&childrenBuf)) {
virBufferAddLit(buf, "/>\n");
} else {
@@ -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;
+
if (virBufferUse(&childrenBuf)) {
virBufferAddLit(buf, ">\n");
virBufferAddBuffer(buf, &childrenBuf);
@@ -23655,6 +23671,9 @@ virDomainInputDefFormat(virBufferPtr buf,
if (virDomainDeviceInfoFormat(&childbuf, &def->info, flags) < 0)
return -1;
+ if (virBufferCheckError(&childbuf) < 0)
+ return -1;
+
if (!virBufferUse(&childbuf)) {
virBufferAddLit(buf, "/>\n");
} else {
@@ -24596,6 +24615,9 @@ virDomainCputuneDefFormat(virBufferPtr buf,
def->iothreadids[i]->iothread_id);
}
+ if (virBufferCheckError(&childrenBuf) < 0)
+ return -1;
+
if (virBufferUse(&childrenBuf)) {
virBufferAddLit(buf, "<cputune>\n");
virBufferAddBuffer(buf, &childrenBuf);
@@ -24709,7 +24731,8 @@ virDomainIOMMUDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "<iommu model='%s'",
virDomainIOMMUModelTypeToString(iommu->model));
- if (virBufferUse(&childBuf)) {
+
+ if (virBufferError(&childBuf) != 0 || virBufferUse(&childBuf)) {
virBufferAddLit(buf, ">\n");
virBufferAddBuffer(buf, &childBuf);
virBufferAddLit(buf, "</iommu>\n");
@@ -24847,6 +24870,10 @@ virDomainDefFormatInternal(virDomainDefPtr def,
virBufferAdjustIndent(&childrenBuf, -2);
virBufferAddLit(&childrenBuf, "</device>\n");
}
+
+ if (virBufferCheckError(&childrenBuf) < 0)
+ goto error;
+
if (virBufferUse(&childrenBuf)) {
virBufferAddLit(buf, "<blkiotune>\n");
virBufferAddBuffer(buf, &childrenBuf);
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/26/2017 09:29 AM, Ján Tomko wrote:
> After an OOM error, virBuffer* APIs set buf->use to zero.
> Adding a buffer to the parent buffer only if use is non-zero
> would quitely drop data on error.
s/quitely/quietly/
>
> Check the error beforehand to make sure buf->use is zero
> because we have not attempted to add anything to it.
> ---
> src/conf/capabilities.c | 5 +++++
> src/conf/cpu_conf.c | 4 ++++
> src/conf/domain_conf.c | 31 +++++++++++++++++++++++++++++--
> 3 files changed, 38 insertions(+), 2 deletions(-)
>
Reviewed-by: John Ferlan <jferlan@redhat.com>
One nit (feel free to ignore) and one concern follows...
John
> 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
It also seems this particular function uses virBufferAdjustIndent on buf
instead of on the child buf (or in this case controlBuf)...
> + 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.
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,
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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,
>
>[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
>> >>> 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.