[PATCH 0/3] virNetDevVlanParse: Fix memleak and refactor
Peter Krempa (3): virNetDevVlanParse: Don't clear data on failure virNetDevVlanParse: Use g_autofree for temporary variables virNetDevVlanParse: Refactor cleanup src/conf/netdev_vlan_conf.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) -- 2.51.0
From: Peter Krempa <pkrempa@redhat.com> Clearing the data on failure is pointless as it's still cleared when other parts of the parser fail. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/netdev_vlan_conf.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c index 1ac66aec54..b98c4d92cf 100644 --- a/src/conf/netdev_vlan_conf.c +++ b/src/conf/netdev_vlan_conf.c @@ -122,8 +122,6 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def) cleanup: VIR_FREE(tagNodes); VIR_FREE(trunk); - if (ret < 0) - virNetDevVlanClear(def); return ret; } -- 2.51.0
From: Peter Krempa <pkrempa@redhat.com> Automatically free the variables to prevent leaks when returning from middle of the function. Fixes: 1de6fd5edb5 Closes: https://gitlab.com/libvirt/libvirt/-/issues/824 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/netdev_vlan_conf.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c index b98c4d92cf..012a28034e 100644 --- a/src/conf/netdev_vlan_conf.c +++ b/src/conf/netdev_vlan_conf.c @@ -34,8 +34,8 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def) { int ret = -1; VIR_XPATH_NODE_AUTORESTORE(ctxt) - char *trunk = NULL; - xmlNodePtr *tagNodes = NULL; + g_autofree char *trunk = NULL; + g_autofree xmlNodePtr *tagNodes = NULL; int nTags; size_t i; @@ -120,8 +120,6 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def) ret = 0; cleanup: - VIR_FREE(tagNodes); - VIR_FREE(trunk); return ret; } -- 2.51.0
On 10/20/25 15:21, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Automatically free the variables to prevent leaks when returning from middle of the function.
Fixes: 1de6fd5edb5 Closes: https://gitlab.com/libvirt/libvirt/-/issues/824 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/netdev_vlan_conf.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c index b98c4d92cf..012a28034e 100644 --- a/src/conf/netdev_vlan_conf.c +++ b/src/conf/netdev_vlan_conf.c @@ -34,8 +34,8 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def) { int ret = -1; VIR_XPATH_NODE_AUTORESTORE(ctxt) - char *trunk = NULL; - xmlNodePtr *tagNodes = NULL; + g_autofree char *trunk = NULL; + g_autofree xmlNodePtr *tagNodes = NULL; int nTags; size_t i;
@@ -120,8 +120,6 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def)
ret = 0; cleanup: - VIR_FREE(tagNodes); - VIR_FREE(trunk); return ret; }
Ah, took me a while to realize what the actual problem is. There's one 'return -1' call which should have been 'goto cleanup' instead. Michal
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/netdev_vlan_conf.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c index 012a28034e..db142024cd 100644 --- a/src/conf/netdev_vlan_conf.c +++ b/src/conf/netdev_vlan_conf.c @@ -30,9 +30,10 @@ VIR_ENUM_IMPL(virNativeVlanMode, ); int -virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def) +virNetDevVlanParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virNetDevVlan *def) { - int ret = -1; VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autofree char *trunk = NULL; g_autofree xmlNodePtr *tagNodes = NULL; @@ -43,12 +44,12 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def) nTags = virXPathNodeSet("./tag", ctxt, &tagNodes); if (nTags < 0) - goto cleanup; + return -1; if (nTags == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing tag id - each <vlan> must have at least one <tag id='n'/> subelement")); - goto cleanup; + return -1; } def->tag = g_new0(unsigned int, nTags); @@ -60,12 +61,12 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def) if (virXMLPropUInt(tagNodes[i], "id", 10, VIR_XML_PROP_REQUIRED, &def->tag[i]) < 0) - goto cleanup; + return -1; if (def->tag[i] > 4095) { virReportError(VIR_ERR_XML_ERROR, _("vlan tag id %1$u too large (maximum 4095)"), def->tag[i]); - goto cleanup; + return -1; } if ((rc = virXMLPropEnum(tagNodes[i], "nativeMode", @@ -77,7 +78,7 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def) if (def->nativeMode != 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("duplicate native vlan setting")); - goto cleanup; + return -1; } def->nativeMode = nativeMode; @@ -101,26 +102,24 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlan *def) virReportError(VIR_ERR_XML_ERROR, _("invalid \"trunk='%1$s'\" in <vlan> - trunk='yes' is required for more than one vlan tag"), trunk); - goto cleanup; + return -1; } if (def->nativeMode != 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid configuration in <vlan> - \"trunk='no'\" is not allowed with a native vlan id")); - goto cleanup; + return -1; } /* allow (but discard) "trunk='no' if there is a single tag */ if (STRCASENEQ(trunk, "no")) { virReportError(VIR_ERR_XML_ERROR, _("invalid \"trunk='%1$s'\" in <vlan> - must be yes or no"), trunk); - goto cleanup; + return -1; } } } - ret = 0; - cleanup: - return ret; + return 0; } int -- 2.51.0
On 10/20/25 15:21, Peter Krempa via Devel wrote:
Peter Krempa (3): virNetDevVlanParse: Don't clear data on failure virNetDevVlanParse: Use g_autofree for temporary variables virNetDevVlanParse: Refactor cleanup
src/conf/netdev_vlan_conf.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa