[libvirt] [PATCH v3] network: bridge: Don't start network if it collides with host routing

[Jim Meyering]

Cole Robinson wrote:
> Fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=235961
>
> If using the default virtual network, an easy way to lose guest network
> connectivity is to install libvirt inside the VM. The autostarted
> default network inside the guest collides with host virtual network
> routing. This is a long standing issue that has caused users quite a
> bit of pain and confusion.
>
> On network startup, parse /proc/net/route and compare the requested
> IP+netmask against host routing destinations: if any matches are found,
> refuse to start the network.
>
> v2: Drop sscanf, fix a comment typo, comment that function could use
>     libnl instead of /proc
>
> v3: Consider route netmask. Compare binary data rather than convert to
>     string.
...
> +static int networkCheckRouteCollision(virNetworkObjPtr network)
> +{
> +    int ret = -1, len;
> +    char *cur, *buf = NULL;
> +    enum {MAX_ROUTE_SIZE = 1024*64};
> +    struct in_addr inaddress, innetmask;
> +    char netaddr[32];
> +
> +    if (!network->def->ipAddress || !network->def->netmask)
> +        return 0;
> +
> +    if (inet_pton(AF_INET, network->def->ipAddress, &inaddress) <= 0) {
> +        networkReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("cannot parse IP address '%s'"),
> +                           network->def->ipAddress);
> +        goto error;
> +    }
> +    if (inet_pton(AF_INET, network->def->netmask, &innetmask) <= 0) {
> +        networkReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("cannot parse netmask '%s'"),
> +                           network->def->netmask);
> +        goto error;
> +    }
> +
> +    inaddress.s_addr &= innetmask.s_addr;
> +    if (!inet_ntop(AF_INET, &inaddress, netaddr, sizeof(netaddr))) {
> +        virReportSystemError(errno, "%s",
> +                             _("failed to format network address"));
> +        goto error;
> +    }
> +
> +    /* Read whole routing table into memory */
> +    if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0)
> +        goto error;

> +    /* Dropping the last character shouldn't hurt */

Hi Cole,

Not sure how much you want to invest in manual parsing,
but in case this code ends up staying with us for a while,
here are some suggestions.

Handle the case in which the file is empty.
This will appease static checkers like clang and coverity.

Either change the "< 0" test above to "<= 0"
or do e.g.,

  if (len > 0)

> +    buf[len-1] = '\0';

Future proof it by skipping that first line only
if it looks like the heading we see now:

  if (STRPREFIX (buf, "Iface"))

> +    /* First line is just headings, skip it */
> +    cur = strchr(buf, '\n');
> +
> +    while (cur) {
> +        char *data[8];
> +        char *dest, *iface, *mask;
> +        unsigned int addr_val, mask_val;
> +        int i;

It's slightly better to make "i" unsigned.

The following assumes no leading white space, which is currently true
at least on F13.  Future/portability-proof it by skipping leading white space:

         while (c_isblank (*cur))
             cur++;

> +        cur++;
> +
> +        /* Delimit interface field */
> +        for (i = 0; i < sizeof(data); ++i) {

Oops.  Won't this make "i" iterate from 0 up to 31 or 63,
depending on sizeof char*?

You probably mean this:
           for (i = 0; i < ARRAY_CARDINALITY(data); ++i) {

> +            data[i] = cur;

Otherwise, this would overrun the 8-element "data" array
and clobber the stack.

> +            /* Parse fields and delimit */
> +            while(*cur > ' ') {

Using "> ' '" works as long as each line has 8 or more fields.
If there are fewer, it would treat the newline just like a regular field
separator and continue getting fields from the next line.
If you use c_isblank you have to handle end of line differently,
but that's a good thing.

How about using c_isblank here, too, rather than relying on
ASCII "space" being smaller than "interesting" byte codes?
On F13, the fields of /proc/net/route are TAB-delimited.
Besides, I think y

               while (*cur && !c_isblank (*cur)) {

> +                cur++;
> +            }
> +            *cur++ = '\0';

You'll want something here to keep "cur" from going more than 1 beyond
the end of the buffer, in case the last row has fewer than 8 columns.

> +        }
> +
> +        iface = data[0];
> +        dest  = data[1];
> +        mask  = data[7];

If a line had fewer than 8 columns, at least mask is not valid.
Similarly for dest if there are fewer than two columns.

> +        if (virStrToLong_ui(dest, NULL, 16, &addr_val) < 0) {
> +            networkReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Failed to convert network address %s"),
> +                               dest);
> +            goto error;
> +        }