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)))) { > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list