Re: [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);