[PATCH 00/12] Remove virXMLPropStringLimit and virXPathStringLimit
The functions have API which is impossible to be use correctly by callers. Refactor callers and remove the functions. Note that this patchset preserves semantics of the callers (except for removing duplicate or ignored errors). Some of the callers look fishy, but that is not addressed here. Peter Krempa (12): util: seclabel: Define autoptr cleanup func for virSecurityLabelDef and virSecurityDeviceLabelDef virSecurityLabelDef: Declare 'type' as 'virDomainSeclabelType' virSecurityLabelDefParseXML: Directly assign strings into appropriate variables virSecurityLabelDefParseXML: Don't reuse temporary string 'p' virSecurityLabelDefParseXML: Use automatic freeing for 'seclabel' virSecurityLabelDefParseXML: Remove pointless 'error' label virNodeDeviceCapVPDParseCustomFields: Don't use 'virXPathStringLimit' virSecurityLabelDefParseXML: Don't use 'virXPathStringLimit' virSecurityDeviceLabelDefParseXML: Use automatic memory clearing for temp strings virSecurityDeviceLabelDefParseXML: Don't use 'virXPathStringLimit' virSecurityLabelDefParseXML: Don't use virXMLPropStringLimit util: xml: Remove virXMLPropStringLimit and virXPathStringLimit src/conf/domain_conf.c | 104 +++++++++++++++----------------- src/conf/node_device_conf.c | 6 +- src/libvirt_private.syms | 2 - src/security/security_selinux.c | 3 +- src/util/virseclabel.h | 6 +- src/util/virxml.c | 62 ------------------- src/util/virxml.h | 8 --- 7 files changed, 59 insertions(+), 132 deletions(-) -- 2.31.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virseclabel.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index 18447051fc..77bf6da2c3 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -63,4 +63,7 @@ virSecurityDeviceLabelDefCopy(const virSecurityDeviceLabelDef *src) ATTRIBUTE_NONNULL(1); void virSecurityLabelDefFree(virSecurityLabelDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSecurityLabelDef, virSecurityLabelDefFree); + void virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSecurityDeviceLabelDef, virSecurityDeviceLabelDefFree); -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virseclabel.h | 3 +++ 1 file changed, 3 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use the appropriate enum type instead of an int and fix the XML parser and one missing fully populated switch. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++++--------- src/security/security_selinux.c | 3 ++- src/util/virseclabel.h | 2 +- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 552d43b845..ba38d510dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7813,15 +7813,11 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, /* set default value */ seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC; - p = virXMLPropString(ctxt->node, "type"); - if (p) { - seclabel->type = virDomainSeclabelTypeFromString(p); - if (seclabel->type <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid security type '%s'"), p); - goto error; - } - } + if (virXMLPropEnum(ctxt->node, "type", + virDomainSeclabelTypeFromString, + VIR_XML_PROP_NONZERO, + &seclabel->type) < 0) + goto error; if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || seclabel->type == VIR_DOMAIN_SECLABEL_NONE) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 840a05844e..5682d2bb9d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -899,7 +899,8 @@ virSecuritySELinuxGenLabel(virSecurityManager *mgr, mcs = g_strdup(sens); break; - + case VIR_DOMAIN_SECLABEL_DEFAULT: + case VIR_DOMAIN_SECLABEL_LAST: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected security label type '%s'"), diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index 77bf6da2c3..eca4d09d24 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -37,7 +37,7 @@ struct _virSecurityLabelDef { char *label; /* security label string */ char *imagelabel; /* security image label string */ char *baselabel; /* base name of label string */ - int type; /* virDomainSeclabelType */ + virDomainSeclabelType type; /* virDomainSeclabelType */ bool relabel; /* true (default) for allowing relabels */ bool implicit; /* true if seclabel is auto-added */ }; -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
Use the appropriate enum type instead of an int and fix the XML parser and one missing fully populated switch.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++++--------- src/security/security_selinux.c | 3 ++- src/util/virseclabel.h | 2 +- 3 files changed, 8 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
'seclabel->label', 'seclabel->imagelabel' and 'seclabel->baselabel' are populated by stealing the pointer from the 'p' temporary string. Remove the extra step. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ba38d510dd..24de57005c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7874,36 +7874,32 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - p = virXPathStringLimit("string(./label[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) { + seclabel->label = virXPathStringLimit("string(./label[1])", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (!seclabel->label) { virReportError(VIR_ERR_XML_ERROR, "%s", _("security label is missing")); goto error; } - - seclabel->label = g_steal_pointer(&p); } /* Only parse imagelabel, if requested live XML with relabeling */ if (seclabel->relabel && (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - p = virXPathStringLimit("string(./imagelabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) { + seclabel->imagelabel = virXPathStringLimit("string(./imagelabel[1])", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (!seclabel->imagelabel) { virReportError(VIR_ERR_XML_ERROR, "%s", _("security imagelabel is missing")); goto error; } - seclabel->imagelabel = g_steal_pointer(&p); } /* Only parse baselabel for dynamic label type */ if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - p = virXPathStringLimit("string(./baselabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - seclabel->baselabel = g_steal_pointer(&p); + seclabel->baselabel = virXPathStringLimit("string(./baselabel[1])", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); } return seclabel; -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
'seclabel->label', 'seclabel->imagelabel' and 'seclabel->baselabel' are populated by stealing the pointer from the 'p' temporary string. Remove the extra step.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use separate variables for 'model' and 'relabel' properties. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++------------ src/util/virseclabel.h | 1 + 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 24de57005c..df0d033d0b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7800,15 +7800,15 @@ static virSecurityLabelDef * virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, unsigned int flags) { - char *p; + g_autofree char *model = NULL; + g_autofree char *relabel = NULL; virSecurityLabelDef *seclabel = NULL; - p = virXMLPropStringLimit(ctxt->node, "model", - VIR_SECURITY_MODEL_BUFLEN - 1); + model = virXMLPropStringLimit(ctxt->node, "model", + VIR_SECURITY_MODEL_BUFLEN - 1); - if (!(seclabel = virSecurityLabelDefNew(p))) + if (!(seclabel = virSecurityLabelDefNew(model))) goto error; - VIR_FREE(p); /* set default value */ seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC; @@ -7823,16 +7823,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, seclabel->type == VIR_DOMAIN_SECLABEL_NONE) seclabel->relabel = false; - VIR_FREE(p); - p = virXMLPropString(ctxt->node, "relabel"); - if (p) { - if (virStringParseYesNo(p, &seclabel->relabel) < 0) { + if ((relabel = virXMLPropString(ctxt->node, "relabel"))) { + if (virStringParseYesNo(relabel, &seclabel->relabel) < 0) { virReportError(VIR_ERR_XML_ERROR, - _("invalid security relabel value %s"), p); + _("invalid security relabel value '%s'"), relabel); goto error; } } - VIR_FREE(p); if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC && !seclabel->relabel) { @@ -7905,7 +7902,6 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, return seclabel; error: - VIR_FREE(p); virSecurityLabelDefFree(seclabel); return NULL; } diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index eca4d09d24..7e62f8a2e2 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -43,6 +43,7 @@ struct _virSecurityLabelDef { }; + /* Security configuration for device */ typedef struct _virSecurityDeviceLabelDef virSecurityDeviceLabelDef; struct _virSecurityDeviceLabelDef { -- 2.31.1
On Mon, 2021-11-22 at 18:12 +0100, Peter Krempa wrote:
(...) diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index eca4d09d24..7e62f8a2e2 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -43,6 +43,7 @@ struct _virSecurityLabelDef { };
+ /* Security configuration for device */ typedef struct _virSecurityDeviceLabelDef virSecurityDeviceLabelDef; struct _virSecurityDeviceLabelDef {
I don't think this change was intentional
On a Monday in 2021, Peter Krempa wrote:
Use separate variables for 'model' and 'relabel' properties.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++------------ src/util/virseclabel.h | 1 + 2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index eca4d09d24..7e62f8a2e2 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -43,6 +43,7 @@ struct _virSecurityLabelDef { };
+
Without this suspicious empty line: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
/* Security configuration for device */ typedef struct _virSecurityDeviceLabelDef virSecurityDeviceLabelDef; struct _virSecurityDeviceLabelDef { -- 2.31.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index df0d033d0b..99bee98df8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7802,7 +7802,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, { g_autofree char *model = NULL; g_autofree char *relabel = NULL; - virSecurityLabelDef *seclabel = NULL; + g_autoptr(virSecurityLabelDef) seclabel = NULL; model = virXMLPropStringLimit(ctxt->node, "model", VIR_SECURITY_MODEL_BUFLEN - 1); @@ -7862,7 +7862,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, /* combination of relabel='yes' and type='static' * is checked a few lines above. */ } - return seclabel; + return g_steal_pointer(&seclabel); } /* Only parse label, if using static labels, or @@ -7899,10 +7899,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, VIR_SECURITY_LABEL_BUFLEN-1, ctxt); } - return seclabel; + return g_steal_pointer(&seclabel); error: - virSecurityLabelDefFree(seclabel); return NULL; } -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 99bee98df8..ee44bbbd4b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7808,7 +7808,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, VIR_SECURITY_MODEL_BUFLEN - 1); if (!(seclabel = virSecurityLabelDefNew(model))) - goto error; + return NULL; /* set default value */ seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC; @@ -7817,7 +7817,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, virDomainSeclabelTypeFromString, VIR_XML_PROP_NONZERO, &seclabel->type) < 0) - goto error; + return NULL; if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || seclabel->type == VIR_DOMAIN_SECLABEL_NONE) @@ -7827,7 +7827,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (virStringParseYesNo(relabel, &seclabel->relabel) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value '%s'"), relabel); - goto error; + return NULL; } } @@ -7835,13 +7835,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, !seclabel->relabel) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("dynamic label type must use resource relabeling")); - goto error; + return NULL; } if (seclabel->type == VIR_DOMAIN_SECLABEL_NONE && seclabel->relabel) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("resource relabeling is not compatible with 'none' label type")); - goto error; + return NULL; } /* For the model 'none' none of the following labels is going to be @@ -7857,7 +7857,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported type='%s' to model 'none'"), virDomainSeclabelTypeToString(seclabel->type)); - goto error; + return NULL; } /* combination of relabel='yes' and type='static' * is checked a few lines above. */ @@ -7876,7 +7876,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (!seclabel->label) { virReportError(VIR_ERR_XML_ERROR, "%s", _("security label is missing")); - goto error; + return NULL; } } @@ -7889,7 +7889,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (!seclabel->imagelabel) { virReportError(VIR_ERR_XML_ERROR, "%s", _("security imagelabel is missing")); - goto error; + return NULL; } } @@ -7900,9 +7900,6 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, } return g_steal_pointer(&seclabel); - - error: - return NULL; } static int -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded. This means that callers are overwriting the error message. Move the length checks into the caller. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/node_device_conf.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index ca534dfbed..0bac0fde8d 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -955,7 +955,8 @@ virNodeDeviceCapVPDParseCustomFields(xmlXPathContextPtr ctxt, virPCIVPDResource g_autofree char *keyword = NULL; ctxt->node = nodes[i]; - if (!(index = virXPathStringLimit("string(./@index[1])", 2, ctxt))) { + if (!(index = virXPathString("string(./@index[1])", ctxt)) || + strlen(index) > 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("<vendor_field> evaluation has failed")); continue; @@ -983,7 +984,8 @@ virNodeDeviceCapVPDParseCustomFields(xmlXPathContextPtr ctxt, virPCIVPDResource VIR_XPATH_NODE_AUTORESTORE(ctxt); ctxt->node = nodes[i]; - if (!(index = virXPathStringLimit("string(./@index[1])", 2, ctxt))) { + if (!(index = virXPathString("string(./@index[1])", ctxt)) || + strlen(index) > 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("<system_field> evaluation has failed")); continue; -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded.
This means that callers are overwriting the error message.
Move the length checks into the caller.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/node_device_conf.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded. This means that callers are either overwriting the error message or ignoring it altogether. Move the length checks into the caller. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ee44bbbd4b..bd9da0744d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7871,9 +7871,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - seclabel->label = virXPathStringLimit("string(./label[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (!seclabel->label) { + seclabel->label = virXPathString("string(./label[1])", ctxt); + if (!seclabel->label || + strlen(seclabel->label) >= VIR_SECURITY_LABEL_BUFLEN - 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("security label is missing")); return NULL; @@ -7884,9 +7884,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (seclabel->relabel && (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - seclabel->imagelabel = virXPathStringLimit("string(./imagelabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (!seclabel->imagelabel) { + seclabel->imagelabel = virXPathString("string(./imagelabel[1])", ctxt); + + if (!seclabel->imagelabel || + strlen(seclabel->imagelabel) >= VIR_SECURITY_LABEL_BUFLEN - 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("security imagelabel is missing")); return NULL; @@ -7895,8 +7896,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, /* Only parse baselabel for dynamic label type */ if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - seclabel->baselabel = virXPathStringLimit("string(./baselabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + seclabel->baselabel = virXPathString("string(./baselabel[1])", ctxt); + + if (seclabel->baselabel && + strlen(seclabel->baselabel) >= VIR_SECURITY_LABEL_BUFLEN - 1) { + g_free(seclabel->baselabel); + seclabel->baselabel = NULL; + } } return g_steal_pointer(&seclabel); -- 2.31.1
On Mon, 2021-11-22 at 18:12 +0100, Peter Krempa wrote:
virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded.
This means that callers are either overwriting the error message or ignoring it altogether.
Move the length checks into the caller.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ee44bbbd4b..bd9da0744d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7871,9 +7871,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - seclabel->label = virXPathStringLimit("string(./label[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (!seclabel->label) { + seclabel->label = virXPathString("string(./label[1])", ctxt); + if (!seclabel->label || + strlen(seclabel->label) >= VIR_SECURITY_LABEL_BUFLEN - 1) {
What is your opinion on using strnlen instead? Not necessarily for security reasons, but since we do not care for the exact string length if it is above a certain size, we can return early.
virReportError(VIR_ERR_XML_ERROR, "%s", _("security label is missing")); return NULL; @@ -7884,9 +7884,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (seclabel->relabel && (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - seclabel->imagelabel = virXPathStringLimit("string(./imagelabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (!seclabel->imagelabel) { + seclabel->imagelabel = virXPathString("string(./imagelabel[1])", ctxt); + + if (!seclabel->imagelabel || + strlen(seclabel->imagelabel) >= VIR_SECURITY_LABEL_BUFLEN - 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("security imagelabel is missing")); return NULL; @@ -7895,8 +7896,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
/* Only parse baselabel for dynamic label type */ if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - seclabel->baselabel = virXPathStringLimit("string(./baselabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + seclabel->baselabel = virXPathString("string(./baselabel[1])", ctxt); + + if (seclabel->baselabel && + strlen(seclabel->baselabel) >= VIR_SECURITY_LABEL_BUFLEN - 1) { + g_free(seclabel->baselabel); + seclabel->baselabel = NULL; + }
g_clear_pointer?
}
return g_steal_pointer(&seclabel);
On Mon, Nov 22, 2021 at 21:01:58 +0100, Tim Wiederhake wrote:
On Mon, 2021-11-22 at 18:12 +0100, Peter Krempa wrote:
virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded.
This means that callers are either overwriting the error message or ignoring it altogether.
Move the length checks into the caller.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ee44bbbd4b..bd9da0744d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7871,9 +7871,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - seclabel->label = virXPathStringLimit("string(./label[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (!seclabel->label) { + seclabel->label = virXPathString("string(./label[1])", ctxt); + if (!seclabel->label || + strlen(seclabel->label) >= VIR_SECURITY_LABEL_BUFLEN - 1) {
What is your opinion on using strnlen instead? Not necessarily for security reasons, but since we do not care for the exact string length if it is above a certain size, we can return early.
Yeah, selling it as security is impossible because we already have at least two copies of the XML in memory. As a optimization sure, but let's see whether it doesn't make the code too bulky as the lenght constant appears twice in the expression.
virReportError(VIR_ERR_XML_ERROR, "%s", _("security label is missing")); return NULL; @@ -7884,9 +7884,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (seclabel->relabel && (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - seclabel->imagelabel = virXPathStringLimit("string(./imagelabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (!seclabel->imagelabel) { + seclabel->imagelabel = virXPathString("string(./imagelabel[1])", ctxt); + + if (!seclabel->imagelabel || + strlen(seclabel->imagelabel) >= VIR_SECURITY_LABEL_BUFLEN - 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("security imagelabel is missing")); return NULL; @@ -7895,8 +7896,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
/* Only parse baselabel for dynamic label type */ if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - seclabel->baselabel = virXPathStringLimit("string(./baselabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + seclabel->baselabel = virXPathString("string(./baselabel[1])", ctxt); + + if (seclabel->baselabel && + strlen(seclabel->baselabel) >= VIR_SECURITY_LABEL_BUFLEN - 1) { + g_free(seclabel->baselabel); + seclabel->baselabel = NULL; + }
g_clear_pointer?
Yeah, I've actually used it in later patch.
On a Monday in 2021, Peter Krempa wrote:
virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded.
This means that callers are either overwriting the error message or ignoring it altogether.
Move the length checks into the caller.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Apart from code simplification the refactor of 'model' fixes an unlikely memory leak of the string if a duplicate model is found. While the coversion of 'label' variable may seem unnecessary it will come in handy in the next patch. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd9da0744d..e829511ac5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8016,7 +8016,10 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, size_t nseclabels = 0; int n; size_t i, j; - char *model, *relabel, *label, *labelskip; + g_autofree char *model = NULL; + g_autofree char *relabel = NULL; + g_autofree char *label = NULL; + g_autofree char *labelskip = NULL; g_autofree xmlNodePtr *list = NULL; if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0) @@ -8041,7 +8044,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, goto error; } } - seclabels[i]->model = model; + seclabels[i]->model = g_steal_pointer(&model); } relabel = virXMLPropString(list[i], "relabel"); @@ -8050,10 +8053,8 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), relabel); - VIR_FREE(relabel); goto error; } - VIR_FREE(relabel); } else { seclabels[i]->relabel = true; } @@ -8063,14 +8064,13 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, seclabels[i]->labelskip = false; if (labelskip && !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) ignore_value(virStringParseYesNo(labelskip, &seclabels[i]->labelskip)); - VIR_FREE(labelskip); ctxt->node = list[i]; label = virXPathStringLimit("string(./label)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - seclabels[i]->label = label; + seclabels[i]->label = g_steal_pointer(&label); - if (label && !seclabels[i]->relabel) { + if (seclabels[i]->label && !seclabels[i]->relabel) { virReportError(VIR_ERR_XML_ERROR, _("Cannot specify a label if relabelling is " "turned off. model=%s"), -- 2.31.1
On Mon, Nov 22, 2021 at 18:12:29 +0100, Peter Krempa wrote:
Apart from code simplification the refactor of 'model' fixes an unlikely memory leak of the string if a duplicate model is found.
While the coversion of 'label' variable may seem unnecessary it will come in handy in the next patch.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd9da0744d..e829511ac5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8016,7 +8016,10 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, size_t nseclabels = 0; int n; size_t i, j; - char *model, *relabel, *label, *labelskip; + g_autofree char *model = NULL; + g_autofree char *relabel = NULL; + g_autofree char *label = NULL; + g_autofree char *labelskip = NULL; g_autofree xmlNodePtr *list = NULL;
if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0) @@ -8041,7 +8044,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, goto error; } } - seclabels[i]->model = model; + seclabels[i]->model = g_steal_pointer(&model); }
relabel = virXMLPropString(list[i], "relabel");
Forgot to squash the following diff into this commit: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 17ba810467..16dea34890 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8017,10 +8017,6 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, size_t nseclabels = 0; int n; size_t i, j; - g_autofree char *model = NULL; - g_autofree char *relabel = NULL; - g_autofree char *label = NULL; - g_autofree char *labelskip = NULL; g_autofree xmlNodePtr *list = NULL; if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0) @@ -8034,6 +8030,11 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, seclabels[i] = g_new0(virSecurityDeviceLabelDef, 1); for (i = 0; i < n; i++) { + g_autofree char *model = NULL; + g_autofree char *relabel = NULL; + g_autofree char *label = NULL; + g_autofree char *labelskip = NULL; + /* get model associated to this override */ model = virXMLPropString(list[i], "model"); if (model) {
On a Monday in 2021, Peter Krempa wrote:
On Mon, Nov 22, 2021 at 18:12:29 +0100, Peter Krempa wrote:
Apart from code simplification the refactor of 'model' fixes an unlikely memory leak of the string if a duplicate model is found.
While the coversion of 'label' variable may seem unnecessary it will come in handy in the next patch.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd9da0744d..e829511ac5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8016,7 +8016,10 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, size_t nseclabels = 0; int n; size_t i, j; - char *model, *relabel, *label, *labelskip; + g_autofree char *model = NULL; + g_autofree char *relabel = NULL; + g_autofree char *label = NULL; + g_autofree char *labelskip = NULL; g_autofree xmlNodePtr *list = NULL;
if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0) @@ -8041,7 +8044,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, goto error; } } - seclabels[i]->model = model; + seclabels[i]->model = g_steal_pointer(&model); }
relabel = virXMLPropString(list[i], "relabel");
Forgot to squash the following diff into this commit:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 17ba810467..16dea34890 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8017,10 +8017,6 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, size_t nseclabels = 0; int n; size_t i, j; - g_autofree char *model = NULL; - g_autofree char *relabel = NULL; - g_autofree char *label = NULL; - g_autofree char *labelskip = NULL; g_autofree xmlNodePtr *list = NULL;
if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0) @@ -8034,6 +8030,11 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, seclabels[i] = g_new0(virSecurityDeviceLabelDef, 1);
for (i = 0; i < n; i++) { + g_autofree char *model = NULL; + g_autofree char *relabel = NULL; + g_autofree char *label = NULL; + g_autofree char *labelskip = NULL; + /* get model associated to this override */
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Mon, 2021-11-22 at 18:12 +0100, Peter Krempa wrote:
Apart from code simplification the refactor of 'model' fixes an unlikely memory leak of the string if a duplicate model is found.
While the coversion of 'label' variable may seem unnecessary it will come in handy in the next patch.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd9da0744d..e829511ac5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8016,7 +8016,10 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, size_t nseclabels = 0; int n; size_t i, j; - char *model, *relabel, *label, *labelskip; + g_autofree char *model = NULL; + g_autofree char *relabel = NULL; + g_autofree char *label = NULL; + g_autofree char *labelskip = NULL; g_autofree xmlNodePtr *list = NULL;
if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0) @@ -8041,7 +8044,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, goto error; } } - seclabels[i]->model = model; + seclabels[i]->model = g_steal_pointer(&model); }
relabel = virXMLPropString(list[i], "relabel"); @@ -8050,10 +8053,8 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), relabel); - VIR_FREE(relabel); goto error; } - VIR_FREE(relabel); } else { seclabels[i]->relabel = true; } @@ -8063,14 +8064,13 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, seclabels[i]->labelskip = false; if (labelskip && !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) ignore_value(virStringParseYesNo(labelskip, &seclabels[i]->labelskip)); - VIR_FREE(labelskip);
ctxt->node = list[i]; label = virXPathStringLimit("string(./label)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - seclabels[i]->label = label; + seclabels[i]->label = g_steal_pointer(&label);
- if (label && !seclabels[i]->relabel) { + if (seclabels[i]->label && !seclabels[i]->relabel) { virReportError(VIR_ERR_XML_ERROR, _("Cannot specify a label if relabelling is " "turned off. model=%s"),
Theese look like they could be simplified to three calls to `virXMLPropTristateBool`
On Mon, Nov 22, 2021 at 21:02:01 +0100, Tim Wiederhake wrote:
On Mon, 2021-11-22 at 18:12 +0100, Peter Krempa wrote:
Apart from code simplification the refactor of 'model' fixes an unlikely memory leak of the string if a duplicate model is found.
While the coversion of 'label' variable may seem unnecessary it will come in handy in the next patch.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd9da0744d..e829511ac5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8016,7 +8016,10 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, size_t nseclabels = 0; int n; size_t i, j; - char *model, *relabel, *label, *labelskip; + g_autofree char *model = NULL; + g_autofree char *relabel = NULL; + g_autofree char *label = NULL; + g_autofree char *labelskip = NULL; g_autofree xmlNodePtr *list = NULL;
if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0) @@ -8041,7 +8044,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, goto error; } } - seclabels[i]->model = model; + seclabels[i]->model = g_steal_pointer(&model); }
relabel = virXMLPropString(list[i], "relabel"); @@ -8050,10 +8053,8 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), relabel); - VIR_FREE(relabel); goto error; } - VIR_FREE(relabel); } else { seclabels[i]->relabel = true; } @@ -8063,14 +8064,13 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, seclabels[i]->labelskip = false; if (labelskip && !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) ignore_value(virStringParseYesNo(labelskip, &seclabels[i]->labelskip)); - VIR_FREE(labelskip);
ctxt->node = list[i]; label = virXPathStringLimit("string(./label)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - seclabels[i]->label = label; + seclabels[i]->label = g_steal_pointer(&label);
- if (label && !seclabels[i]->relabel) { + if (seclabels[i]->label && !seclabels[i]->relabel) { virReportError(VIR_ERR_XML_ERROR, _("Cannot specify a label if relabelling is " "turned off. model=%s"),
Theese look like they could be simplified to three calls to `virXMLPropTristateBool`
Indeed, 'relabel' and 'labelksip' can be parsed into a temp tristate and then set the boolean in the label from it. I don't want to go further to refactor all security labelling code to a real tristate.
virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded. This means that the callers is completely ignoring the error. Move the length check into the caller. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e829511ac5..d6eefed398 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8066,9 +8066,10 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, ignore_value(virStringParseYesNo(labelskip, &seclabels[i]->labelskip)); ctxt->node = list[i]; - label = virXPathStringLimit("string(./label)", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - seclabels[i]->label = g_steal_pointer(&label); + label = virXPathString("string(./label)", ctxt); + + if (label && strlen(label) < VIR_SECURITY_LABEL_BUFLEN) + seclabels[i]->label = g_steal_pointer(&label); if (seclabels[i]->label && !seclabels[i]->relabel) { virReportError(VIR_ERR_XML_ERROR, -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded.
This means that the callers is completely ignoring the error.
Move the length check into the caller.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function produces an error which is ignored in this code path. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d6eefed398..17ba810467 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7804,8 +7804,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, g_autofree char *relabel = NULL; g_autoptr(virSecurityLabelDef) seclabel = NULL; - model = virXMLPropStringLimit(ctxt->node, "model", - VIR_SECURITY_MODEL_BUFLEN - 1); + if ((model = virXMLPropString(ctxt->node, "model")) && + strlen(model) >= VIR_SECURITY_MODEL_BUFLEN - 1) + g_clear_pointer(&model, g_free); if (!(seclabel = virSecurityLabelDefNew(model))) return NULL; -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
The function produces an error which is ignored in this code path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The functions have very difficult semantics where callers are not able to tell whether the property is missing or failed the length check. Only the latter produces errors. Since usage of the functions was phased out, remove them completely to avoid further broken code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 -- src/util/virxml.c | 62 ---------------------------------------- src/util/virxml.h | 8 ------ 3 files changed, 72 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7bc50a4d1..13b7266c97 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3625,7 +3625,6 @@ virXMLPropEnum; virXMLPropEnumDefault; virXMLPropInt; virXMLPropString; -virXMLPropStringLimit; virXMLPropTristateBool; virXMLPropTristateSwitch; virXMLPropUInt; @@ -3646,7 +3645,6 @@ virXPathNode; virXPathNodeSet; virXPathNumber; virXPathString; -virXPathStringLimit; virXPathUInt; virXPathULong; virXPathULongHex; diff --git a/src/util/virxml.c b/src/util/virxml.c index b736d59d9c..4b09374107 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -89,45 +89,6 @@ virXPathString(const char *xpath, } -static char * -virXMLStringLimitInternal(char *value, - size_t maxlen, - const char *name) -{ - if (value != NULL && strlen(value) >= maxlen) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%s' value longer than '%zu' bytes"), - name, maxlen); - VIR_FREE(value); - return NULL; - } - - return value; -} - - -/** - * virXPathStringLimit: - * @xpath: the XPath string to evaluate - * @maxlen: maximum length permitted string - * @ctxt: an XPath context - * - * Wrapper for virXPathString, which validates the length of the returned - * string. - * - * Returns a new string which must be deallocated by the caller or NULL if - * the evaluation failed. - */ -char * -virXPathStringLimit(const char *xpath, - size_t maxlen, - xmlXPathContextPtr ctxt) -{ - char *tmp = virXPathString(xpath, ctxt); - - return virXMLStringLimitInternal(tmp, maxlen, xpath); -} - /** * virXPathNumber: * @xpath: the XPath string to evaluate @@ -492,29 +453,6 @@ virXMLPropString(xmlNodePtr node, } -/** - * virXMLPropStringLimit: - * @node: XML dom node pointer - * @name: Name of the property (attribute) to get - * @maxlen: maximum permitted length of the string - * - * Wrapper for virXMLPropString, which validates the length of the returned - * string. - * - * Returns a new string which must be deallocated by the caller or NULL if - * the evaluation failed. - */ -char * -virXMLPropStringLimit(xmlNodePtr node, - const char *name, - size_t maxlen) -{ - char *tmp = (char *)xmlGetProp(node, BAD_CAST name); - - return virXMLStringLimitInternal(tmp, maxlen, name); -} - - /** * virXMLNodeContentString: * @node: XML dom node pointer diff --git a/src/util/virxml.h b/src/util/virxml.h index 06fb7aebd8..6f9f217f73 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -47,10 +47,6 @@ virXPathBoolean(const char *xpath, char * virXPathString(const char *xpath, xmlXPathContextPtr ctxt); -char * -virXPathStringLimit(const char *xpath, - size_t maxlen, - xmlXPathContextPtr ctxt); int virXPathNumber(const char *xpath, xmlXPathContextPtr ctxt, @@ -98,10 +94,6 @@ char * virXMLPropString(xmlNodePtr node, const char *name); char * -virXMLPropStringLimit(xmlNodePtr node, - const char *name, - size_t maxlen); -char * virXMLNodeContentString(xmlNodePtr node); int -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
The functions have very difficult semantics where callers are not able to tell whether the property is missing or failed the length check. Only the latter produces errors.
Since usage of the functions was phased out, remove them completely to avoid further broken code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 -- src/util/virxml.c | 62 ---------------------------------------- src/util/virxml.h | 8 ------ 3 files changed, 72 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko -
Peter Krempa -
Tim Wiederhake