[PATCH v3 3/5] socketutils: build a xlat table dynamically for decoding "family" field of Netlink GENERIC protocol

[Dmitry V. Levin]

On Tue, Jun 13, 2017 at 05:26:43PM +0900, Masatake YAMATO wrote:
> * configure.ac: Verify existence of linux/genetlink.h.
> 
> * socketutils.c (genl_families_xlat): exported function for building
> the xlat table. Include linux/genetlink.h.
> 
> * defs.h (genl_families_xlat): add the declaration.
> (genl_send_dump_families, genl_parse_families_response):
> helper functions.
> 
> Changes in v3 (all suggested by ldv):
> 
> 	* Check whether linux/genetlink.h is available or not.
> 
> 	* Don't add NLM_F_ACK to nlmsg_type when dumping genl families.
> 
> 	* Use xstrndup to extract family name from netlink data.
> 	  xstrndup can limits the length of data copied and puts
> 	  nul char at the end of buffer.
> 
> 	* Free the buffer for storing family name before overwriting it.
> 
> 	* Remove id_set local variable. id is now a pointer. So NULL
> 	  check of id can be used to know whether the value is assigned or not.
> 
> 	* Free the buffer for storing family name even if it is not stored
> 	  to dyxlat.
> 
> Signed-off-by: Masatake YAMATO <yamato at redhat.com>
> ---
>  configure.ac  |   1 +
>  defs.h        |   2 ++
>  socketutils.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 113 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index dc49d39..0974743 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -366,6 +366,7 @@ AC_CHECK_HEADERS(m4_normalize([
>  	linux/falloc.h
>  	linux/fiemap.h
>  	linux/filter.h
> +	linux/genetlink.h
>  	linux/hiddev.h
>  	linux/ip_vs.h
>  	linux/ipc.h
> diff --git a/defs.h b/defs.h
> index 063394e..f50b474 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -511,6 +511,8 @@ void dyxlat_free(struct dyxlat *dyxlat);
>  struct xlat *dyxlat_get(struct dyxlat *dyxlat);
>  void dyxlat_add_pair(struct dyxlat *dyxlat, uint64_t val, const char *str);
>  
> +struct xlat *genl_families_xlat(void);

Let's change this prototype to
const struct xlat *genl_families_xlat(void);

> +
>  extern unsigned long get_pagesize(void);
>  extern int
>  string_to_uint_ex(const char *str, char **endptr,
> diff --git a/socketutils.c b/socketutils.c
> index 6486e27..bc18430 100644
> --- a/socketutils.c
> +++ b/socketutils.c
> @@ -37,6 +37,9 @@
>  #include <linux/unix_diag.h>
>  #include <linux/netlink_diag.h>
>  #include <linux/rtnetlink.h>
> +#if HAVE_LINUX_GENETLINK_H
> +#include <linux/genetlink.h>
> +#endif
>  
>  #include <sys/un.h>
>  #ifndef UNIX_PATH_MAX
> @@ -541,3 +544,110 @@ print_sockaddr_by_inode(struct tcb *const tcp, const int fd,
>  	return print_sockaddr_by_inode_cached(inode) ? true :
>  		print_sockaddr_by_inode_uncached(inode, getfdproto(tcp, fd));
>  }
> +
> +#ifdef HAVE_LINUX_GENETLINK_H
> +/*
> + * Managing the cache for decoding communications of Netlink GENERIC protocol
> + *
> + * As name shown Netlink GENERIC protocol is generic protocol. The
> + * numbers of msg types used in the protocol are not defined
> + * statically. Kernel defines them on demand.  So the xlat converted
> + * from header files doesn't help for decoding the protocol. Following
> + * codes are building xlat(dyxlat) at runtime.
> + */
> +static bool
> +genl_send_dump_families(const int fd)
> +{
> +	struct {
> +		const struct nlmsghdr nlh;
> +		struct genlmsghdr gnlh;
> +	} req = {
> +		.nlh = {
> +			.nlmsg_len = sizeof(req),
> +			.nlmsg_type = GENL_ID_CTRL,
> +			.nlmsg_flags = NLM_F_DUMP|NLM_F_REQUEST,
> +		},
> +		.gnlh = {
> +			.cmd = CTRL_CMD_GETFAMILY,
> +		}
> +	};
> +	return send_query(fd, &req, sizeof(req));
> +}
> +
> +static int
> +genl_parse_families_response(const void *const data,
> +			     const int data_len, const unsigned long inode,
> +			     void *user_data)
> +{
> +	struct dyxlat *dyxlat = user_data;
> +	const struct genlmsghdr * const gnlh = data;
> +	struct rtattr *attr;
> +	int rta_len = data_len - NLMSG_LENGTH(sizeof(*gnlh));
> +
> +	char *name = NULL;
> +	uint16_t *id = NULL;
> +
> +	if (rta_len < 0)
> +		return -1;
> +	if (gnlh->cmd != CTRL_CMD_NEWFAMILY)
> +		return -1;
> +	if (gnlh->version != 2)
> +		return -1;
> +
> +	for (attr = (struct rtattr *) (gnlh + 1);
> +	     RTA_OK(attr, rta_len);
> +	     attr = RTA_NEXT(attr, rta_len)) {
> +		switch (attr->rta_type) {
> +		case CTRL_ATTR_FAMILY_NAME:
> +			if (name)
> +				free(name);

What if we used the first occurrence instead?

> +			name = xstrndup(RTA_DATA(attr), RTA_PAYLOAD(attr));

What if we just saved the pointer?  I mean something like this:

char *name = NULL;
unsigned int name_len = 0;
...
case CTRL_ATTR_FAMILY_NAME:
	if (!name) {
		name = RTA_DATA(attr);
		name_len = RTA_PAYLOAD(attr);
	}
...
if (name && name_len && id)
	dyxlat_add_pair(dyxlat, *id, name, name_len);

> +			break;
> +		case CTRL_ATTR_FAMILY_ID:
> +			if (RTA_PAYLOAD(attr) >= 2)

Wouldn't it be better to use a strict check, e.g.
if (!id && RTA_PAYLOAD(attr) == sizeof(*id))


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170614/78e16352/attachment.bin>