[libvirt] [PATCH] network: check for invalid forward delay time
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"/> </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; + } 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]); 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) ret = -2; } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) && (!(isnan(obj->floatval)))) { -- 1.9.3
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)))) {
Thank you John for your review, however, before I send another patch I'd like to know your (or someone else's) opinion on my notes/thoughts I posted below. Erik On 09/08/2014 10:16 PM, John Ferlan wrote:
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.
Well, the only reason why I wanted to change it in the schema is that there is an idea to do the schema check not only as a part of unit testing, but to do it after XML domain editing (future feature) as well. In that case, I think it might be correct, if we would stay consistent with our definitions.
</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); }
Yep, much better idea, although I went through possible callers of virXPathULong, I think most of them (but not all) do not honor the feature -1 being the max/all/everything. Now the problem is that we do not check for negative values, we leave them as they are. If we fixed it at one specific place, then we should stay consistent and do the check on all other places. What's more, even if we did it (not sure if worth the effort), we do not check for reasonable ranges, so in the end, whether we checked for negative values or not, we'd end up with huge values to be set which get ignored by qemu or kernel.
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.
Good tip, I found a couple of lines where we actually forgot to free tmp in case of an error, thanks.
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)
Sure, you're right.
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Erik Skultety -
John Ferlan