details: https://lore.kernel.org/all/CAFEAcA831S0wGbyLwDK7yVeEoi1SFPD7gpYribNqP6AmyQHN5A@mail.gmail.com/

 

 

On Tue, 23 Jul 2024 at 08:03, Konstantin Kostiuk <kkostiuk@redhat.com> wrote:
>
> From: Dehan Meng <demeng@redhat.com>
>
> The Route information of the Linux VM needs to be used
> by administrators and users when debugging network problems
> and troubleshooting.
>
> Signed-off-by: Dehan Meng <demeng@redhat.com>
> Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
> Message-ID: <20240613092802.346246-2-demeng@redhat.com>
> Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>

Hi; Coverity points out some potential issues with this commit:

> +static char *hexToIPAddress(const void *hexValue, int is_ipv6)
> +{
> +    if (is_ipv6) {
> +        char addr[INET6_ADDRSTRLEN];
> +        struct in6_addr in6;
> +        const char *hexStr = (const char *)hexValue;
> +        int i;
> +
>         for (i = 0; i < 16; i+) {
> +            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);

We don't check the sscanf() return value here. (CID 1558558)
 
The handling of the getline() buffer in this function doesn't
seem to be correct (CID 1558559).

Firstly, the manpage says that to get the initial "allocate me
a buffer", line must be NULL and also n must be 0, but we don't
initialize n here.

>     for (i = 0; i < 2; i+) {
> +        firstLine = 1;
> +        is_ipv6 = (i == 1);
> +        fp = fopen(routeFiles[i], "r");
> +        if (fp == NULL)

{ > +            error_setg_errno(errp, errno, "open(\"%s\")", routeFiles[i]); > +            free(line); Here we free() line, but we continue the for() loop. So next time around the loop (assuming the second fopen succeeds) we'll pass line to getline() and it will be a non-NULL pointer to freed memory. Is this error case supposed to exit the for() loop entirely instead of continuing? Either way, it shouldn't free(line) here I think. > +            continue; > +        }

> +
> +        while (getline(&line, &n, fp) != -1) {
> +            if (firstLine && !is_ipv6)

{ > +                firstLine = 0; > +                continue; > +            }

> +            GuestNetworkRoute *route = NULL;
> +            GuestNetworkRoute *networkroute;
> +            char Iface[IFNAMSIZ];

Our coding style says you shouldn't declare variables in the
middle of a block. Coding style also says variable names are
lowercase with underscores, not CamelCase. (CamelCase is for
typenames.)

> +            if (is_ipv6) {
> +                char Destination[33], Source[33], NextHop[33];
> +                int DesPrefixlen, SrcPrefixlen, Metric, RefCnt, Use, Flags;
> +
> +                /* Parse the line and extract the values */
> +                if (sscanf(line, "%32s %x %32s %x %32s %x %x %x %x %s",
> +                           Destination, &DesPrefixlen, Source,
> +                           &SrcPrefixlen, NextHop, &Metric, &RefCnt,
> +                           &Use, &Flags, Iface) != 10)

{ > +                    continue; > +                }

> +
> +                route = g_new0(GuestNetworkRoute, 1);
> +                networkroute = route;

Why do we have separate "route" and "networkroute" variables
here? As far as I can see they are identical and can be merged.
 
Similarly here we free(line) but next time around the for()
loop we'll pass it to getline anyway.

> +        fclose(fp);
> +    }

Since getline() will reallocate the buffer as needed, we don't
need to free it anywhere except right before we exit the
function, here.

> +
> +    return head;
> +}

thanks
– PMM