[libvirt] [PATCH] Coverity fix for virNetDevIPCheckIPv6ForwardingCallback
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
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
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.
participants (3)
-
Cédric Bosdonnat -
Laine Stump -
Peter Krempa