On Wed, 20 Dec 2017 09:37:31 +0200
Serhey Popovych <serhe.popov...@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.popov...@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.
