[Libvir] [PATCH] Fix MAC address parsing for 1-digit case

[Jim Meyering]

Hiroyuki Kaguchi <fj7025cf at aa.jp.fujitsu.com> wrote:
> libvirt fails in parsing when MAC address like "00:16:3e:12:3:61"
> is specified for installation.
> This is because virt-install can pass 1-digit (like 3) for
> MAC address from Cset:316 for Solaris
> But libvirt cannot support this MAC 1-digit (like 3) parameter.

Thanks for the patch.
Any change that eliminates a sscanf-based parser is a good one ;-)

A few suggestions:

...
> +static int
> +parseMacAddr(const char* str, unsigned char *addr)
> +{
> +    int i;

Initialize errno to 0 before the loop.
Otherwise, an incoming nonzero value could cause rejection
of a valid MAC address.

> +    for (i = 0; i < 6; i++) {
> +        char *end_ptr;
> +        unsigned long result;
> +
> +        if (!isdigit(*str) && !isalpha(*str))
> +            break;

Use isxdigit instead, and add a comment
saying this is solely to avoid accepting the leading
space or "+" that strtoul would otherwise accept.

           if (!isxdigit(*str))
               break;

> +
> +        result = strtoul(str, &end_ptr, 16);
> +
> +        if ((end_ptr - str) < 1 || 2 < (end_ptr - str) ||
> +            (errno == ERANGE) ||

While it's ok to test errno == ERANGE,
testing errno != 0 is a little better.

> +            (0xFF < result))
> +            break;
> +
> +        addr[i] = (unsigned char) result;

The following test ensures that there is a ":" between adjacent
pairs, but does not require a terminating NUL byte.  That means
it would accept arbitrary text after a valid sixth component,
e.g., "00:16:3e:12:3:61-anything-at-all" or a less-contrived
s/0/O/ typo, e.g., "00:16:3e:12:3:6O"

> +        if (*end_ptr != ':')
> +            return (i == 5) ? 0 : -1;

You can use this instead:

           if (*end_ptr != (i < 5 ? ':' : '\0'))
               return -1;
           if (i == 5)
               return 0;

...
>      virBufferAddLit(buf, "(vif ");
>      if (mac != NULL) {
> -        unsigned int addr[12];
> -        int tmp = sscanf((const char *) mac,
> -		     "%01x%01x:%01x%01x:%01x%01x:%01x%01x:%01x%01x:%01x%01x",
> -                         (unsigned int *) &addr[0],
> -                         (unsigned int *) &addr[1],
> -                         (unsigned int *) &addr[2],
> -                         (unsigned int *) &addr[3],
> -                         (unsigned int *) &addr[4],
> -                         (unsigned int *) &addr[5],
> -                         (unsigned int *) &addr[6],
> -                         (unsigned int *) &addr[7],
> -                         (unsigned int *) &addr[8],
> -                         (unsigned int *) &addr[9],
> -                         (unsigned int *) &addr[10],
> -                         (unsigned int *) &addr[11]);
> -        if (tmp != 12 || strlen((const char *) mac) != 17) {
> +        unsigned char addr[6];
> +        if (parseMacAddr((const char*) mac, addr) == -1) {
>              virXMLError(conn, VIR_ERR_INVALID_MAC, (const char *) mac, 0);
>              goto error;
>          }