On Wed, 10 Jul 2019 15:13:02 -0400
Aaron Conole <acon...@redhat.com> wrote:
> Stephen Hemminger <step...@networkplumber.org> writes:
>
> > On Wed, 10 Jul 2019 14:33:42 -0400
> > Aaron Conole <acon...@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 <msant...@redhat.com>
> >> Fixes: 596d31092d32 ("net: add function to convert string to ethernet
> >> address")
> >> Signed-off-by: Aaron Conole <acon...@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);
}
