[libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding

[John Ferlan]

[...]

> +
> +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...

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;
> +}