Re: [PATCH iproute2 3/3] utils: ll_map: Make network device name fixed size array of char

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 20 Dec 2017 09:37:31 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> Network device names are fixed in size and never exceed
> IFNAMSIZ (16 bytes).
> 
> Make name fixed size array to always malloc() same size chunk
> of memory and use memcpy()/memcmp() with constant IFNAMSIZ
> to benefit from possible compiler optimizations replacing
> call to a function with two/four load/store instructions
> on 64/32 bit systems.
> 
> Check if IFLA_IFNAME attribute present in netlink message
> (should always) and use strncpy() to pad name with zeros.
> 
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
> ---
>  lib/ll_map.c |   20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/ll_map.c b/lib/ll_map.c
> index abe7bdc..fcbf0fb 100644
> --- a/lib/ll_map.c
> +++ b/lib/ll_map.c
> @@ -30,7 +30,7 @@ struct ll_cache {
>  	unsigned	flags;
>  	unsigned 	index;
>  	unsigned short	type;
> -	char		name[];
> +	char		name[IFNAMSIZ];
>  };
>  
>  #define IDXMAP_SIZE	1024
> @@ -71,7 +71,7 @@ static struct ll_cache *ll_get_by_name(const char *name)
>  		struct ll_cache *im
>  			= container_of(n, struct ll_cache, name_hash);
>  
> -		if (strncmp(im->name, name, IFNAMSIZ) == 0)
> +		if (!strcmp(im->name, name))
>  			return im;
>  	}
>  
> @@ -82,7 +82,7 @@ int ll_remember_index(const struct sockaddr_nl *who,
>  		      struct nlmsghdr *n, void *arg)
>  {
>  	unsigned int h;
> -	const char *ifname;
> +	char ifname[IFNAMSIZ];
>  	struct ifinfomsg *ifi = NLMSG_DATA(n);
>  	struct ll_cache *im;
>  	struct rtattr *tb[IFLA_MAX+1];
> @@ -105,17 +105,21 @@ int ll_remember_index(const struct sockaddr_nl *who,
>  	}
>  
>  	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), IFLA_PAYLOAD(n));
> -	ifname = rta_getattr_str(tb[IFLA_IFNAME]);
> -	if (ifname == NULL)
> +
> +	if (!tb[IFLA_IFNAME])
> +		return 0;
> +	strncpy(ifname, rta_getattr_str(tb[IFLA_IFNAME]), IFNAMSIZ);
> +	if (!ifname[0])
>  		return 0;
> +	ifname[IFNAMSIZ - 1] = '\0';
>  
>  	if (im) {
>  		/* change to existing entry */
> -		rehash = strcmp(im->name, ifname);
> +		rehash = memcmp(im->name, ifname, IFNAMSIZ);

This is not safe. There is not guarantee that bytes after end of string are zero.
And in your code, strncpy() will overwrite characters from the beginning to null,
it will not overwrite after that. Then comparison with entry may not work because
of the data after that.

I really doubt this is critical path on anything. Probably just having a better
hash table implementation would solve that.