Re: [dpdk-dev] [PATCH] rte_ether: force format string for unformat_addr

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 10 Jul 2019 15:13:02 -0400
Aaron Conole <aconole@redhat.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
> > On Wed, 10 Jul 2019 14:33:42 -0400
> > Aaron Conole <aconole@redhat.com> wrote:
> >  
> >> rte_ether_unformation_addr is very lax in what it accepts now, including
> >> ethernet addresses formatted ambiguously as "x:xx:x:xx:x:xx".  However,
> >> previously this behavior was enforced via the my_ether_aton which would
> >> fail ambiguously formatted values.
> >> 
> >> Reported-by: Michael Santana <msantana@redhat.com>
> >> Fixes: 596d31092d32 ("net: add function to convert string to ethernet address")
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> ---
> >>  lib/librte_net/rte_ether.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/lib/librte_net/rte_ether.c b/lib/librte_net/rte_ether.c
> >> index 8d040173c..4f252b813 100644
> >> --- a/lib/librte_net/rte_ether.c
> >> +++ b/lib/librte_net/rte_ether.c
> >> @@ -45,7 +45,8 @@ rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea)
> >>  	if (n == 6) {
> >>  		/* Standard format XX:XX:XX:XX:XX:XX */
> >>  		if (o0 > UINT8_MAX || o1 > UINT8_MAX || o2 > UINT8_MAX ||
> >> -		    o3 > UINT8_MAX || o4 > UINT8_MAX || o5 > UINT8_MAX) {
> >> +		    o3 > UINT8_MAX || o4 > UINT8_MAX || o5 > UINT8_MAX ||
> >> +		    strlen(s) != RTE_ETHER_ADDR_FMT_SIZE - 1) {
> >>  			rte_errno = ERANGE;
> >>  			return -1;
> >>  		}
> >> @@ -58,7 +59,8 @@ rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea)
> >>  		ea->addr_bytes[5] = o5;
> >>  	} else if (n == 3) {
> >>  		/* Support the format XXXX:XXXX:XXXX */
> >> -		if (o0 > UINT16_MAX || o1 > UINT16_MAX || o2 > UINT16_MAX) {
> >> +		if (o0 > UINT16_MAX || o1 > UINT16_MAX || o2 > UINT16_MAX ||
> >> +		    strlen(s) != RTE_ETHER_ADDR_FMT_SIZE - 4) {
> >>  			rte_errno = ERANGE;
> >>  			return -1;
> >>  		}  
> >
> > NAK
> > Skipping leading zero should be ok. There is no need for this patch.  
> 
> Is it intended to skip the leading 0?  Why not the trailing 0?  I'm not
> familiar with the format that is used here  (example - X:XX:X:XX:X)
> 
> It isn't described in any RFC I could find (but I only did a small
> search).  Even in IEEE, the format is always a full octet.
> 
> > The current behavior is superset of what standard ether_aton accepts.  
> 
> Okay, but it introduces a test failure for the cmdline tests and then
> that test will need a few lines removed for 'unsuccessful' formats.
> 
> ether_aton is much more rigid in the formats it accepts, so the test
> case is enforcing that.  I guess either the current behavior of this
> function changes (and since it is a new behavior of the cmdline parser,
> I would think it should be changed) or the test case should be changed
> to adopt it.

BSD ether_aton is:
/*
 * Convert an ASCII representation of an ethernet address to binary form.
 */
struct ether_addr *
ether_aton_r(const char *a, struct ether_addr *e)
{
	int i;
	unsigned int o0, o1, o2, o3, o4, o5;

	i = sscanf(a, "%x:%x:%x:%x:%x:%x", &o0, &o1, &o2, &o3, &o4, &o5);
	if (i != 6)
		return (NULL);
	e->octet[0]=o0;
	e->octet[1]=o1;
	e->octet[2]=o2;
	e->octet[3]=o3;
	e->octet[4]=o4;
	e->octet[5]=o5;
	return (e);
}