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);