From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v1 2/3] net/hyperv: implement core functionality Date: Tue, 19 Dec 2017 11:01:55 +0100 Message-ID: <20171219100155.GB5377@6wind.com> References: <20171124172132.GW4062@6wind.com> <20171218162443.12971-1-adrien.mazarguil@6wind.com> <20171218162443.12971-3-adrien.mazarguil@6wind.com> <20171218155946.5806589a@xeon-e3> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Ferruh Yigit , dev@dpdk.org To: Stephen Hemminger Return-path: Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id 269DC1B016 for ; Tue, 19 Dec 2017 11:02:08 +0100 (CET) Received: by mail-wm0-f42.google.com with SMTP id 64so2351008wme.3 for ; Tue, 19 Dec 2017 02:02:08 -0800 (PST) Content-Disposition: inline In-Reply-To: <20171218155946.5806589a@xeon-e3> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Dec 18, 2017 at 03:59:46PM -0800, Stephen Hemminger wrote: > On Mon, 18 Dec 2017 17:46:23 +0100 > Adrien Mazarguil wrote: > > > +static int > > +ether_addr_from_str(struct ether_addr *eth_addr, const char *str) > > +{ > > + static const uint8_t conv[0x100] = { > > + ['0'] = 0x80, ['1'] = 0x81, ['2'] = 0x82, ['3'] = 0x83, > > + ['4'] = 0x84, ['5'] = 0x85, ['6'] = 0x86, ['7'] = 0x87, > > + ['8'] = 0x88, ['9'] = 0x89, ['a'] = 0x8a, ['b'] = 0x8b, > > + ['c'] = 0x8c, ['d'] = 0x8d, ['e'] = 0x8e, ['f'] = 0x8f, > > + ['A'] = 0x8a, ['B'] = 0x8b, ['C'] = 0x8c, ['D'] = 0x8d, > > + ['E'] = 0x8e, ['F'] = 0x8f, [':'] = 0x40, ['-'] = 0x40, > > + ['\0'] = 0x60, > > + }; > > + uint64_t addr = 0; > > + uint64_t buf = 0; > > + unsigned int i = 0; > > + unsigned int n = 0; > > + uint8_t tmp; > > + > > + do { > > + tmp = conv[(int)*(str++)]; > > Cast to int will cause out of bounds reference on non-ascii strings. > The parser will get confused by: > 001:aa:bb:cc:dd:ee:ff or invalid strings. Nice catch! I added the (int) cast to shut up a GCC complaint about using char as index type. The proper fix taking care of integer conversion and array bounds safety check should read: tmp = conv[*str++ & 0xffu]; > Why not use sscanf which would be safer in this case. Right, this is indeed the obvious implementation, however not only the fixed MAC-48 format is not the most convenient to use for user input (somewhat like forcing them to enter fully expanded IPv6 addresses every time), sscanf() also ignores leading white spaces and successfully parses weird expressions like " -42: 0x66: 0af: 0: 44:-6", which I think is a problem. > /** > * Parse 48bits Ethernet address in pattern xx:xx:xx:xx:xx:xx. > * > * @param eth_addr > * A pointer to a ether_addr structure. > * @param str > * A pointer to string contains the formatted MAC address. > * @return > * 0 if the address is valid > * -EINVAL if address is not formatted properly > */ > static inline int > ether_parse_addr(struct ether_addr *eth_addr, const char *str) > { > int n; > > n = sscanf(str, > "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx", > ð_addr->addr_bytes[0], > ð_addr->addr_bytes[1], > ð_addr->addr_bytes[2], > ð_addr->addr_bytes[3], > ð_addr->addr_bytes[4], > ð_addr->addr_bytes[5]); > return (n == ETHER_ADDR_LEN) ? 0 : -EINVAL; > } > > > + if (!tmp) > > + return -EINVAL; > > + if (tmp & 0x40) { > > + i += (i & 1) + (!i << 1); > > + addr = (addr << (i << 2)) | buf; > > + n += i; > > + buf = 0; > > + i = 0; > > + } else { > > + buf = (buf << 4) | (tmp & 0xf); > > + ++i; > > + } > > + } while (!(tmp & 0x20)); > > + if (n > 12) > > + return -EINVAL; > > + i = RTE_DIM(eth_addr->addr_bytes); > > + while (i) { > > + eth_addr->addr_bytes[--i] = addr & 0xff; > > + addr >>= 8; > > + } > > + return 0; > > +} > > + -- Adrien Mazarguil 6WIND