[libvirt] [PATCH v4] 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.
...
> v4: Return to using sscanf, drop inet functions in favor of virSocket,
>     parsing safety checks. Don't make parse failures fatal, in case
>     expected format changes.
...

Hi Cole,

This is better indeed.  Thanks.

> +    while (cur) {
> +        char dest[128], iface[17], mask[128];

Please reorder to match the expected order/use below:

           char iface[17], dest[128], mask[128];

> +        unsigned int addr_val, mask_val;
> +        int num;
> +
> +        cur++;
> +        num = sscanf(cur, "%16s %127s %*s %*s %*s %*s %*s %127s",
> +                     iface, dest, mask);

This is fine, except when there are fewer than 8 columns.
When that happens, it will take additional items from the next line.
You can avoid that by NUL-terminating the line.  i.e.,
put this just before the sscanf:

           /* NUL-terminate the line, so sscanf doesn't go beyond a newline.  */
           char *nl = strchr(cur, '\n');
           if (nl)
               *nl = '\0';


> +        if (num != 3) {
> +            VIR_DEBUG("Failed to parse %s", PROC_NET_ROUTE);
> +            break;
> +        }
> +
> +        if (virStrToLong_ui(dest, NULL, 16, &addr_val) < 0) {
> +            VIR_DEBUG("Failed to convert network address %s to uint", dest);
> +            break;
> +        }
> +
> +        if (virStrToLong_ui(mask, NULL, 16, &mask_val) < 0) {
> +            VIR_DEBUG("Failed to convert network mask %s to uint", mask);
> +            break;
> +        }

Each of the three tests above does a "break" upon surprising input,
which effectively discards all remaining lines in the file.
It would be more consistent to skip only the offending line,
so that this function behaves the same when a single offending
line is at the end of the file as when it's at the beginning.
So, s/break/continue/ ?

> +        addr_val &= mask_val;
> +
> +        if ((net_dest == addr_val) &&
> +            (innetmask.inet4.sin_addr.s_addr == mask_val)) {
> +            networkReportError(VIR_ERR_INTERNAL_ERROR,
> +                              _("Network %s/%s is already in use by "
> +                                "interface %s"),
> +                                network->def->ipAddress,
> +                                network->def->netmask, iface);
> +            goto error;
> +        }
> +
> +        cur = strchr(cur, '\n');

Then you can change this:

           cur = nl;

> +    }
> +
> +out:
> +    ret = 0;
> +error:
> +    VIR_FREE(buf);
> +    return ret;
> +}

Either way, ACK.
The above matter only for "surprising" inputs.