11:12 a.m.
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
11:12 a.m.
New subject: [PATCH 01/12] util: seclabel: Define autoptr cleanup func for virSecurityLabelDef and virSecurityDeviceLabelDef
Signed-off-by: Peter Krempa <pkrempa(a)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
6:56 a.m.
New subject: [PATCH 01/12] util: seclabel: Define autoptr cleanup func for virSecurityLabelDef and virSecurityDeviceLabelDef
On a Monday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virseclabel.h | 3 +++
1 file changed, 3 insertions(+)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:12 a.m.
New subject: [PATCH 02/12] virSecurityLabelDef: Declare 'type' as 'virDomainSeclabelType'
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(a)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
6:57 a.m.
New subject: [PATCH 02/12] virSecurityLabelDef: Declare 'type' as 'virDomainSeclabelType'
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(a)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(a)redhat.com>
Jano
11:12 a.m.
New subject: [PATCH 03/12] virSecurityLabelDefParseXML: Directly assign strings into appropriate variables
'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(a)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
8:26 a.m.
New subject: [PATCH 03/12] virSecurityLabelDefParseXML: Directly assign strings into appropriate variables
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(a)redhat.com>
---
src/conf/domain_conf.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:12 a.m.
New subject: [PATCH 04/12] virSecurityLabelDefParseXML: Don't reuse temporary string 'p'
Use separate variables for 'model' and 'relabel' properties.
Signed-off-by: Peter Krempa <pkrempa(a)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
1:24 p.m.
New subject: [PATCH 04/12] virSecurityLabelDefParseXML: Don't reuse temporary string 'p'
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
8:28 a.m.
New subject: [PATCH 04/12] virSecurityLabelDefParseXML: Don't reuse temporary string 'p'
On a Monday in 2021, Peter Krempa wrote:
Use separate variables for 'model' and 'relabel'
properties.
Signed-off-by: Peter Krempa <pkrempa(a)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(a)redhat.com>
Jano
/* Security configuration for device */
typedef struct _virSecurityDeviceLabelDef virSecurityDeviceLabelDef;
struct _virSecurityDeviceLabelDef {
--
2.31.1
11:12 a.m.
New subject: [PATCH 05/12] virSecurityLabelDefParseXML: Use automatic freeing for 'seclabel'
Signed-off-by: Peter Krempa <pkrempa(a)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
8:29 a.m.
New subject: [PATCH 05/12] virSecurityLabelDefParseXML: Use automatic freeing for 'seclabel'
On a Monday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:12 a.m.
New subject: [PATCH 06/12] virSecurityLabelDefParseXML: Remove pointless 'error' label
Signed-off-by: Peter Krempa <pkrempa(a)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
8:29 a.m.
New subject: [PATCH 06/12] virSecurityLabelDefParseXML: Remove pointless 'error' label
On a Monday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:12 a.m.
New subject: [PATCH 07/12] virNodeDeviceCapVPDParseCustomFields: Don't use 'virXPathStringLimit'
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(a)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
8:30 a.m.
New subject: [PATCH 07/12] virNodeDeviceCapVPDParseCustomFields: Don't use 'virXPathStringLimit'
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(a)redhat.com>
---
src/conf/node_device_conf.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:12 a.m.
New subject: [PATCH 08/12] virSecurityLabelDefParseXML: Don't use 'virXPathStringLimit'
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(a)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
2:01 p.m.
New subject: [PATCH 08/12] virSecurityLabelDefParseXML: Don't use 'virXPathStringLimit'
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(a)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);
3:41 a.m.
New subject: [PATCH 08/12] virSecurityLabelDefParseXML: Don't use 'virXPathStringLimit'
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(a)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.
8:32 a.m.
New subject: [PATCH 08/12] virSecurityLabelDefParseXML: Don't use 'virXPathStringLimit'
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(a)redhat.com>
---
src/conf/domain_conf.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:12 a.m.
New subject: [PATCH 09/12] virSecurityDeviceLabelDefParseXML: Use automatic memory clearing for temp strings
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(a)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
12:33 p.m.
New subject: [PATCH 09/12] virSecurityDeviceLabelDefParseXML: Use automatic memory clearing for temp strings
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(a)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) {
8:34 a.m.
New subject: [PATCH 09/12] virSecurityDeviceLabelDefParseXML: Use automatic memory clearing for temp strings
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(a)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(a)redhat.com>
Jano
2:02 p.m.
New subject: [PATCH 09/12] virSecurityDeviceLabelDefParseXML: Use automatic memory clearing for temp strings
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(a)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`
2:20 a.m.
New subject: [PATCH 09/12] virSecurityDeviceLabelDefParseXML: Use automatic memory clearing for temp strings
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(a)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.
11:12 a.m.
New subject: [PATCH 10/12] virSecurityDeviceLabelDefParseXML: Don't use 'virXPathStringLimit'
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(a)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
8:36 a.m.
New subject: [PATCH 10/12] virSecurityDeviceLabelDefParseXML: Don't use 'virXPathStringLimit'
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(a)redhat.com>
---
src/conf/domain_conf.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:12 a.m.
New subject: [PATCH 11/12] virSecurityLabelDefParseXML: Don't use virXMLPropStringLimit
The function produces an error which is ignored in this code path.
Signed-off-by: Peter Krempa <pkrempa(a)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
8:37 a.m.
New subject: [PATCH 11/12] virSecurityLabelDefParseXML: Don't use virXMLPropStringLimit
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(a)redhat.com>
---
src/conf/domain_conf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:12 a.m.
New subject: [PATCH 12/12] util: xml: Remove virXMLPropStringLimit and virXPathStringLimit
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(a)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
8:37 a.m.
New subject: [PATCH 12/12] util: xml: Remove virXMLPropStringLimit and virXPathStringLimit
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(a)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(a)redhat.com>
Jano
1095
days inactive
1096
days old
30 comments
3 participants
participants (3)
-
Ján Tomko -
Peter Krempa -
Tim Wiederhake