Re: [nft PATCH] monitor: Print NEWGEN events

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Jul 19, 2017 at 04:32:57PM +0200, Phil Sutter wrote:
> Now that they contain process information, they're actually interesting.
> For backwards compatibility, print process information only if it was
> present in the message.

Wait, a couple of comments.

[...]
> diff --git a/src/netlink.c b/src/netlink.c
> index e3c90dac8c7a6..44a9806097d00 100644
> --- a/src/netlink.c
> +++ b/src/netlink.c
> @@ -2915,6 +2915,43 @@ static void netlink_events_debug(uint16_t type)
>  #endif /* DEBUG */
>  }
>  
> +static int netlink_events_newgen_cb(const struct nlmsghdr *nlh, int type,
> +				    struct netlink_mon_handler *monh)
> +{
> +	const struct nlattr *attr;
> +	char name[256] = "";
> +	int genid, pid = -1;
> +
> +	mnl_attr_for_each(attr, nlh, sizeof(struct nfgenmsg)) {
> +		switch (mnl_attr_get_type(attr)) {
> +		case NFTA_GEN_ID:
> +			if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
> +				break;

I think it's better to hit netlink_abi_error() error. If validation
fails, it means someone in the kernel has accidentally changed the
size/type of this attribute. This it's basically breaking the ABI.
Better bail out here so people come to us so we quickly fix this.

> +			genid = ntohl(mnl_attr_get_u32(attr));
> +			break;
> +		case NFTA_GEN_PROC_NAME:
> +			if (mnl_attr_validate(attr, MNL_TYPE_NUL_STRING) < 0)
> +				break;
> +			strncpy(name, mnl_attr_get_str(attr), sizeof(name));

What is maximum process name length? If we hit this bound, we have to
make sure this does:

                        name[X - 1] = '\0';

Where X is the name buffer size.

> +			break;
> +		case NFTA_GEN_PROC_ID:
> +			if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
> +				break;
> +			pid = ntohl(mnl_attr_get_u32(attr));
> +			break;
> +		}
> +	}
> +	printf("new generation %d", genid);

Please, prepend '#' to this message.

> +	if (pid >= 0) {
> +		printf(" by process %d", pid);
> +		if (!monh->ctx->octx->numeric)
> +			printf(" (%s)", name);
> +	}
> +	printf("\n");
> +
> +	return MNL_CB_OK;
> +}

Thanks!