Re: [dpdk-dev] [PATCH] net/memif: fix invalid unix domain address length

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 22 Oct 2019 18:08:29 +0200
Jakub Grajciar <jgrajcia@cisco.com> wrote:

> Cc: stephen@networkplumber.org
> 
> Signed-off-by: Jakub Grajciar <jgrajcia@cisco.com>
> ---
>  doc/guides/nics/memif.rst         |  2 +-
>  drivers/net/memif/memif_socket.c  | 27 +++++++++++----------------
>  drivers/net/memif/memif_socket.h  |  6 ++++--
>  drivers/net/memif/rte_eth_memif.c |  5 +++++
>  4 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/doc/guides/nics/memif.rst b/doc/guides/nics/memif.rst
> index de2d481eb..9a568455e 100644
> --- a/doc/guides/nics/memif.rst
> +++ b/doc/guides/nics/memif.rst
> @@ -42,7 +42,7 @@ client.
>     "role=master", "Set memif role", "slave", "master|slave"
>     "bsize=1024", "Size of single packet buffer", "2048", "uint16_t"
>     "rsize=11", "Log2 of ring size. If rsize is 10, actual ring size is 1024", "10", "1-14"
> -   "socket=/tmp/memif.sock", "Socket filename", "/tmp/memif.sock", "string len 256"
> +   "socket=/tmp/memif.sock", "Socket filename", "/tmp/memif.sock", "string len 108"
>     "mac=01:23:45:ab:cd:ef", "Mac address", "01:ab:23:cd:45:ef", ""
>     "secret=abc123", "Secret is an optional security option, which if specified, must be matched by peer", "", "string len 24"
>     "zero-copy=yes", "Enable/disable zero-copy slave mode", "no", "yes|no"
> diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memif_socket.c
> index 0c71f6c45..4efa68e1a 100644
> --- a/drivers/net/memif/memif_socket.c
> +++ b/drivers/net/memif/memif_socket.c
> @@ -7,7 +7,6 @@
>  #include <unistd.h>
>  #include <sys/types.h>
>  #include <sys/socket.h>
> -#include <sys/un.h>
>  #include <sys/ioctl.h>
>  #include <errno.h>
>  
> @@ -860,16 +859,12 @@ memif_listener_handler(void *arg)
>  		rte_free(cc);
>  }
>  
> -#define MEMIF_SOCKET_UN_SIZE	\
> -	(offsetof(struct sockaddr_un, sun_path) + MEMIF_SOCKET_KEY_LEN)
> -
>  static struct memif_socket *
>  memif_socket_create(struct pmd_internals *pmd,
>  		    const char *key, uint8_t listener)
>  {
>  	struct memif_socket *sock;
> -	struct sockaddr_un *un;
> -	char un_buf[MEMIF_SOCKET_UN_SIZE];
> +	struct sockaddr_un un;
>  	int sockfd;
>  	int ret;
>  	int on = 1;
> @@ -881,7 +876,7 @@ memif_socket_create(struct pmd_internals *pmd,
>  	}
>  
>  	sock->listener = listener;
> -	strlcpy(sock->filename, key, MEMIF_SOCKET_KEY_LEN);
> +	strlcpy(sock->filename, key, MEMIF_SOCKET_UN_SIZE);
>  	TAILQ_INIT(&sock->dev_queue);
>  
>  	if (listener != 0) {
> @@ -889,18 +884,18 @@ memif_socket_create(struct pmd_internals *pmd,
>  		if (sockfd < 0)
>  			goto error;
>  
> -		memset(un_buf, 0, sizeof(un_buf));
> -		un = (struct sockaddr_un *)un_buf;
> -		un->sun_family = AF_UNIX;
> -		strlcpy(un->sun_path, sock->filename, MEMIF_SOCKET_KEY_LEN);
> +		un.sun_family = AF_UNIX;
> +		strlcpy(un.sun_path, sock->filename, MEMIF_SOCKET_UN_SIZE);


Why does this not use abstract unix domain socket naming?
That would be much less error prone, because then the socket would disappear
when all applications using it are closed.