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 <pkre...@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);