[PATCH] virMacAddrParse: Fix wrong termination character

[Peter Krempa]

On Thu, Jun 16, 2022 at 14:09:06 +0800, Eustance Wu wrote:
> From ef22e53c9360ddb4bdff61a12013a2812fb7346a Mon Sep 17 00:00:00 2001
> From: longtao <longtao.wu at zstack.io>
> Date: Thu, 16 Jun 2022 14:08:14 +0800
> Subject: [PATCH] virMacAddrParse: Fix wrong termination character
> 
> The judgment of the termination character should be the '\0' character, not
> a space.
> Using spaces to judge, content can be injected into mac. such as:
> "70:af:e7:1f:3f:89\32injected".
> 
> Before this patch, the terminating character was a space ('\32'),not '\0'.

This sentence is inaccurate, it was any byte between 0 and ' '.

> So I can set the DHCP host mac like this "<host mac='c0:3b:04:21:15:35
>  injected' name='name129' ip='192.168.100.129'/>".

This format does not conform to our XML schema. The 'mac' attribute is
defined as:

  <define name="uniMacAddr">
    <data type="string">
      <param name="pattern">[a-fA-F0-9][02468aAcCeE](:[a-fA-F0-9]{2}){5}</param>
    </data>
  </define>

Thus 'space' or any other character is _not_ allowed in int including
the word 'injected'.

What are you actually trying to achieve? This is not clear from the
description in this bug.

Please describe also the end goal ...

> When running the network, no error is reported.
> But, when using this mac to create a virtual machine,  Will get
> "virNetSocketReadWire:1805 : End of file while reading data: Input/output

... rather than just an error you are seeing, since this doesn't seem to
be a plain bugfix.

> error" in the libvirtd log.
> ---
>  src/util/virmacaddr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> index 6b22384cee..ba7c7e7076 100644
> --- a/src/util/virmacaddr.c
> +++ b/src/util/virmacaddr.c
> @@ -163,7 +163,7 @@ virMacAddrParse(const char* str, virMacAddr *addr)

This helper is used in many places, outside of the network driver, so
this will need careful assesment whether any other code depends on the
old behaviour.

> 
>          addr->addr[i] = (unsigned char) result;
> 
> -        if ((i == 5) && (*end_ptr <= ' '))
> +        if ((i == 5) && (*end_ptr == 0))

We prefer to use the character literal version of the nul byte ('\0').

>              return 0;
>          if (*end_ptr != ':')
>              break;
> -- 
> 2.32.0