On 03/23/2017 04:44 AM, Cedric Bosdonnat wrote:
On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
[...]
+ +static int +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, + void *opaque) +{ + struct rtmsg *rtmsg = NLMSG_DATA(resp); + int accept_ra = -1; + struct rtattr *rta; + char *ifname = NULL; + struct virNetDevIPCheckIPv6ForwardingData *data = opaque; + int ret = 0; + int len = RTM_PAYLOAD(resp); + int oif = -1; + + /* Ignore messages other than route ones */ + if (resp->nlmsg_type != RTM_NEWROUTE) + return ret; + + /* Extract a few attributes */ + for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { + switch (rta->rta_type) { + case RTA_OIF: + oif = *(int *)RTA_DATA(rta); + + if (!(ifname = virNetDevGetName(oif))) + goto error; + break;
Did you really mean to break from the for loop if ifname is set? This breaks only from the switch/case. Of course Coverity doesn't know much more than you'd be going back to the top of the for loop and could overwrite ifname again. It proclaims a resource leak...
In my dev version I was also getting the RTA_DST attribute to print some debugging message that I removed in the submitted version. The break was here between the switch cases.
In the current case I don't really care if we break out of the loop or not. As there aren't that many attributes in an rtnetlink message to loop over, breaking wouldn't gain a lot of cycles.
What I don't get though is what this break is actually doing? isn't it for the switch case even though there is no other case after it?
-- Cedric
So should this be changed to: if (rta->rta_type == RTA_OIF) { oif = *(int *)RTA_DATA(rta); if (!(ifname = virNetDevGetName(oif))) goto error; break; } leaving two questions in my mind 1. Can there be more than one RTA_OIF 2. If we don't finish the loop (e.g. we break out), then does one of the subsequent checks fail? Is it more of a problem that we find *two* RTA_OIF's and thus should add a: if (ifname) { VIR_DEBUG("some sort of message"); goto error; } prior to the virNetDevGetName call and then remove the break;? John (who doesn't know the answer!) BTW: The issue from patch4 is resolved by my 22 patch bomb from yesterday
John
+ } + } + + /* No need to do anything else for non RA routes */ + if (rtmsg->rtm_protocol != RTPROT_RA) + goto cleanup; + + data->hasRARoutes = true; + + /* Check the accept_ra value for the interface */ + accept_ra = virNetDevIPGetAcceptRA(ifname); + VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); + + if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0) + goto error; + + cleanup: + VIR_FREE(ifname); + return ret; + + error: + ret = -1; + goto cleanup; +}