Re: [PATCH v2] net/tap: resolve stringop-overflow with gcc 12 on ppc64le

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 3/23/2023 5:01 PM, David Christensen wrote:
> Building DPDK with gcc 12 on a ppc64le system generates a
> stringop-overflow warning. Replace the local MAC address
> validation function parse_user_mac() with a call to
> rte_ether_unformat_addr() instead.
> 
> Bugzilla ID: 1197
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> ---
> v2:
> * Added NULL checks previously performed in parse_user_mac()
> ---
>  drivers/net/tap/rte_eth_tap.c | 26 ++------------------------
>  1 file changed, 2 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 089ac202fa..8c50801fd4 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2267,29 +2267,6 @@ set_remote_iface(const char *key __rte_unused,
>  	return 0;
>  }
>  
> -static int parse_user_mac(struct rte_ether_addr *user_mac,
> -		const char *value)
> -{
> -	unsigned int index = 0;
> -	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte = NULL;
> -
> -	if (user_mac == NULL || value == NULL)
> -		return 0;
> -
> -	strlcpy(mac_temp, value, sizeof(mac_temp));
> -	mac_byte = strtok(mac_temp, ":");
> -
> -	while ((mac_byte != NULL) &&
> -			(strlen(mac_byte) <= 2) &&
> -			(strlen(mac_byte) == strspn(mac_byte,
> -					ETH_TAP_CMP_MAC_FMT))) {
> -		user_mac->addr_bytes[index++] = strtoul(mac_byte, NULL, 16);
> -		mac_byte = strtok(NULL, ":");
> -	}
> -
> -	return index;
> -}
> -
>  static int
>  set_mac_type(const char *key __rte_unused,
>  	     const char *value,
> @@ -2311,7 +2288,8 @@ set_mac_type(const char *key __rte_unused,
>  		goto success;
>  	}
>  
> -	if (parse_user_mac(user_mac, value) != 6)
> +	if (value == NULL || user_mac == NULL ||
> +			rte_ether_unformat_addr(value, user_mac) < 0)
>  		goto error;
>  success:
>  	TAP_LOG(DEBUG, "TAP user MAC param (%s)", value);

Hi David,

I confirm the build error, btw it helps to future references to put
build failure to the commit log,

and change is reasonable to convert PMD local parse function to an API,
BUT my concern is they don't behave exactly same, which changes user
interface of the driver.

The 'rte_ether_unformat_addr()' API expects exact "XX:XX:XX:XX:XX:XX or
XXXX:XXXX:XXXX" format.
Like 'parse_user_mac()' accepts 'a:a:a:a:a:a' as input, but API requires
'0A:0A:0A:0A:0A:0A'.

This is a small change but still may create a bad experience if an
existing user/script hit by this, and I believe we don't have a strong
reason to change the interface.


To keep behavior same, we can either update 'rte_ether_unformat_addr()'
to accept singe chars between ':',
or fix the existing 'parse_user_mac()' for compiler warning, what do you
think?