From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [RFC PATCH] use strlcpy for string copies
Date: Fri, 23 Feb 2018 18:18:09 +0100 [thread overview]
Message-ID: <20180223171809.GM4256@6wind.com> (raw)
In-Reply-To: <20180220170727.220340-1-bruce.richardson@intel.com>
On Tue, Feb 20, 2018 at 05:07:27PM +0000, Bruce Richardson wrote:
> Following on from the number of patches needing to be done for strncpy
> issues highlighted by coverity...
>
> The strncpy function is error prone for doing "safe" string copies, so
> we generally try to use "snprintf" instead in the code. The function
> "strlcpy" is a better alternative, though, since it better conveys the
> intention of the programmer, and doesn't suffer from the non-null
> terminating behaviour of it's n'ed brethern.
>
> The downside of this function is that it is not available by default
> on linux, though standard in the BSD's. It is available on most
> distros by installing "libbsd" package.
>
> This RFC therefore provides the following in rte_string_fns.h to ensure
> that strlcpy is available there:
> * for BSD, include string.h as normal
> * if RTE_USE_LIBBSD is set, include <bsd/string.h>
> * if not set, fallback to snprintf for strlcpy
>
> Using make build system, the RTE_USE_LIBBSD is a hard-coded value to "n",
> but when using meson, it's automatically set based on what is available
> on the platform.
>
> Instances of snprintf using "%s" alone as a string format are replaced
> via coccinelle script with the new strlcpy function. Instances of
> strncpy should be replaced too, but requires manual checking as to
> whether the NULL termination is manually done afterward or not.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
OK with the RFC, a few comments below regarding mlx4, mlx5 and the
definition itself though.
<snip>
> diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c
> index 3bc692731..c92dd6d43 100644
> --- a/drivers/net/mlx4/mlx4_ethdev.c
> +++ b/drivers/net/mlx4/mlx4_ethdev.c
> @@ -120,7 +120,7 @@ mlx4_get_ifname(const struct priv *priv, char (*ifname)[IF_NAMESIZE])
> goto try_dev_id;
> dev_port_prev = dev_port;
> if (dev_port == (priv->port - 1u))
> - snprintf(match, sizeof(match), "%s", name);
> + strlcpy(match, name, sizeof(match));
> }
> closedir(dir);
> if (match[0] == '\0') {
Missing #include <rte_string_fns.h>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 666507691..894a045ec 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -163,7 +163,7 @@ priv_get_ifname(const struct priv *priv, char (*ifname)[IF_NAMESIZE])
> goto try_dev_id;
> dev_port_prev = dev_port;
> if (dev_port == (priv->port - 1u))
> - snprintf(match, sizeof(match), "%s", name);
> + strlcpy(match, name, sizeof(match));
> }
> closedir(dir);
> if (match[0] == '\0')
Ditto (note I didn't check missing occurrences in other components).
<snip>
> diff --git a/lib/librte_eal/common/include/rte_string_fns.h b/lib/librte_eal/common/include/rte_string_fns.h
> index e97047a47..ff4c98b2a 100644
> --- a/lib/librte_eal/common/include/rte_string_fns.h
> +++ b/lib/librte_eal/common/include/rte_string_fns.h
> @@ -45,6 +45,20 @@ int
> rte_strsplit(char *string, int stringlen,
> char **tokens, int maxtokens, char delim);
>
> +/* pull in a strlcpy function */
> +#ifdef RTE_EXEC_ENV_BSDAPP
> +#include <string.h>
> +
> +#else /* non-BSD platforms */
> +#ifdef RTE_USE_LIBBSD
> +#include <bsd/string.h>
> +
> +#else /* no BSD header files, create own */
> +#define strlcpy(dst, src, size) snprintf(dst, size, "%s", src)
Missing #include <stdio.h> for that.
What also bothers me is that on some platforms, applications get a true
function definition and a macro on others. I suggest a static inline
or even a proper versioned definition in eal_common_string_fns.c instead.
--
Adrien Mazarguil
6WIND
next prev parent reply other threads:[~2018-02-23 17:18 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-20 17:07 Bruce Richardson
2018-02-20 23:01 ` Stephen Hemminger
2018-02-23 17:18 ` Adrien Mazarguil [this message]
2018-02-23 18:11 ` Matteo Croce
2018-03-12 11:32 ` [dpdk-dev] [PATCH 1/2] add support for strlcpy function Bruce Richardson
2018-03-12 11:33 ` [dpdk-dev] [PATCH 2/2] convert snprintf to strlcpy Bruce Richardson
2018-03-12 11:50 ` Bruce Richardson
2018-04-04 13:26 ` Thomas Monjalon
2018-03-12 11:51 ` [dpdk-dev] [PATCH 1/2] add support for strlcpy function Bruce Richardson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180223171809.GM4256@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).