[libvirt] [PATCH 0/3] conf: Fix some issues
Fallout from merging the zPCI patches. Andrea Bolognani (3): conf: Fix error flow in virDomainPCIAddressEnsureAddr() conf: Add several cleanup paths conf: Perform error checking in virDomainDeviceInfoFormat() src/conf/domain_addr.c | 16 ++- src/conf/domain_conf.c | 234 +++++++++++++++++++++++++++++------------ 2 files changed, 177 insertions(+), 73 deletions(-) -- 2.19.1
This avoids setting 'ret' multiple times, which will result in errors being masked if the first operation fails but the second one succeeds. Introduced-by: f183b87fc1dbcc6446ac3c1cef9cdd345b9725fb Spotted-by: Coverity Reported-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 3e1d767e4f..cc9ea82a33 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -940,15 +940,21 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, addrStr, flags, true)) goto cleanup; - ret = virDomainPCIAddressReserveAddrInternal(addrs, &dev->addr.pci, - flags, dev->isolationGroup, - true); + if (virDomainPCIAddressReserveAddrInternal(addrs, &dev->addr.pci, + flags, dev->isolationGroup, + true) < 0) { + goto cleanup; + } } else { - ret = virDomainPCIAddressReserveNextAddr(addrs, dev, flags, -1); + if (virDomainPCIAddressReserveNextAddr(addrs, dev, flags, -1) < 0) + goto cleanup; } dev->addr.pci.extFlags = dev->pciAddrExtFlags; - ret = virDomainPCIAddressExtensionEnsureAddr(addrs, &dev->addr.pci); + if (virDomainPCIAddressExtensionEnsureAddr(addrs, &dev->addr.pci) < 0) + goto cleanup; + + ret = 0; cleanup: VIR_FREE(addrStr); -- 2.19.1
On Fri, Nov 16, 2018 at 05:21:29PM +0100, Andrea Bolognani wrote:
This avoids setting 'ret' multiple times, which will result in errors being masked if the first operation fails but the second one succeeds.
Introduced-by: f183b87fc1dbcc6446ac3c1cef9cdd345b9725fb Spotted-by: Coverity Reported-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
In many cases, an early exit from a function would cause memory allocated by local virBuffer instances not to be released. Provide proper cleanup paths to solve the issue. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 137 ++++++++++++++++++++++++++++++----------- 1 file changed, 100 insertions(+), 37 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c3dbba6919..1d04cac104 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24353,13 +24353,14 @@ virDomainControllerDefFormat(virBufferPtr buf, const char *model = NULL; const char *modelName = NULL; virBuffer childBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; virBufferSetChildIndent(&childBuf, buf); if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected controller type %d"), def->type); - return -1; + goto cleanup; } if (def->model != -1) { @@ -24368,7 +24369,7 @@ virDomainControllerDefFormat(virBufferPtr buf, if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected model type %d"), def->model); - return -1; + goto cleanup; } } @@ -24432,7 +24433,7 @@ virDomainControllerDefFormat(virBufferPtr buf, virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected model name value %d"), def->opts.pciopts.modelName); - return -1; + goto cleanup; } virBufferAsprintf(&childBuf, "<model name='%s'/>\n", modelName); } @@ -24483,7 +24484,7 @@ virDomainControllerDefFormat(virBufferPtr buf, } if (virBufferCheckError(&childBuf) < 0) - return -1; + goto cleanup; if (virBufferUse(&childBuf)) { virBufferAddLit(buf, ">\n"); @@ -24493,6 +24494,11 @@ virDomainControllerDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } + ret = 0; + + cleanup: + virBufferFreeAndReset(&childBuf); + return 0; } @@ -24523,17 +24529,18 @@ virDomainFSDefFormat(virBufferPtr buf, const char *wrpolicy = virDomainFSWrpolicyTypeToString(def->wrpolicy); const char *src = def->src->path; virBuffer driverBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected filesystem type %d"), def->type); - return -1; + goto cleanup; } if (!accessmode) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected accessmode %d"), def->accessmode); - return -1; + goto cleanup; } @@ -24557,7 +24564,7 @@ virDomainFSDefFormat(virBufferPtr buf, virDomainVirtioOptionsFormat(&driverBuf, def->virtio); if (virBufferCheckError(&driverBuf) < 0) - return -1; + goto cleanup; if (virBufferUse(&driverBuf)) { virBufferAddLit(buf, "<driver"); @@ -24617,7 +24624,13 @@ virDomainFSDefFormat(virBufferPtr buf, } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</filesystem>\n"); - return 0; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&driverBuf); + + return ret; } @@ -25847,13 +25860,14 @@ virDomainSoundDefFormat(virBufferPtr buf, const char *model = virDomainSoundModelTypeToString(def->model); virBuffer childBuf = VIR_BUFFER_INITIALIZER; size_t i; + int ret = -1; virBufferSetChildIndent(&childBuf, buf); if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected sound model %d"), def->model); - return -1; + goto cleanup; } for (i = 0; i < def->ncodecs; i++) @@ -25862,7 +25876,7 @@ virDomainSoundDefFormat(virBufferPtr buf, virDomainDeviceInfoFormat(&childBuf, &def->info, flags); if (virBufferCheckError(&childBuf) < 0) - return -1; + goto cleanup; virBufferAsprintf(buf, "<sound model='%s'", model); if (virBufferUse(&childBuf)) { @@ -25873,6 +25887,11 @@ virDomainSoundDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } + ret = 0; + + cleanup: + virBufferFreeAndReset(&childBuf); + return 0; } @@ -25884,11 +25903,12 @@ virDomainMemballoonDefFormat(virBufferPtr buf, { const char *model = virDomainMemballoonModelTypeToString(def->model); virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected memballoon model %d"), def->model); - return -1; + goto cleanup; } virBufferAsprintf(buf, "<memballoon model='%s'", model); @@ -25909,10 +25929,9 @@ virDomainMemballoonDefFormat(virBufferPtr buf, virDomainVirtioOptionsFormat(&driverBuf, def->virtio); - if (virBufferCheckError(&driverBuf) < 0) { - virBufferFreeAndReset(&childrenBuf); - return -1; - } + if (virBufferCheckError(&driverBuf) < 0) + goto cleanup; + if (virBufferUse(&driverBuf)) { virBufferAddLit(&childrenBuf, "<driver"); virBufferAddBuffer(&childrenBuf, &driverBuf); @@ -25921,7 +25940,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, } if (virBufferCheckError(&childrenBuf) < 0) - return -1; + goto cleanup; if (!virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "/>\n"); @@ -25931,6 +25950,11 @@ virDomainMemballoonDefFormat(virBufferPtr buf, virBufferAddLit(buf, "</memballoon>\n"); } + ret = 0; + + cleanup: + virBufferFreeAndReset(&childrenBuf); + return 0; } @@ -25958,25 +25982,26 @@ virDomainWatchdogDefFormat(virBufferPtr buf, const char *model = virDomainWatchdogModelTypeToString(def->model); const char *action = virDomainWatchdogActionTypeToString(def->action); virBuffer childBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; virBufferSetChildIndent(&childBuf, buf); if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected watchdog model %d"), def->model); - return -1; + goto cleanup; } if (!action) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected watchdog action %d"), def->action); - return -1; + goto cleanup; } virDomainDeviceInfoFormat(&childBuf, &def->info, flags); if (virBufferCheckError(&childBuf) < 0) - return -1; + goto cleanup; virBufferAsprintf(buf, "<watchdog model='%s' action='%s'", model, action); @@ -25989,13 +26014,19 @@ virDomainWatchdogDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - return 0; + ret = 0; + + cleanup: + virBufferFreeAndReset(&childBuf); + + return ret; } static int virDomainPanicDefFormat(virBufferPtr buf, virDomainPanicDefPtr def) { virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; virBufferAddLit(buf, "<panic"); @@ -26007,7 +26038,7 @@ static int virDomainPanicDefFormat(virBufferPtr buf, virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0); if (virBufferCheckError(&childrenBuf) < 0) - return -1; + goto cleanup; if (virBufferUse(&childrenBuf)) { virBufferAddLit(buf, ">\n"); @@ -26016,8 +26047,13 @@ static int virDomainPanicDefFormat(virBufferPtr buf, } else { virBufferAddLit(buf, "/>\n"); } + + ret = 0; + + cleanup: virBufferFreeAndReset(&childrenBuf); - return 0; + + return ret; } static int @@ -26067,6 +26103,7 @@ virDomainRNGDefFormat(virBufferPtr buf, const char *model = virDomainRNGModelTypeToString(def->model); const char *backend = virDomainRNGBackendTypeToString(def->backend); virBuffer driverBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; virBufferAsprintf(buf, "<rng model='%s'>\n", model); virBufferAdjustIndent(buf, 2); @@ -26085,11 +26122,11 @@ virDomainRNGDefFormat(virBufferPtr buf, case VIR_DOMAIN_RNG_BACKEND_EGD: if (virDomainChrAttrsDefFormat(buf, def->source.chardev, false) < 0) - return -1; + goto cleanup; virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); if (virDomainChrSourceDefFormat(buf, def->source.chardev, flags) < 0) - return -1; + goto cleanup; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</backend>\n"); @@ -26099,7 +26136,7 @@ virDomainRNGDefFormat(virBufferPtr buf, virDomainVirtioOptionsFormat(&driverBuf, def->virtio); if (virBufferCheckError(&driverBuf) < 0) - return -1; + goto cleanup; if (virBufferUse(&driverBuf)) { virBufferAddLit(buf, "<driver"); @@ -26111,7 +26148,13 @@ virDomainRNGDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</rng>\n"); - return 0; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&driverBuf); + + return ret; } void @@ -26258,18 +26301,19 @@ virDomainVideoDefFormat(virBufferPtr buf, { const char *model = virDomainVideoTypeToString(def->type); virBuffer driverBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected video model %d"), def->type); - return -1; + goto cleanup; } virBufferAddLit(buf, "<video>\n"); virBufferAdjustIndent(buf, 2); virDomainVirtioOptionsFormat(&driverBuf, def->virtio); if (virBufferCheckError(&driverBuf) < 0) - return -1; + goto cleanup; if (virBufferUse(&driverBuf) || (def->driver && def->driver->vgaconf)) { virBufferAddLit(buf, "<driver"); if (virBufferUse(&driverBuf)) @@ -26308,7 +26352,13 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</video>\n"); - return 0; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&driverBuf); + + return ret; } static int @@ -26320,6 +26370,7 @@ virDomainInputDefFormat(virBufferPtr buf, const char *bus = virDomainInputBusTypeToString(def->bus); virBuffer childbuf = VIR_BUFFER_INITIALIZER; virBuffer driverBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; /* don't format keyboard into migratable XML for backward compatibility */ if (flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE && @@ -26331,12 +26382,12 @@ virDomainInputDefFormat(virBufferPtr buf, if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected input type %d"), def->type); - return -1; + goto cleanup; } if (!bus) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected input bus type %d"), def->bus); - return -1; + goto cleanup; } virBufferAsprintf(buf, "<input type='%s' bus='%s'", @@ -26345,7 +26396,7 @@ virDomainInputDefFormat(virBufferPtr buf, virBufferSetChildIndent(&childbuf, buf); virDomainVirtioOptionsFormat(&driverBuf, def->virtio); if (virBufferCheckError(&driverBuf) < 0) - return -1; + goto cleanup; if (virBufferUse(&driverBuf)) { virBufferAddLit(&childbuf, "<driver"); @@ -26356,7 +26407,7 @@ virDomainInputDefFormat(virBufferPtr buf, virDomainDeviceInfoFormat(&childbuf, &def->info, flags); if (virBufferCheckError(&childbuf) < 0) - return -1; + goto cleanup; if (!virBufferUse(&childbuf)) { virBufferAddLit(buf, "/>\n"); @@ -26366,7 +26417,13 @@ virDomainInputDefFormat(virBufferPtr buf, virBufferAddLit(buf, "</input>\n"); } - return 0; + ret = 0; + + cleanup: + virBufferFreeAndReset(&childbuf); + virBufferFreeAndReset(&driverBuf); + + return ret; } @@ -27028,19 +27085,20 @@ virDomainHubDefFormat(virBufferPtr buf, { const char *type = virDomainHubTypeToString(def->type); virBuffer childBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; virBufferSetChildIndent(&childBuf, buf); if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected hub type %d"), def->type); - return -1; + goto cleanup; } virDomainDeviceInfoFormat(&childBuf, &def->info, flags); if (virBufferCheckError(&childBuf) < 0) - return -1; + goto cleanup; virBufferAsprintf(buf, "<hub type='%s'", type); if (virBufferUse(&childBuf)) { @@ -27051,7 +27109,12 @@ virDomainHubDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - return 0; + ret = 0; + + cleanup: + virBufferFreeAndReset(&childBuf); + + return ret; } -- 2.19.1
On Fri, Nov 16, 2018 at 05:21:30PM +0100, Andrea Bolognani wrote:
In many cases, an early exit from a function would cause memory allocated by local virBuffer instances not to be released.
Provide proper cleanup paths to solve the issue.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 137 ++++++++++++++++++++++++++++++----------- 1 file changed, 100 insertions(+), 37 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
This patch breaks the error return value for: virDomainControllerDefFormat() virDomainSoundDefFormat() virDomainMemballoonDefFormat() Patch adds the "ret" variable but in error exit it use "return 0" statement. Actually this breaks compilation. Was this code compiled ? conf/domain_conf.c: In function 'virDomainControllerDefFormat': conf/domain_conf.c:24368:9: error: variable 'ret' set but not used [-Werror=unused-but-set-variable] int ret = -1; ^~~ CC test/libvirt_driver_test_la-test_driver.lo CC vmx/libvirt_vmx_la-vmx.lo CC vmware/libvirt_driver_vmware_la-vmware_driver.lo CC vmware/libvirt_driver_vmware_la-vmware_conf.lo conf/domain_conf.c: In function 'virDomainSoundDefFormat': conf/domain_conf.c:25882:9: error: variable 'ret' set but not used [-Werror=unused-but-set-variable] int ret = -1; ^~~ conf/domain_conf.c: In function 'virDomainMemballoonDefFormat': conf/domain_conf.c:25926:9: error: variable 'ret' set but not used [-Werror=unused-but-set-variable] int ret = -1; ^~~ On Fri, 16 Nov 2018 at 19:56, Pavel Hrdina <phrdina@redhat.com> wrote:
On Fri, Nov 16, 2018 at 05:21:30PM +0100, Andrea Bolognani wrote:
In many cases, an early exit from a function would cause memory allocated by local virBuffer instances not to be released.
Provide proper cleanup paths to solve the issue.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 137 ++++++++++++++++++++++++++++++----------- 1 file changed, 100 insertions(+), 37 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
virXMLFormatElement() might fail, but we were not checking its return value. Fixing this requires us to change virDomainDeviceInfoFormat() so that it can report an error back to the caller. Introduced-by: 0d6b87335c00451b0923ecc91d617f71e4135bf8 Spotted-by: Coverity Reported-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 97 ++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 31 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1d04cac104..8cb9b2719c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6418,13 +6418,14 @@ virDomainVirtioOptionsFormat(virBufferPtr buf, } -static void ATTRIBUTE_NONNULL(2) +static int ATTRIBUTE_NONNULL(2) virDomainDeviceInfoFormat(virBufferPtr buf, virDomainDeviceInfoPtr info, unsigned int flags) { virBuffer attrBuf = VIR_BUFFER_INITIALIZER; virBuffer childBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) { virBufferAsprintf(buf, "<boot order='%u'", info->bootIndex); @@ -6467,8 +6468,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf, } if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || - info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - return; + info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { + /* We're done here */ + ret = 0; + goto cleanup; + } virBufferAsprintf(&attrBuf, " type='%s'", virDomainDeviceAddressTypeToString(info->type)); @@ -6562,10 +6566,16 @@ virDomainDeviceInfoFormat(virBufferPtr buf, break; } - virXMLFormatElement(buf, "address", &attrBuf, &childBuf); + if (virXMLFormatElement(buf, "address", &attrBuf, &childBuf) < 0) + goto cleanup; + + ret = 0; + cleanup: virBufferFreeAndReset(&attrBuf); virBufferFreeAndReset(&childBuf); + + return ret; } static int @@ -24299,8 +24309,10 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->src->encryption && !def->src->encryptionInherited && virStorageEncryptionFormat(buf, def->src->encryption) < 0) return -1; - virDomainDeviceInfoFormat(buf, &def->info, - flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT); + if (virDomainDeviceInfoFormat(buf, &def->info, + flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0) { + return -1; + } if (virDomainDiskDefFormatPrivateData(buf, def, flags, xmlopt) < 0) return -1; @@ -24475,7 +24487,8 @@ virDomainControllerDefFormat(virBufferPtr buf, virDomainControllerDriverFormat(&childBuf, def); - virDomainDeviceInfoFormat(&childBuf, &def->info, flags); + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) + goto cleanup; if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && def->opts.pciopts.pcihole64) { @@ -24613,7 +24626,8 @@ virDomainFSDefFormat(virBufferPtr buf, if (def->readonly) virBufferAddLit(buf, "<readonly/>\n"); - virDomainDeviceInfoFormat(buf, &def->info, flags); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + goto cleanup; if (def->space_hard_limit) virBufferAsprintf(buf, "<space_hard_limit unit='bytes'>" @@ -25412,9 +25426,11 @@ virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefCoalesceFormatXML(buf, def->coalesce); - virDomainDeviceInfoFormat(buf, &def->info, - flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT - | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM); + if (virDomainDeviceInfoFormat(buf, &def->info, + flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT + | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) < 0) { + return -1; + } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</interface>\n"); @@ -25723,7 +25739,8 @@ virDomainChrDefFormat(virBufferPtr buf, if (virDomainChrTargetDefFormat(buf, def, flags) < 0) return -1; - virDomainDeviceInfoFormat(buf, &def->info, flags); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAsprintf(buf, "</%s>\n", elementName); @@ -25772,7 +25789,8 @@ virDomainSmartcardDefFormat(virBufferPtr buf, _("unexpected smartcard type %d"), def->type); goto cleanup; } - virDomainDeviceInfoFormat(&childBuf, &def->info, flags); + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) + goto cleanup; if (virBufferCheckError(&childBuf) < 0) goto cleanup; @@ -25843,7 +25861,8 @@ virDomainTPMDefFormat(virBufferPtr buf, break; } - virDomainDeviceInfoFormat(buf, &def->info, flags); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</tpm>\n"); @@ -25873,7 +25892,8 @@ virDomainSoundDefFormat(virBufferPtr buf, for (i = 0; i < def->ncodecs; i++) virDomainSoundCodecDefFormat(&childBuf, def->codecs[i]); - virDomainDeviceInfoFormat(&childBuf, &def->info, flags); + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) + goto cleanup; if (virBufferCheckError(&childBuf) < 0) goto cleanup; @@ -25922,7 +25942,8 @@ virDomainMemballoonDefFormat(virBufferPtr buf, if (def->period) virBufferAsprintf(&childrenBuf, "<stats period='%i'/>\n", def->period); - virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags); + if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0) + goto cleanup; if (def->virtio) { virBuffer driverBuf = VIR_BUFFER_INITIALIZER; @@ -25965,7 +25986,8 @@ virDomainNVRAMDefFormat(virBufferPtr buf, { virBufferAddLit(buf, "<nvram>\n"); virBufferAdjustIndent(buf, 2); - virDomainDeviceInfoFormat(buf, &def->info, flags); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</nvram>\n"); @@ -25998,7 +26020,8 @@ virDomainWatchdogDefFormat(virBufferPtr buf, goto cleanup; } - virDomainDeviceInfoFormat(&childBuf, &def->info, flags); + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) + goto cleanup; if (virBufferCheckError(&childBuf) < 0) goto cleanup; @@ -26035,7 +26058,8 @@ static int virDomainPanicDefFormat(virBufferPtr buf, virDomainPanicModelTypeToString(def->model)); virBufferSetChildIndent(&childrenBuf, buf); - virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0); + if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0) + goto cleanup; if (virBufferCheckError(&childrenBuf) < 0) goto cleanup; @@ -26087,7 +26111,8 @@ virDomainShmemDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - virDomainDeviceInfoFormat(buf, &def->info, flags); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</shmem>\n"); @@ -26144,7 +26169,8 @@ virDomainRNGDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - virDomainDeviceInfoFormat(buf, &def->info, flags); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + goto cleanup; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</rng>\n"); @@ -26271,7 +26297,8 @@ virDomainMemoryDefFormat(virBufferPtr buf, virDomainMemoryTargetDefFormat(buf, def); - virDomainDeviceInfoFormat(buf, &def->info, flags); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</memory>\n"); @@ -26348,7 +26375,8 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - virDomainDeviceInfoFormat(buf, &def->info, flags); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + goto cleanup; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</video>\n"); @@ -26404,7 +26432,8 @@ virDomainInputDefFormat(virBufferPtr buf, virBufferAddLit(&childbuf, "/>\n"); } virBufferEscapeString(&childbuf, "<source evdev='%s'/>\n", def->source.evdev); - virDomainDeviceInfoFormat(&childbuf, &def->info, flags); + if (virDomainDeviceInfoFormat(&childbuf, &def->info, flags) < 0) + goto cleanup; if (virBufferCheckError(&childbuf) < 0) goto cleanup; @@ -27004,9 +27033,11 @@ virDomainHostdevDefFormat(virBufferPtr buf, if (def->shareable) virBufferAddLit(buf, "<shareable/>\n"); - virDomainDeviceInfoFormat(buf, def->info, - flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT - | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM); + if (virDomainDeviceInfoFormat(buf, def->info, + flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT + | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) < 0) { + return -1; + } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</hostdev>\n"); @@ -27032,8 +27063,10 @@ virDomainRedirdevDefFormat(virBufferPtr buf, if (virDomainChrSourceDefFormat(buf, def->source, flags) < 0) return -1; - virDomainDeviceInfoFormat(buf, &def->info, - flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT); + if (virDomainDeviceInfoFormat(buf, &def->info, + flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0) { + return -1; + } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</redirdev>\n"); return 0; @@ -27095,7 +27128,8 @@ virDomainHubDefFormat(virBufferPtr buf, goto cleanup; } - virDomainDeviceInfoFormat(&childBuf, &def->info, flags); + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) + goto cleanup; if (virBufferCheckError(&childBuf) < 0) goto cleanup; @@ -27816,7 +27850,8 @@ virDomainVsockDefFormat(virBufferPtr buf, if (virXMLFormatElement(&childBuf, "cid", &cidAttrBuf, NULL) < 0) goto cleanup; - virDomainDeviceInfoFormat(&childBuf, &vsock->info, 0); + if (virDomainDeviceInfoFormat(&childBuf, &vsock->info, 0) < 0) + goto cleanup; if (virXMLFormatElement(buf, "vsock", &attrBuf, &childBuf) < 0) goto cleanup; -- 2.19.1
On Fri, Nov 16, 2018 at 05:21:31PM +0100, Andrea Bolognani wrote:
virXMLFormatElement() might fail, but we were not checking its return value.
Fixing this requires us to change virDomainDeviceInfoFormat() so that it can report an error back to the caller.
Introduced-by: 0d6b87335c00451b0923ecc91d617f71e4135bf8 Spotted-by: Coverity Reported-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 97 ++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 31 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (3)
-
Andrea Bolognani -
Pavel Hrdina -
Radoslaw Biernacki