Add check for more than one RTA_OIF, even though this is rather
unlikely and get rid of the buggy switch / break.
---
 src/util/virnetdevip.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index c9ac6baf7..f5662413a 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -556,15 +556,17 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
     if (resp->nlmsg_type != RTM_NEWROUTE)
         return ret;
 
-    /* Extract a few attributes */
+    /* Extract a device ID attribute */
     for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
-        switch (rta->rta_type) {
-        case RTA_OIF:
+        if (rta->rta_type == RTA_OIF) {
             oif = *(int *)RTA_DATA(rta);
 
+            /* Should never happen: netlink message would be broken */
+            if (ifname)
+                goto error;
+
             if (!(ifname = virNetDevGetName(oif)))
                 goto error;
-            break;
         }
     }
 
-- 
2.12.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Mar 24, 2017 at 13:04:07 +0100, Cédric Bosdonnat wrote:
> Add check for more than one RTA_OIF, even though this is rather
> unlikely and get rid of the buggy switch / break.
> ---
>  src/util/virnetdevip.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Making coverity happy is a weak justification.

> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index c9ac6baf7..f5662413a 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -556,15 +556,17 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
>      if (resp->nlmsg_type != RTM_NEWROUTE)
>          return ret;
>  
> -    /* Extract a few attributes */
> +    /* Extract a device ID attribute */
>      for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> -        switch (rta->rta_type) {
> -        case RTA_OIF:
> +        if (rta->rta_type == RTA_OIF) {

This removes future expandability.

>              oif = *(int *)RTA_DATA(rta);
>  
> +            /* Should never happen: netlink message would be broken */
> +            if (ifname)
> +                goto error;

This is weird. I know it's in a loop, but this jumps out without
reporting an error, which would make debugging even harder than in case
of a leak. 

> +
>              if (!(ifname = virNetDevGetName(oif)))
>                  goto error;
> -            break;
>          }
>      }
>  
> -- 
> 2.12.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 03/24/2017 08:10 AM, Peter Krempa wrote:
> On Fri, Mar 24, 2017 at 13:04:07 +0100, Cédric Bosdonnat wrote:
>> Add check for more than one RTA_OIF, even though this is rather
>> unlikely and get rid of the buggy switch / break.
>> ---
>>  src/util/virnetdevip.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> Making coverity happy is a weak justification.

Tell that to John! :-P

Seriously though, although it was Coverity that pointed out the problem,
it is a bonafide problem that needs to be fixed.

> 
>> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
>> index c9ac6baf7..f5662413a 100644
>> --- a/src/util/virnetdevip.c
>> +++ b/src/util/virnetdevip.c
>> @@ -556,15 +556,17 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
>>      if (resp->nlmsg_type != RTM_NEWROUTE)
>>          return ret;
>>  
>> -    /* Extract a few attributes */
>> +    /* Extract a device ID attribute */
>>      for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
>> -        switch (rta->rta_type) {
>> -        case RTA_OIF:
>> +        if (rta->rta_type == RTA_OIF) {
> 
> This removes future expandability.

It's doubtful this function will ever need much, if any, expansion. It's
not a general purpose thing that needs to have a case added for every
new possible value of rta_type. Instead, it's just looking for a single
type of message attribute, so I think it is appropriate that it be done
with a simple if() rather than a switch().

> 
>>              oif = *(int *)RTA_DATA(rta);
>>  
>> +            /* Should never happen: netlink message would be broken */
>> +            if (ifname)
>> +                goto error;
> 
> This is weird. I know it's in a loop, but this jumps out without
> reporting an error, which would make debugging even harder than in case
> of a leak. 

Agreed - if ifname has already been set, then we should log an error.

Or actually I think it would be better to instead do a VIR_WARN() and
continue (which might be a nicer thing to do, since any "error" logged
here would be an error it would be impossible for the user to correct,
so they would be unable to start the network until libvirt's code was
changed to deal with this unexpected occurence.

Or instead  we could just break from the loop as soon as we find one
RTA_OIF, and not even look for any others. But since it's so simple to
check for duplicates, I say we should do that - just put in something like:

    if (ifname) {
        char *ifname2 = virNetDevGetName(oif);
        VIR_WARN("Single route has unexpected 2nd interface "
                 "- '%s' and '%s'");
        VIR_FREE(ifname2);
        break;
    }

That way we might learn about it if it ever happened, and could
reevaluate our code, but we wouldn't doom someone to a non-working system.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list