[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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.


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 @@

                <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.
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)) {
                        _("Invalid delay value in network '%s'"),
         goto error;

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)

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'


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

libvir-list mailing list
libvir-list redhat com

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]