[PATCH 00/17] conf: Fix and prevent uninitialized memory use with new virXMLProp* helpers
Compilers aren't able to see that the value passed via a pointer from the new virXMLProp helpers may be uninitialized in certain cases. Fix 3 such cases, prepare the code and then ensure that the new virXMLProp* helpers always initialize the memory. CI pipeline (once it finishes) can be viewed at: https://gitlab.com/pipo.sk/libvirt/-/pipelines/298562552 Peter Krempa (17): util: xml: Extract implementation of xml property -> enum parsing to a common helper virXMLPropULongLong: Always initialize @result virDomainVcpuParse: Assign default vcpus count based on return value of virXMLPropUInt virDomainDiskDefDriverParseXML: Fix usage of virXMLPropUInt virXMLPropUInt: Always initialize @result conf: Define autoptr func for virDomainIOThreadIDDef virDomainIOThreadIDDefParseXML: Refactor cleanup virXMLPropInt: Always initialize '@result' virDomainBackupDiskDefParseXML: Fill default backup state after parsing it virXMLPropTristateBool: Always initialize '@result' conf: domain: Don't initialize virTristateBool local variables used for virXMLPropTristateBool virXMLPropTristateSwitch: Always initialize '@result' virDomainAudioCommonParse: Fix parsing of 'format' virDomainVideoDefParseXML: Fix parsing of 'backend' util: xml: Introduce virXMLPropEnumDefault conf: domain: Convert virXMLPropEnum to virXMLPropEnumDefault where we set defaults virXMLPropEnum: Always initialize '@result' src/conf/backup_conf.c | 5 +- src/conf/domain_conf.c | 176 ++++++++++++++++++++------------------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/util/virxml.c | 156 +++++++++++++++++++--------------- src/util/virxml.h | 14 +++- 6 files changed, 198 insertions(+), 155 deletions(-) -- 2.30.2
virXMLPropTristateBool/virXMLPropTristateSwitch/virXMLPropEnum can be implemented using the same internal code. Extract it into a new function called virXMLPropEnumInternal, which will also simplify adding versions of these functions with a custom default value. This way we'll be able to always initialize @result so that unused value bugs can be prevented. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.c | 108 ++++++++++++++++++---------------------------- 1 file changed, 42 insertions(+), 66 deletions(-) diff --git a/src/util/virxml.c b/src/util/virxml.c index 7cc73ab9a0..9929064993 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -557,6 +557,40 @@ virXMLNodeContentString(xmlNodePtr node) return ret; } +static int +virXMLPropEnumInternal(xmlNodePtr node, + const char* name, + int (*strToInt)(const char*), + virXMLPropFlags flags, + unsigned int *result) + +{ + g_autofree char *tmp = NULL; + int ret; + + if (!(tmp = virXMLPropString(node, name))) { + if (!(flags & VIR_XML_PROP_REQUIRED)) + return 0; + + virReportError(VIR_ERR_XML_ERROR, + _("Missing required attribute '%s' in element '%s'"), + name, node->name); + return -1; + } + + ret = strToInt(tmp); + if (ret < 0 || + ((flags & VIR_XML_PROP_NONZERO) && (ret == 0))) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid value for attribute '%s' in element '%s': '%s'."), + name, node->name, NULLSTR(tmp)); + return -1; + } + + *result = ret; + return 1; +} + /** * virXMLPropTristateBool: @@ -577,28 +611,10 @@ virXMLPropTristateBool(xmlNodePtr node, virXMLPropFlags flags, virTristateBool *result) { - g_autofree char *tmp = NULL; - int val; - - if (!(tmp = virXMLPropString(node, name))) { - if (!(flags & VIR_XML_PROP_REQUIRED)) - return 0; - - virReportError(VIR_ERR_XML_ERROR, - _("Missing required attribute '%s' in element '%s'"), - name, node->name); - return -1; - } - - if ((val = virTristateBoolTypeFromString(tmp)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid value for attribute '%s' in element '%s': '%s'. Expected 'yes' or 'no'"), - name, node->name, tmp); - return -1; - } + flags |= VIR_XML_PROP_NONZERO; - *result = val; - return 1; + return virXMLPropEnumInternal(node, name, virTristateBoolTypeFromString, + flags, result); } @@ -621,28 +637,10 @@ virXMLPropTristateSwitch(xmlNodePtr node, virXMLPropFlags flags, virTristateSwitch *result) { - g_autofree char *tmp = NULL; - int val; - - if (!(tmp = virXMLPropString(node, name))) { - if (!(flags & VIR_XML_PROP_REQUIRED)) - return 0; - - virReportError(VIR_ERR_XML_ERROR, - _("Missing required attribute '%s' in element '%s'"), - name, node->name); - return -1; - } - - if ((val = virTristateSwitchTypeFromString(tmp)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid value for attribute '%s' in element '%s': '%s'. Expected 'on' or 'off'"), - name, node->name, tmp); - return -1; - } + flags |= VIR_XML_PROP_NONZERO; - *result = val; - return 1; + return virXMLPropEnumInternal(node, name, virTristateSwitchTypeFromString, + flags, result); } @@ -833,32 +831,10 @@ virXMLPropEnum(xmlNodePtr node, virXMLPropFlags flags, unsigned int *result) { - g_autofree char *tmp = NULL; - int ret; - - if (!(tmp = virXMLPropString(node, name))) { - if (!(flags & VIR_XML_PROP_REQUIRED)) - return 0; - - virReportError(VIR_ERR_XML_ERROR, - _("Missing required attribute '%s' in element '%s'"), - name, node->name); - return -1; - } - - ret = strToInt(tmp); - if (ret < 0 || - ((flags & VIR_XML_PROP_NONZERO) && (ret == 0))) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid value for attribute '%s' in element '%s': '%s'."), - name, node->name, NULLSTR(tmp)); - return -1; - } - - *result = ret; - return 1; + return virXMLPropEnumInternal(node, name, strToInt, flags, result); } + /** * virXPathBoolean: * @xpath: the XPath string to evaluate -- 2.30.2
Compilers aren't able to see whether @result is set or not and thus don't warn of a potential use of uninitialized value. Always set @result to prevent uninitialized use. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virxml.c b/src/util/virxml.c index 9929064993..707a6a2235 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -762,6 +762,7 @@ virXMLPropUInt(xmlNodePtr node, * @result: The returned value * * Convenience function to return value of an unsigned long long attribute. + * @result is initialized to 0 on error or if the element is not found. * * Returns 1 in case of success in which case @result is set, * or 0 if the attribute is not present, @@ -778,6 +779,8 @@ virXMLPropULongLong(xmlNodePtr node, int ret; unsigned long long val; + *result = 0; + if (!(tmp = virXMLPropString(node, name))) { if (!(flags & VIR_XML_PROP_REQUIRED)) return 0; -- 2.30.2
Assign the vcpu count when virXMLPropUInt returns '0' meaning that the cpu count was not present in the XML. This will allow to always initialize the value of @result in virXMLPropUInt to prevent use of uninitialized values. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f536630d72..a2dd7d649f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18030,6 +18030,7 @@ virDomainVcpuParse(virDomainDef *def, unsigned int vcpus; g_autofree char *tmp = NULL; g_autofree xmlNodePtr *nodes = NULL; + int rc; vcpus = maxvcpus = 1; @@ -18044,10 +18045,11 @@ virDomainVcpuParse(virDomainDef *def, } VIR_FREE(tmp); - vcpus = maxvcpus; - - if (virXMLPropUInt(vcpuNode, "current", 10, VIR_XML_PROP_NONE, &vcpus) < 0) + if ((rc = virXMLPropUInt(vcpuNode, "current", 10, VIR_XML_PROP_NONE, &vcpus)) < 0) { return -1; + } else if (rc == 0) { + vcpus = maxvcpus; + } def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC; if (virXMLPropEnum(vcpuNode, "placement", -- 2.30.2
VIR_XML_PROP_NONE has value of 0 so it's pointless to include it in an binary-or expression. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a2dd7d649f..baf5d31606 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8923,9 +8923,7 @@ virDomainDiskDefDriverParseXML(virDomainDiskDef *def, VIR_XML_PROP_NONZERO, &def->discard) < 0) return -1; - if (virXMLPropUInt(cur, "iothread", 10, - VIR_XML_PROP_NONE | VIR_XML_PROP_NONZERO, - &def->iothread) < 0) + if (virXMLPropUInt(cur, "iothread", 10, VIR_XML_PROP_NONZERO, &def->iothread) < 0) return -1; if ((tmp = virXMLPropString(cur, "type"))) { -- 2.30.2
Compilers aren't able to see whether @result is set or not and thus don't warn of a potential use of uninitialized value. Always set @result to prevent uninitialized use. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virxml.c b/src/util/virxml.c index 707a6a2235..a03cbf7265 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -706,6 +706,7 @@ virXMLPropInt(xmlNodePtr node, * @result: The returned value * * Convenience function to return value of an unsigned integer attribute. + * @result is initialized to 0 on error or if the element is not found. * * Returns 1 in case of success in which case @result is set, * or 0 if the attribute is not present, @@ -722,6 +723,8 @@ virXMLPropUInt(xmlNodePtr node, int ret; unsigned int val; + *result = 0; + if (!(tmp = virXMLPropString(node, name))) { if (!(flags & VIR_XML_PROP_REQUIRED)) return 0; -- 2.30.2
Register virDomainIOThreadIDDefFree to do the cleanup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 64465dd8d6..2d5462bb55 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2531,6 +2531,7 @@ struct _virDomainIOThreadIDDef { }; void virDomainIOThreadIDDefFree(virDomainIOThreadIDDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainIOThreadIDDef, virDomainIOThreadIDDefFree); struct _virDomainCputune { -- 2.30.2
Automatically free 'iothrid' and remove all the cleanup cruft. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index baf5d31606..78775bb2b3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17010,21 +17010,14 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, static virDomainIOThreadIDDef * virDomainIOThreadIDDefParseXML(xmlNodePtr node) { - virDomainIOThreadIDDef *iothrid; - - iothrid = g_new0(virDomainIOThreadIDDef, 1); + g_autoptr(virDomainIOThreadIDDef) iothrid = g_new0(virDomainIOThreadIDDef, 1); if (virXMLPropUInt(node, "id", 10, VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, &iothrid->iothread_id) < 0) - goto error; - - return iothrid; + return NULL; - error: - virDomainIOThreadIDDefFree(iothrid); - iothrid = NULL; - return iothrid; + return g_steal_pointer(&iothrid); } -- 2.30.2
Compilers aren't able to see whether @result is set or not and thus don't warn of a potential use of uninitialized value. Always set @result to prevent uninitialized use. This is done by adding a @defaultResult argument to virXMLPropInt since many places have a non-0 default. In certain cases such as in virDomainControllerDefParseXML we pass the value from the original value, which will still trigger compiler checks if unused while preserving the existing functionality of keeping the previous value. This commit fixes 3 uses of uninitialized value parsed by this function: in virDomainDiskSourceNetworkParse introduced by 38dc25989c5 in virDomainChrSourceDefParseTCP introduced by fa48004af5b in virDomainGraphicsListenDefParseXML introduced by 0b20fd3754c Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 57 +++++++++++++++++++++--------------------- src/util/virxml.c | 6 ++++- src/util/virxml.h | 3 ++- 3 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 78775bb2b3..2bc2e55ee4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8221,7 +8221,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, if (flags & VIR_DOMAIN_DEF_PARSE_STATUS) { int value; if (virXMLPropInt(node, "tlsFromConfig", 10, VIR_XML_PROP_NONE, - &value) < 0) + &value, 0) < 0) return -1; src->tlsFromConfig = !!value; } @@ -9414,7 +9414,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, g_autofree xmlNodePtr *modelNodes = NULL; int nmodelNodes = 0; int numaNode = -1; - int ports = -1; + int ports; VIR_XPATH_NODE_AUTORESTORE(ctxt) int rc; g_autofree char *idx = NULL; @@ -9494,19 +9494,23 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, if (ntargetNodes == 1) { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { if (virXMLPropInt(targetNodes[0], "chassisNr", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.chassisNr) < 0) + &def->opts.pciopts.chassisNr, + def->opts.pciopts.chassisNr) < 0) return NULL; if (virXMLPropInt(targetNodes[0], "chassis", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.chassis) < 0) + &def->opts.pciopts.chassis, + def->opts.pciopts.chassis) < 0) return NULL; if (virXMLPropInt(targetNodes[0], "port", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.port) < 0) + &def->opts.pciopts.port, + def->opts.pciopts.port) < 0) return NULL; if (virXMLPropInt(targetNodes[0], "busNr", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.busNr) < 0) + &def->opts.pciopts.busNr, + def->opts.pciopts.busNr) < 0) return NULL; if (virXMLPropTristateSwitch(targetNodes[0], "hotplug", @@ -9515,7 +9519,8 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, return NULL; if ((rc = virXMLPropInt(targetNodes[0], "index", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.targetIndex)) < 0) + &def->opts.pciopts.targetIndex, + def->opts.pciopts.targetIndex)) < 0) return NULL; if ((rc == 1) && def->opts.pciopts.targetIndex == -1) @@ -9548,7 +9553,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, return NULL; } - if ((rc = virXMLPropInt(node, "ports", 10, VIR_XML_PROP_NONE, &ports)) < 0) + if ((rc = virXMLPropInt(node, "ports", 10, VIR_XML_PROP_NONE, &ports, -1)) < 0) return NULL; if ((rc == 1) && ports < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -9559,7 +9564,8 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: { if ((rc = virXMLPropInt(node, "vectors", 10, VIR_XML_PROP_NONE, - &def->opts.vioserial.vectors)) < 0) + &def->opts.vioserial.vectors, + def->opts.vioserial.vectors)) < 0) return NULL; if ((rc == 1) && def->opts.vioserial.vectors < 0) { @@ -9630,7 +9636,8 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, break; case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS: { if ((rc = virXMLPropInt(node, "maxGrantFrames", 10, VIR_XML_PROP_NONE, - &def->opts.xenbusopts.maxGrantFrames)) < 0) + &def->opts.xenbusopts.maxGrantFrames, + def->opts.xenbusopts.maxGrantFrames)) < 0) return NULL; if ((rc == 1) && def->opts.xenbusopts.maxGrantFrames < 0) { @@ -9641,7 +9648,8 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, } if ((rc = virXMLPropInt(node, "maxEventChannels", 10, VIR_XML_PROP_NONE, - &def->opts.xenbusopts.maxEventChannels)) < 0) + &def->opts.xenbusopts.maxEventChannels, + def->opts.xenbusopts.maxEventChannels)) < 0) return NULL; if ((rc == 1) && def->opts.xenbusopts.maxEventChannels < 0) { @@ -11181,7 +11189,7 @@ virDomainChrSourceDefParseTCP(virDomainChrSourceDef *def, int tmpVal; if (virXMLPropInt(source, "tlsFromConfig", 10, VIR_XML_PROP_NONE, - &tmpVal) < 0) + &tmpVal, 0) < 0) return -1; def->data.tcp.tlsFromConfig = !!tmpVal; } @@ -12376,7 +12384,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def, if (flags & VIR_DOMAIN_DEF_PARSE_STATUS) { int tmp; - if (virXMLPropInt(node, "fromConfig", 10, VIR_XML_PROP_NONE, &tmp) < 0) + if (virXMLPropInt(node, "fromConfig", 10, VIR_XML_PROP_NONE, &tmp, 0) < 0) return -1; def->fromConfig = tmp != 0; } @@ -12519,7 +12527,7 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDef *def, } if (virXMLPropInt(node, "websocket", 10, VIR_XML_PROP_NONE, - &def->data.vnc.websocket) < 0) + &def->data.vnc.websocket, 0) < 0) return -1; if (websocketGenerated) @@ -12663,14 +12671,12 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef *def, if (virDomainGraphicsListensParseXML(def, node, ctxt, flags) < 0) return -1; - def->data.spice.port = 0; if (virXMLPropInt(node, "port", 10, VIR_XML_PROP_NONE, - &def->data.spice.port) < 0) + &def->data.spice.port, 0) < 0) return -1; - def->data.spice.tlsPort = 0; if (virXMLPropInt(node, "tlsPort", 10, VIR_XML_PROP_NONE, - &def->data.spice.tlsPort) < 0) + &def->data.spice.tlsPort, 0) < 0) return -1; if (virXMLPropTristateBool(node, "autoport", VIR_XML_PROP_NONE, @@ -13527,7 +13533,6 @@ virDomainMemballoonDefParseXML(virDomainXMLOption *xmlopt, virDomainMemballoonDef *def; VIR_XPATH_NODE_AUTORESTORE(ctxt) xmlNodePtr stats; - unsigned int period = 0; ctxt->node = node; @@ -13546,10 +13551,9 @@ virDomainMemballoonDefParseXML(virDomainXMLOption *xmlopt, &def->free_page_reporting) < 0) goto error; - def->period = period; if ((stats = virXPathNode("./stats", ctxt))) { if (virXMLPropInt(stats, "period", 0, VIR_XML_PROP_NONE, - &def->period) < 0) + &def->period, 0) < 0) goto error; if (def->period < 0) @@ -14509,8 +14513,7 @@ virDomainRedirFilterUSBDevDefParseXML(xmlNodePtr node) def = g_new0(virDomainRedirFilterUSBDevDef, 1); - def->usbClass = -1; - if (virXMLPropInt(node, "class", 0, VIR_XML_PROP_NONE, &def->usbClass) < 0) + if (virXMLPropInt(node, "class", 0, VIR_XML_PROP_NONE, &def->usbClass, -1) < 0) return NULL; if (def->usbClass != -1 && def->usbClass &~ 0xFF) { @@ -14519,12 +14522,10 @@ virDomainRedirFilterUSBDevDefParseXML(xmlNodePtr node) return NULL; } - def->vendor = -1; - if (virXMLPropInt(node, "vendor", 0, VIR_XML_PROP_NONE, &def->vendor) < 0) + if (virXMLPropInt(node, "vendor", 0, VIR_XML_PROP_NONE, &def->vendor, -1) < 0) return NULL; - def->product = -1; - if (virXMLPropInt(node, "product", 0, VIR_XML_PROP_NONE, &def->product) < 0) + if (virXMLPropInt(node, "product", 0, VIR_XML_PROP_NONE, &def->product, -1) < 0) return NULL; version = virXMLPropString(node, "version"); @@ -17868,7 +17869,7 @@ virDomainSchedulerParseCommonAttrs(xmlNodePtr node, if (*policy == VIR_PROC_POLICY_FIFO || *policy == VIR_PROC_POLICY_RR) { if (virXMLPropInt(node, "priority", 10, VIR_XML_PROP_REQUIRED, - priority) < 0) + priority, 0) < 0) return -1; } diff --git a/src/util/virxml.c b/src/util/virxml.c index a03cbf7265..449453121f 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -651,6 +651,7 @@ virXMLPropTristateSwitch(xmlNodePtr node, * @base: Number base, see strtol * @flags: Bitwise or of virXMLPropFlags * @result: The returned value + * @defaultResult: default value of @result in case the property is not found * * Convenience function to return value of an integer attribute. * @@ -663,11 +664,14 @@ virXMLPropInt(xmlNodePtr node, const char *name, int base, virXMLPropFlags flags, - int *result) + int *result, + int defaultResult) { g_autofree char *tmp = NULL; int val; + *result = defaultResult; + if (!(tmp = virXMLPropString(node, name))) { if (!(flags & VIR_XML_PROP_REQUIRED)) return 0; diff --git a/src/util/virxml.h b/src/util/virxml.h index eb92fbf94e..939d2482cb 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -123,7 +123,8 @@ virXMLPropInt(xmlNodePtr node, const char *name, int base, virXMLPropFlags flags, - int *result) + int *result, + int defaultResult) ATTRIBUTE_NONNULL(0) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); int -- 2.30.2
On 5/6/21 5:31 PM, Peter Krempa wrote:
Compilers aren't able to see whether @result is set or not and thus don't warn of a potential use of uninitialized value. Always set @result to prevent uninitialized use.
This is done by adding a @defaultResult argument to virXMLPropInt since many places have a non-0 default.
In certain cases such as in virDomainControllerDefParseXML we pass the value from the original value, which will still trigger compiler checks if unused while preserving the existing functionality of keeping the previous value.
This commit fixes 3 uses of uninitialized value parsed by this function: in virDomainDiskSourceNetworkParse introduced by 38dc25989c5 in virDomainChrSourceDefParseTCP introduced by fa48004af5b in virDomainGraphicsListenDefParseXML introduced by 0b20fd3754c
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 57 +++++++++++++++++++++--------------------- src/util/virxml.c | 6 ++++- src/util/virxml.h | 3 ++- 3 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 78775bb2b3..2bc2e55ee4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8221,7 +8221,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, if (flags & VIR_DOMAIN_DEF_PARSE_STATUS) { int value; if (virXMLPropInt(node, "tlsFromConfig", 10, VIR_XML_PROP_NONE, - &value) < 0) + &value, 0) < 0) return -1; src->tlsFromConfig = !!value; } @@ -9414,7 +9414,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, g_autofree xmlNodePtr *modelNodes = NULL; int nmodelNodes = 0; int numaNode = -1; - int ports = -1; + int ports; VIR_XPATH_NODE_AUTORESTORE(ctxt) int rc; g_autofree char *idx = NULL; @@ -9494,19 +9494,23 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, if (ntargetNodes == 1) { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { if (virXMLPropInt(targetNodes[0], "chassisNr", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.chassisNr) < 0) + &def->opts.pciopts.chassisNr, + def->opts.pciopts.chassisNr) < 0)
Ugrh, but I don't think there's much better option, unless we are willing to turn virXMLPropInt() into a macro. Something like: #define virXMLPropInt(node, name, base, flags, result) \ (virXMLPropIntImpl(node, name, base, flags, result, *result)) That way, virXMLPropInt() would stay in line with other virXMLPropXXX functions which do not take the 6th argument and just force the default. Michal
On Thu, May 06, 2021 at 19:07:59 +0200, Michal Prívozník wrote:
On 5/6/21 5:31 PM, Peter Krempa wrote:
Compilers aren't able to see whether @result is set or not and thus don't warn of a potential use of uninitialized value. Always set @result to prevent uninitialized use.
This is done by adding a @defaultResult argument to virXMLPropInt since many places have a non-0 default.
In certain cases such as in virDomainControllerDefParseXML we pass the value from the original value, which will still trigger compiler checks if unused while preserving the existing functionality of keeping the previous value.
This commit fixes 3 uses of uninitialized value parsed by this function: in virDomainDiskSourceNetworkParse introduced by 38dc25989c5 in virDomainChrSourceDefParseTCP introduced by fa48004af5b in virDomainGraphicsListenDefParseXML introduced by 0b20fd3754c
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 57 +++++++++++++++++++++--------------------- src/util/virxml.c | 6 ++++- src/util/virxml.h | 3 ++- 3 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 78775bb2b3..2bc2e55ee4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8221,7 +8221,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, if (flags & VIR_DOMAIN_DEF_PARSE_STATUS) { int value; if (virXMLPropInt(node, "tlsFromConfig", 10, VIR_XML_PROP_NONE, - &value) < 0) + &value, 0) < 0) return -1; src->tlsFromConfig = !!value; } @@ -9414,7 +9414,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, g_autofree xmlNodePtr *modelNodes = NULL; int nmodelNodes = 0; int numaNode = -1; - int ports = -1; + int ports; VIR_XPATH_NODE_AUTORESTORE(ctxt) int rc; g_autofree char *idx = NULL; @@ -9494,19 +9494,23 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, if (ntargetNodes == 1) { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { if (virXMLPropInt(targetNodes[0], "chassisNr", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.chassisNr) < 0) + &def->opts.pciopts.chassisNr, + def->opts.pciopts.chassisNr) < 0)
Ugrh, but I don't think there's much better option, unless we are willing to turn virXMLPropInt() into a macro. Something like:a
The other obvious option is to just populate it with the real values we expect as default ('-1' in this case). I didn't want to declare them in two places though. In case virXMLPropInt it's a relatively large amount of cases when '0' isn't the default thus I didn't opt for doing a version which would pick 0 as default as that would be almost pointless.
Set the backup mode to VIR_TRISTATE_BOOL_YES after virXMLPropTristateBool left it set to VIR_TRISTATE_BOOL_ABSENT. This will allow fixing virXMLPropTristateBool to always initialize @result. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/backup_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index 7f176b783f..ac92bd4f26 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -121,12 +121,13 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, return -1; } - def->backup = VIR_TRISTATE_BOOL_YES; - if (virXMLPropTristateBool(node, "backup", VIR_XML_PROP_NONE, &def->backup) < 0) return -1; + if (def->backup == VIR_TRISTATE_BOOL_ABSENT) + def->backup = VIR_TRISTATE_BOOL_YES; + /* don't parse anything else if backup is disabled */ if (def->backup == VIR_TRISTATE_BOOL_NO) return 0; -- 2.30.2
Compilers aren't able to see whether @result is set or not and thus don't warn of a potential use of uninitialized value. Always set @result to prevent uninitialized use. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/util/virxml.c b/src/util/virxml.c index 449453121f..d9bc5199b3 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -600,6 +600,8 @@ virXMLPropEnumInternal(xmlNodePtr node, * @result: The returned value * * Convenience function to return value of a yes / no attribute. + * In case when the property is missing @result is initialized to + * VIR_TRISTATE_BOOL_ABSENT. * * Returns 1 in case of success in which case @result is set, * or 0 if the attribute is not present, @@ -613,6 +615,8 @@ virXMLPropTristateBool(xmlNodePtr node, { flags |= VIR_XML_PROP_NONZERO; + *result = VIR_TRISTATE_BOOL_ABSENT; + return virXMLPropEnumInternal(node, name, virTristateBoolTypeFromString, flags, result); } -- 2.30.2
virXMLPropTristateBool already initializes the value to VIR_TRISTATE_BOOL_ABSENT so we no longer need to do that for certain local variables. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2bc2e55ee4..ea99a4c40a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12569,7 +12569,7 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDef *def, { VIR_XPATH_NODE_AUTORESTORE(ctxt) xmlNodePtr glNode; - virTristateBool fullscreen = VIR_TRISTATE_BOOL_NO; + virTristateBool fullscreen; ctxt->node = node; @@ -12640,7 +12640,7 @@ static int virDomainGraphicsDefParseXMLDesktop(virDomainGraphicsDef *def, xmlNodePtr node) { - virTristateBool fullscreen = VIR_TRISTATE_BOOL_NO; + virTristateBool fullscreen; if (virXMLPropTristateBool(node, "fullscreen", VIR_XML_PROP_NONE, &fullscreen) < 0) @@ -12662,7 +12662,7 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef *def, g_autofree xmlNodePtr *node_list = NULL; int n = 0; size_t i = 0; - virTristateBool autoport = VIR_TRISTATE_BOOL_NO; + virTristateBool autoport; xmlNodePtr cur; VIR_XPATH_NODE_AUTORESTORE(ctxt) -- 2.30.2
Compilers aren't able to see whether @result is set or not and thus don't warn of a potential use of uninitialized value. Always set @result to prevent uninitialized use. In two cases the code needed to be adjusted to preserve functionality. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 10 ++++++++-- src/util/virxml.c | 4 ++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ea99a4c40a..ce0ffc60ef 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17443,12 +17443,15 @@ virDomainFeaturesDefParse(virDomainDef *def, case VIR_DOMAIN_FEATURE_PVSPINLOCK: case VIR_DOMAIN_FEATURE_VMPORT: case VIR_DOMAIN_FEATURE_SMM: { - virTristateSwitch state = VIR_TRISTATE_SWITCH_ON; + virTristateSwitch state; if (virXMLPropTristateSwitch(nodes[i], "state", VIR_XML_PROP_NONE, &state) < 0) return -1; + if (state == VIR_TRISTATE_SWITCH_ABSENT) + state = VIR_TRISTATE_SWITCH_ON; + def->features[val] = state; break; } @@ -17770,7 +17773,7 @@ virDomainFeaturesDefParse(virDomainDef *def, return -1; for (i = 0; i < n; i++) { - virTristateSwitch state = VIR_TRISTATE_SWITCH_ON; + virTristateSwitch state; int val = virDomainProcessCapsFeatureTypeFromString((const char *)nodes[i]->name); if (val < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -17783,6 +17786,9 @@ virDomainFeaturesDefParse(virDomainDef *def, &state) < 0) return -1; + if (state == VIR_TRISTATE_SWITCH_ABSENT) + state = VIR_TRISTATE_SWITCH_ON; + def->caps_features[val] = state; } VIR_FREE(nodes); diff --git a/src/util/virxml.c b/src/util/virxml.c index d9bc5199b3..8e28629e9a 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -630,6 +630,8 @@ virXMLPropTristateBool(xmlNodePtr node, * @result: The returned value * * Convenience function to return value of an on / off attribute. + * In case when the property is missing @result is initialized to + * VIR_TRISTATE_SWITCH_ABSENT. * * Returns 1 in case of success in which case @result is set, * or 0 if the attribute is not present, @@ -643,6 +645,8 @@ virXMLPropTristateSwitch(xmlNodePtr node, { flags |= VIR_XML_PROP_NONZERO; + *result = VIR_TRISTATE_SWITCH_ABSENT; + return virXMLPropEnumInternal(node, name, virTristateSwitchTypeFromString, flags, result); } -- 2.30.2
Commit 38180f87f5b converted the code to use virXMLPropEnum unfaithfully ommitting the check where 'format' must be non-zero when parsed from the user. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ce0ffc60ef..d36ff536f4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13098,7 +13098,7 @@ virDomainAudioCommonParse(virDomainAudioIOCommon *def, if (virXMLPropEnum(settings, "format", virDomainAudioFormatTypeFromString, - VIR_XML_PROP_NONE, &def->format) < 0) + VIR_XML_PROP_NONZERO, &def->format) < 0) return -1; } -- 2.30.2
Commit 8391cfbc2dbc converted the code to use virXMLPropEnum unfaithfully ommitting the check where 'backend' must be non-zero when parsed from the user. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d36ff536f4..bddc5eee26 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14257,7 +14257,7 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt, if ((driver = virXPathNode("./driver", ctxt))) { if (virXMLPropEnum(driver, "name", virDomainVideoBackendTypeFromString, - VIR_XML_PROP_NONE, &def->backend) < 0) + VIR_XML_PROP_NONZERO, &def->backend) < 0) return NULL; if (virDomainVirtioOptionsParseXML(driver, &def->virtio) < 0) return NULL; -- 2.30.2
The helper is almost identical to virXMLPropEnum but it allows to pass a default value to initialize the result to. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 30 ++++++++++++++++++++++++++++++ src/util/virxml.h | 11 +++++++++++ 3 files changed, 42 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9f761c2c00..1b12c49018 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3550,6 +3550,7 @@ virXMLNodeToString; virXMLParseHelper; virXMLPickShellSafeComment; virXMLPropEnum; +virXMLPropEnumDefault; virXMLPropInt; virXMLPropString; virXMLPropStringLimit; diff --git a/src/util/virxml.c b/src/util/virxml.c index 8e28629e9a..3ad596b3e2 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -827,6 +827,36 @@ virXMLPropULongLong(xmlNodePtr node, } +/** + * virXMLPropEnumDefault: + * @node: XML dom node pointer + * @name: Name of the property (attribute) to get + * @strToInt: Conversion function to turn enum name to value. Expected to + * return negative value on failure. + * @flags: Bitwise or of virXMLPropFlags + * @result: The returned value + * @defaultResult: default value set to @result in case the property is missing + * + * Convenience function to return value of an enum attribute. + * + * Returns 1 in case of success in which case @result is set, + * or 0 if the attribute is not present, + * or -1 and reports an error on failure. + */ +int +virXMLPropEnumDefault(xmlNodePtr node, + const char* name, + int (*strToInt)(const char*), + virXMLPropFlags flags, + unsigned int *result, + unsigned int defaultResult) +{ + *result = defaultResult; + + return virXMLPropEnumInternal(node, name, strToInt, flags, result); +} + + /** * virXMLPropEnum: * @node: XML dom node pointer diff --git a/src/util/virxml.h b/src/util/virxml.h index 939d2482cb..ed02abd2e9 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -152,6 +152,17 @@ virXMLPropEnum(xmlNodePtr node, ATTRIBUTE_NONNULL(0) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); +int +virXMLPropEnumDefault(xmlNodePtr node, + const char* name, + int (*strToInt)(const char*), + virXMLPropFlags flags, + unsigned int *result, + unsigned int defaultResult) + ATTRIBUTE_NONNULL(0) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) + ATTRIBUTE_NONNULL(4); + + /* Internal function; prefer the macros below. */ xmlDocPtr virXMLParseHelper(int domcode, -- 2.30.2
There are few cases where we set a default value when using virXMLPropEnum which can be converted to virXMLPropEnumDefault. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 74 +++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bddc5eee26..e8632e4d73 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9083,11 +9083,9 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt, ctxt->node = node; - /* defaults */ - def->device = VIR_DOMAIN_DISK_DEVICE_DISK; - - if (virXMLPropEnum(node, "device", virDomainDiskDeviceTypeFromString, - VIR_XML_PROP_NONE, &def->device) < 0) + if (virXMLPropEnumDefault(node, "device", virDomainDiskDeviceTypeFromString, + VIR_XML_PROP_NONE, &def->device, + VIR_DOMAIN_DISK_DEVICE_DISK) < 0) return NULL; if (virXMLPropEnum(node, "model", virDomainDiskModelTypeFromString, @@ -11171,10 +11169,11 @@ virDomainChrSourceDefParseTCP(virDomainChrSourceDef *def, xmlXPathContextPtr ctxt, unsigned int flags) { - virDomainChrSourceModeType mode = VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT; + virDomainChrSourceModeType mode; - if (virXMLPropEnum(source, "mode", virDomainChrSourceModeTypeFromString, - VIR_XML_PROP_NONE, &mode) < 0) + if (virXMLPropEnumDefault(source, "mode", virDomainChrSourceModeTypeFromString, + VIR_XML_PROP_NONE, &mode, + VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT) < 0) return -1; def->data.tcp.listen = mode == VIR_DOMAIN_CHR_SOURCE_MODE_BIND; @@ -11208,10 +11207,11 @@ static int virDomainChrSourceDefParseUDP(virDomainChrSourceDef *def, xmlNodePtr source) { - virDomainChrSourceModeType mode = VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT; + virDomainChrSourceModeType mode; - if (virXMLPropEnum(source, "mode", virDomainChrSourceModeTypeFromString, - VIR_XML_PROP_NONE, &mode) < 0) + if (virXMLPropEnumDefault(source, "mode", virDomainChrSourceModeTypeFromString, + VIR_XML_PROP_NONE, &mode, + VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT) < 0) return -1; if (mode == VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT && @@ -11233,10 +11233,11 @@ virDomainChrSourceDefParseUnix(virDomainChrSourceDef *def, xmlNodePtr source, xmlXPathContextPtr ctxt) { - virDomainChrSourceModeType mode = VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT; + virDomainChrSourceModeType mode; - if (virXMLPropEnum(source, "mode", virDomainChrSourceModeTypeFromString, - VIR_XML_PROP_NONE, &mode) < 0) + if (virXMLPropEnumDefault(source, "mode", virDomainChrSourceModeTypeFromString, + VIR_XML_PROP_NONE, &mode, + VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT) < 0) return -1; def->data.nix.listen = mode == VIR_DOMAIN_CHR_SOURCE_MODE_BIND; @@ -17426,11 +17427,12 @@ virDomainFeaturesDefParse(virDomainDef *def, break; case VIR_DOMAIN_FEATURE_CAPABILITIES: { - virDomainCapabilitiesPolicy policy = VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT; + virDomainCapabilitiesPolicy policy; - if (virXMLPropEnum(nodes[i], "policy", - virDomainCapabilitiesPolicyTypeFromString, - VIR_XML_PROP_NONE, &policy) < 0) + if (virXMLPropEnumDefault(nodes[i], "policy", + virDomainCapabilitiesPolicyTypeFromString, + VIR_XML_PROP_NONE, &policy, + VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT) < 0) return -1; def->features[val] = policy; @@ -17465,10 +17467,11 @@ virDomainFeaturesDefParse(virDomainDef *def, break; case VIR_DOMAIN_FEATURE_IOAPIC: { - virDomainIOAPIC driver = VIR_DOMAIN_IOAPIC_NONE; + virDomainIOAPIC driver; - if (virXMLPropEnum(nodes[i], "driver", virDomainIOAPICTypeFromString, - VIR_XML_PROP_NONZERO, &driver) < 0) + if (virXMLPropEnumDefault(nodes[i], "driver", virDomainIOAPICTypeFromString, + VIR_XML_PROP_NONZERO, &driver, + VIR_DOMAIN_IOAPIC_NONE) < 0) return -1; def->features[val] = driver; @@ -17502,10 +17505,11 @@ virDomainFeaturesDefParse(virDomainDef *def, break; case VIR_DOMAIN_FEATURE_CFPC: { - virDomainCFPC value = VIR_DOMAIN_CFPC_NONE; + virDomainCFPC value; - if (virXMLPropEnum(nodes[i], "value", virDomainCFPCTypeFromString, - VIR_XML_PROP_NONZERO, &value) < 0) + if (virXMLPropEnumDefault(nodes[i], "value", virDomainCFPCTypeFromString, + VIR_XML_PROP_NONZERO, &value, + VIR_DOMAIN_CFPC_NONE) < 0) return -1; def->features[val] = value; @@ -17513,10 +17517,11 @@ virDomainFeaturesDefParse(virDomainDef *def, } case VIR_DOMAIN_FEATURE_SBBC: { - virDomainSBBC value = VIR_DOMAIN_SBBC_NONE; + virDomainSBBC value; - if (virXMLPropEnum(nodes[i], "value", virDomainSBBCTypeFromString, - VIR_XML_PROP_NONZERO, &value) < 0) + if (virXMLPropEnumDefault(nodes[i], "value", virDomainSBBCTypeFromString, + VIR_XML_PROP_NONZERO, &value, + VIR_DOMAIN_SBBC_NONE) < 0) return -1; def->features[val] = value; @@ -17524,10 +17529,11 @@ virDomainFeaturesDefParse(virDomainDef *def, } case VIR_DOMAIN_FEATURE_IBS: { - virDomainIBS value = VIR_DOMAIN_IBS_NONE; + virDomainIBS value; - if (virXMLPropEnum(nodes[i], "value", virDomainIBSTypeFromString, - VIR_XML_PROP_NONZERO, &value) < 0) + if (virXMLPropEnumDefault(nodes[i], "value", virDomainIBSTypeFromString, + VIR_XML_PROP_NONZERO, &value, + VIR_DOMAIN_IBS_NONE) < 0) return -1; def->features[val] = value; @@ -18049,10 +18055,10 @@ virDomainVcpuParse(virDomainDef *def, vcpus = maxvcpus; } - def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC; - if (virXMLPropEnum(vcpuNode, "placement", - virDomainCpuPlacementModeTypeFromString, - VIR_XML_PROP_NONE, &def->placement_mode) < 0) + if (virXMLPropEnumDefault(vcpuNode, "placement", + virDomainCpuPlacementModeTypeFromString, + VIR_XML_PROP_NONE, &def->placement_mode, + VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC) < 0) return -1; if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { -- 2.30.2
Compilers aren't able to see whether @result is set or not and thus don't warn of a potential use of uninitialized value. Always set @result to prevent uninitialized use. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/util/virxml.c b/src/util/virxml.c index 3ad596b3e2..8dcece704a 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -562,12 +562,15 @@ virXMLPropEnumInternal(xmlNodePtr node, const char* name, int (*strToInt)(const char*), virXMLPropFlags flags, - unsigned int *result) + unsigned int *result, + unsigned int defaultResult) { g_autofree char *tmp = NULL; int ret; + *result = defaultResult; + if (!(tmp = virXMLPropString(node, name))) { if (!(flags & VIR_XML_PROP_REQUIRED)) return 0; @@ -615,10 +618,8 @@ virXMLPropTristateBool(xmlNodePtr node, { flags |= VIR_XML_PROP_NONZERO; - *result = VIR_TRISTATE_BOOL_ABSENT; - return virXMLPropEnumInternal(node, name, virTristateBoolTypeFromString, - flags, result); + flags, result, VIR_TRISTATE_BOOL_ABSENT); } @@ -645,10 +646,8 @@ virXMLPropTristateSwitch(xmlNodePtr node, { flags |= VIR_XML_PROP_NONZERO; - *result = VIR_TRISTATE_SWITCH_ABSENT; - return virXMLPropEnumInternal(node, name, virTristateSwitchTypeFromString, - flags, result); + flags, result, VIR_TRISTATE_SWITCH_ABSENT); } @@ -851,9 +850,7 @@ virXMLPropEnumDefault(xmlNodePtr node, unsigned int *result, unsigned int defaultResult) { - *result = defaultResult; - - return virXMLPropEnumInternal(node, name, strToInt, flags, result); + return virXMLPropEnumInternal(node, name, strToInt, flags, result, defaultResult); } @@ -867,6 +864,7 @@ virXMLPropEnumDefault(xmlNodePtr node, * @result: The returned value * * Convenience function to return value of an enum attribute. + * @result is initialized to 0 on error or if the element is not found. * * Returns 1 in case of success in which case @result is set, * or 0 if the attribute is not present, @@ -879,7 +877,7 @@ virXMLPropEnum(xmlNodePtr node, virXMLPropFlags flags, unsigned int *result) { - return virXMLPropEnumInternal(node, name, strToInt, flags, result); + return virXMLPropEnumInternal(node, name, strToInt, flags, result, 0); } -- 2.30.2
On 5/6/21 5:30 PM, Peter Krempa wrote:
Compilers aren't able to see that the value passed via a pointer from the new virXMLProp helpers may be uninitialized in certain cases.
Fix 3 such cases, prepare the code and then ensure that the new virXMLProp* helpers always initialize the memory.
CI pipeline (once it finishes) can be viewed at:
https://gitlab.com/pipo.sk/libvirt/-/pipelines/298562552
Peter Krempa (17): util: xml: Extract implementation of xml property -> enum parsing to a common helper virXMLPropULongLong: Always initialize @result virDomainVcpuParse: Assign default vcpus count based on return value of virXMLPropUInt virDomainDiskDefDriverParseXML: Fix usage of virXMLPropUInt virXMLPropUInt: Always initialize @result conf: Define autoptr func for virDomainIOThreadIDDef virDomainIOThreadIDDefParseXML: Refactor cleanup virXMLPropInt: Always initialize '@result' virDomainBackupDiskDefParseXML: Fill default backup state after parsing it virXMLPropTristateBool: Always initialize '@result' conf: domain: Don't initialize virTristateBool local variables used for virXMLPropTristateBool virXMLPropTristateSwitch: Always initialize '@result' virDomainAudioCommonParse: Fix parsing of 'format' virDomainVideoDefParseXML: Fix parsing of 'backend' util: xml: Introduce virXMLPropEnumDefault conf: domain: Convert virXMLPropEnum to virXMLPropEnumDefault where we set defaults virXMLPropEnum: Always initialize '@result'
src/conf/backup_conf.c | 5 +- src/conf/domain_conf.c | 176 ++++++++++++++++++++------------------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/util/virxml.c | 156 +++++++++++++++++++--------------- src/util/virxml.h | 14 +++- 6 files changed, 198 insertions(+), 155 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa