[libvirt] [PATCH] network: check for invalid forward delay time

[John Ferlan]

On 09/08/2014 10:49 AM, Erik Skultety wrote:
> When spanning tree protocol is allowed in bridge settings, forward delay
> value is set as well (default is 0 if omitted). Until now, there was no
> check for delay value validity. Delay makes sense only as a positive
> numerical value.
> 
> Note: However, even if you provide positive  numerical value, brctl
> utility only uses values from range <2,30>, so the number provided can
> be modified (kernel most likely) to fall within this range.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1125764
> ---
>  docs/schemas/network.rng |  2 +-
>  src/conf/network_conf.c  | 18 +++++++++++++-----
>  src/util/virxml.c        |  2 +-
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 0e7da89..ab41814 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -62,7 +62,7 @@
>  
>              <optional>
>                <attribute name="delay">
> -                <data type="integer"/>
> +                <data type="unsignedLong"/>

Changing the schema - not sure this is supposed to be changed since
we've already released as 'integer'.

Although unsignedLong is what the 'delay' is defined as in
network_conf.h - so technically it's correct.

>                </attribute>
>              </optional>
>  
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 9571ee1..4d6db8c 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -2016,6 +2016,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>      xmlNodePtr save = ctxt->node;
>      xmlNodePtr bandwidthNode = NULL;
>      xmlNodePtr vlanNode;
> +    int ret = 0;
>  
>      if (VIR_ALLOC(def) < 0)
>          return NULL;
> @@ -2078,8 +2079,15 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>      stp = virXPathString("string(./bridge[1]/@stp)", ctxt);
>      def->stp = (stp && STREQ(stp, "off")) ? false : true;
>  
> -    if (virXPathULong("string(./bridge[1]/@delay)", ctxt, &def->delay) < 0)
> -        def->delay = 0;
> +    ret = virXPathULong("string(./bridge[1]/@delay)", ctxt, &def->delay);
> +        if (ret == -2) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("Invalid delay value in network '%s'"),
> +                           def->name);
> +            goto error;
> +        } else if (ret < 0) {
> +            def->delay = 0;
> +        }

See [1] below...

You probably want to make a virStrToLong_ulp() directly here -


tmp = virXPathString("string(./bridge[1]/@delay)", ctxt);
if (tmp) {
    if (virStrToLong_ulp(tmp, NULL, 10, &def->delay)) {
        virReportError(VIR_ERR_XML_ERROR,
                       _("Invalid delay value in network '%s'"),
                       def->name);
        VIR_FREE(tmp);
        goto error;
    }
    VIR_FREE(tmp);
}

Although the extra VIR_FREE(tmp) in the error path could be eliminated
from this and other paths by setting tmp = NULL at the start and adding
one in the 'error:' label.

>  
>      tmp = virXPathString("string(./mac[1]/@address)", ctxt);
>      if (tmp) {
> @@ -2126,7 +2134,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>              goto error;
>          /* parse each portgroup */
>          for (i = 0; i < nPortGroups; i++) {
> -            int ret = virNetworkPortGroupParseXML(&def->portGroups[i],
> +            ret = virNetworkPortGroupParseXML(&def->portGroups[i],
>                                                    portGroupNodes[i], ctxt);
>              if (ret < 0)
>                  goto error;
> @@ -2147,7 +2155,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>              goto error;
>          /* parse each addr */
>          for (i = 0; i < nIps; i++) {
> -            int ret = virNetworkIPDefParseXML(def->name, ipNodes[i],
> +            ret = virNetworkIPDefParseXML(def->name, ipNodes[i],
>                                                ctxt, &def->ips[i]);
>              if (ret < 0)
>                  goto error;
> @@ -2168,7 +2176,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>              goto error;
>          /* parse each definition */
>          for (i = 0; i < nRoutes; i++) {
> -            int ret = virNetworkRouteDefParseXML(def->name, routeNodes[i],
> +            ret = virNetworkRouteDefParseXML(def->name, routeNodes[i],
>                                                ctxt, &def->routes[i]);

These each could just be :

if (func(args) < 0)
    goto error;

if you were going to change them anyway (since ret isn't used)

>              if (ret < 0)
>                  goto error;
> diff --git a/src/util/virxml.c b/src/util/virxml.c
> index cc4a85c..f730f5e 100644
> --- a/src/util/virxml.c
> +++ b/src/util/virxml.c
> @@ -286,7 +286,7 @@ virXPathULongBase(const char *xpath,
>      ctxt->node = relnode;
>      if ((obj != NULL) && (obj->type == XPATH_STRING) &&
>          (obj->stringval != NULL) && (obj->stringval[0] != 0)) {
> -        if (virStrToLong_ul((char *) obj->stringval, NULL, base, value) < 0)
> +        if (virStrToLong_ulp((char *) obj->stringval, NULL, base, value) < 0)

[1]
I believe this will be far too generic as there are a set of commands
that use -1 as a "feature" to indicate everything or all the time or in
some manner the max value.

There's quite a few callers of the callers to virXPathULongBase() that
would need to be checked to see if they "desire" allowing a negative value

There's some "related" commitid's you may want to look at '37e663ad,
'0e2d7305', 'c6212539'


John

>              ret = -2;
>      } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) &&
>                 (!(isnan(obj->floatval)))) {
>