[PATCH 08/12] virSecurityLabelDefParseXML: Don't use 'virXPathStringLimit'

[Tim Wiederhake]

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