On Wed, Jul 17, 2019 at 10:04:10AM -0400, Aaron Conole wrote:
> Stephen Hemminger <step...@networkplumber.org> writes:
>
> > On Tue, 16 Jul 2019 01:17:56 +0000
> > "Li, WenjieX A" <wenjiex.a...@intel.com> wrote:
> >
> >> Hi Stephen,
> >>
> >>
> >>
> >> This DPDK patch makes cmdline_autotest failed. List the details as below.
> >>
> >> Could you please help to resolve it?
> >>
> >> Thank you!
> >> Test Setup
> >> Steps to reproduce
> >> List the steps to reproduce the issue.
> >> ./x86_64-native-linuxapp-gcc/app/test -n 1 -c 0xff
> >> testpmd> cmdline_autotest
> >> Show the output from the previous commands.
> >> Testind parsing ethernet addresses...
> >> Error: parsing 01:23:45:67:89:A succeeded!
> >> Test Failed
> >> Expected Result
> >> Explain what is the expected result in text or as an example output:
> >> Testind parsing ethernet addresses...
> >> Testind parsing port lists...
> >> Testind parsing numbers...
> >> Testing parsing IP addresses...
> >> Testing parsing strings...
> >> Testing circular buffer...
> >> Testing library functions...
> >> Test OK
> >>
> >>
> >>
> >> BR,
> >>
> >> Wenjie
> >>
> >
> > There are two possible solutions. Make the ether_unformat_addr function
> > more complex and more restrictive. The new version corresponds to the
> > FreeBSD
> > behavior.
>
> Not exactly. The new code accepts 2 forms (X:X:X:X:X:X which is
> FreeBSD, as well as X:X:X which is not). Because of this, there is a
> higher likelihood of ambiguity. I want to submit a patch to fix this
> ambiguity (but got pulled into a customer bug). Maybe someone else can?
I will have a look at it.
> I think it's best to make the code slightly more complex. I originally
> submitted a change just validating the string length[0], which is probably
> too restrictive.
>
> Maybe a better version would be something like the original (rewritten
> to ensure it supports things like XX:X:XX:X:XX:X format, also):
>
> {
> unsigned long o[ETHER_ADDR_LEN];
> char *end, *input = buf;
> size_t i = 0;
>
> do {
> errno = 0;
> o[i] = strtoul(input, &end, 16);
> if (errno != 0 || end == input || (end && *end != ':' && *end != 0))
> return ERROR;
> input = end+1;
> } while((input - buf < buflen) && ++i < ARRAY_SIZE(o) && *end != 0);
>
> if (*end != 0)
> return ERROR;
>
> if (i == 6) /*x:x:x:x:x:x*/
> else if (i == 3) /*x:x:x*/
> else return ERROR
> }
>
> WDYT?
>
> > The other possibility is just remove the test case.
>
> I don't like this approach - right now its possible that a user (doing
> c/p) can submit an ipv6 address which will be accepted as valid. I
> prefer to be validating the form. Even if XX:X:X:XX:X:X is okay,
> XXXX:XXXX:XX::X shouldn't be. There's a limit to how liberal we should
> be when accepting input.
>
> > I am leaning towards the latter as the least work.
>
> Least work isn't always the right thing.
>
> 0: http://mails.dpdk.org/archives/dev/2019-July/137827.html
