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

Erik Skultety eskultet at redhat.com
Tue Sep 9 09:51:22 UTC 2014


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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>




More information about the libvir-list mailing list