* [dpdk-dev] [RFC PATCH] use strlcpy for string copies
@ 2018-02-20 17:07 Bruce Richardson
2018-02-20 23:01 ` Stephen Hemminger
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Bruce Richardson @ 2018-02-20 17:07 UTC (permalink / raw)
To: dev; +Cc: Bruce Richardson
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>
---
app/pdump/main.c | 11 +++++------
app/test-pmd/parameters.c | 5 ++---
config/common_base | 1 +
config/meson.build | 7 +++++++
devtools/cocci/strlcpy.cocci | 8 ++++++++
drivers/net/bonding/rte_eth_bond_pmd.c | 2 +-
drivers/net/failsafe/failsafe_args.c | 5 +++--
drivers/net/mlx4/mlx4_ethdev.c | 2 +-
drivers/net/mlx5/mlx5_ethdev.c | 2 +-
drivers/net/mrvl/mrvl_qos.c | 2 +-
drivers/net/tap/rte_eth_tap.c | 5 +++--
.../ip_pipeline/pipeline/pipeline_flow_classification_be.c | 4 ++--
examples/ip_pipeline/pipeline/pipeline_passthrough_be.c | 4 ++--
examples/load_balancer/config.c | 2 +-
lib/librte_cmdline/cmdline_parse.c | 6 +++---
lib/librte_cmdline/cmdline_parse_etheraddr.c | 2 +-
lib/librte_cmdline/cmdline_parse_ipaddr.c | 2 +-
lib/librte_cmdline/cmdline_parse_portlist.c | 2 +-
lib/librte_cmdline/cmdline_parse_string.c | 4 ++--
lib/librte_eal/common/eal_common_bus.c | 3 ++-
lib/librte_eal/common/include/rte_string_fns.h | 14 ++++++++++++++
lib/librte_pdump/rte_pdump.c | 9 +++++----
test/test/test_cmdline_cirbuf.c | 2 +-
test/test/test_eal_flags.c | 10 ++++++----
test/test/test_malloc.c | 2 +-
25 files changed, 75 insertions(+), 41 deletions(-)
create mode 100644 devtools/cocci/strlcpy.cocci
diff --git a/app/pdump/main.c b/app/pdump/main.c
index f6865bdbd..d29de0321 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -24,6 +24,7 @@
#include <rte_kvargs.h>
#include <rte_mempool.h>
#include <rte_ring.h>
+#include <rte_string_fns.h>
#include <rte_pdump.h>
#define CMD_LINE_OPT_PDUMP "pdump"
@@ -408,17 +409,15 @@ launch_args_parse(int argc, char **argv, char *prgname)
if (!strncmp(long_option[option_index].name,
CMD_LINE_OPT_SER_SOCK_PATH,
sizeof(CMD_LINE_OPT_SER_SOCK_PATH))) {
- snprintf(server_socket_path,
- sizeof(server_socket_path), "%s",
- optarg);
+ strlcpy(server_socket_path, optarg,
+ sizeof(server_socket_path));
}
if (!strncmp(long_option[option_index].name,
CMD_LINE_OPT_CLI_SOCK_PATH,
sizeof(CMD_LINE_OPT_CLI_SOCK_PATH))) {
- snprintf(client_socket_path,
- sizeof(client_socket_path), "%s",
- optarg);
+ strlcpy(client_socket_path, optarg,
+ sizeof(client_socket_path));
}
break;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 97d22b860..2192bdcdf 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -658,9 +658,8 @@ launch_args_parse(int argc, char** argv)
if (!strcmp(lgopts[opt_idx].name, "cmdline-file")) {
printf("CLI commands to be read from %s\n",
optarg);
- snprintf(cmdline_filename,
- sizeof(cmdline_filename), "%s",
- optarg);
+ strlcpy(cmdline_filename, optarg,
+ sizeof(cmdline_filename));
}
if (!strcmp(lgopts[opt_idx].name, "auto-start")) {
printf("Auto-start selected\n");
diff --git a/config/common_base b/config/common_base
index ad03cf433..20d4cbcf2 100644
--- a/config/common_base
+++ b/config/common_base
@@ -76,6 +76,7 @@ CONFIG_RTE_EAL_VFIO=n
CONFIG_RTE_MAX_VFIO_GROUPS=64
CONFIG_RTE_MALLOC_DEBUG=n
CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n
+CONFIG_RTE_USE_LIBBSD=n
#
# Recognize/ignore the AVX/AVX512 CPU flags for performance/power testing.
diff --git a/config/meson.build b/config/meson.build
index f8c67578d..77af5d897 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -38,6 +38,13 @@ if numa_dep.found() and cc.has_header('numaif.h')
dpdk_extra_ldflags += '-lnuma'
endif
+# check for strlcpy
+if host_machine.system() == 'linux' and cc.find_library('bsd', required: false).found()
+ dpdk_conf.set('RTE_USE_LIBBSD', 1)
+ add_project_link_arguments('-lbsd', language: 'c')
+ dpdk_extra_ldflags += '-lbsd'
+endif
+
# add -include rte_config to cflags
add_project_arguments('-include', 'rte_config.h', language: 'c')
diff --git a/devtools/cocci/strlcpy.cocci b/devtools/cocci/strlcpy.cocci
new file mode 100644
index 000000000..335e27128
--- /dev/null
+++ b/devtools/cocci/strlcpy.cocci
@@ -0,0 +1,8 @@
+@use_strlcpy@
+identifier src, dst;
+expression size;
+@@
+(
+- snprintf(dst, size, "%s", src)
++ strlcpy(dst, src, size)
+)
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index c34c3251f..f958ece72 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -617,7 +617,7 @@ mode6_debug(const char __attribute__((unused)) *info, struct ether_hdr *eth_h,
uint16_t offset = get_vlan_offset(eth_h, ðer_type);
#ifdef RTE_LIBRTE_BOND_DEBUG_ALB
- snprintf(buf, 16, "%s", info);
+ strlcpy(buf, info, 16);
#endif
if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4)) {
diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
index 366dbea16..ea934f92a 100644
--- a/drivers/net/failsafe/failsafe_args.c
+++ b/drivers/net/failsafe/failsafe_args.c
@@ -14,6 +14,7 @@
#include <rte_devargs.h>
#include <rte_malloc.h>
#include <rte_kvargs.h>
+#include <rte_string_fns.h>
#include "failsafe_private.h"
@@ -340,7 +341,7 @@ fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])
a = b + 1;
}
out:
- snprintf(params, DEVARGS_MAXLEN, "%s", buffer);
+ strlcpy(params, buffer, DEVARGS_MAXLEN);
return 0;
}
@@ -392,7 +393,7 @@ failsafe_args_parse(struct rte_eth_dev *dev, const char *params)
ret = 0;
priv->subs_tx = FAILSAFE_MAX_ETHPORTS;
/* default parameters */
- n = snprintf(mut_params, sizeof(mut_params), "%s", params);
+ n = strlcpy(mut_params, params, sizeof(mut_params));
if (n >= sizeof(mut_params)) {
ERROR("Parameter string too long (>=%zu)",
sizeof(mut_params));
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') {
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')
diff --git a/drivers/net/mrvl/mrvl_qos.c b/drivers/net/mrvl/mrvl_qos.c
index fbb368131..edc4d92d6 100644
--- a/drivers/net/mrvl/mrvl_qos.c
+++ b/drivers/net/mrvl/mrvl_qos.c
@@ -190,7 +190,7 @@ get_entry_values(const char *entry, uint8_t *tab,
return -1;
/* Copy the entry to safely use rte_strsplit(). */
- snprintf(entry_cpy, RTE_DIM(entry_cpy), "%s", entry);
+ strlcpy(entry_cpy, entry, RTE_DIM(entry_cpy));
/*
* If there are more tokens than array size, rte_strsplit will
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f09db0ea9..fbba9aa63 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -15,6 +15,7 @@
#include <rte_net.h>
#include <rte_debug.h>
#include <rte_ip.h>
+#include <rte_string_fns.h>
#include <sys/types.h>
#include <sys/stat.h>
@@ -1548,7 +1549,7 @@ set_interface_name(const char *key __rte_unused,
char *name = (char *)extra_args;
if (value)
- snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s", value);
+ strlcpy(name, value, RTE_ETH_NAME_MAX_LEN - 1);
else
snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s%d",
DEFAULT_TAP_NAME, (tap_unit - 1));
@@ -1564,7 +1565,7 @@ set_remote_iface(const char *key __rte_unused,
char *name = (char *)extra_args;
if (value)
- snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s", value);
+ strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
return 0;
}
diff --git a/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c b/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c
index 097ec3469..3e26ae86b 100644
--- a/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c
+++ b/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c
@@ -8,6 +8,7 @@
#include <rte_malloc.h>
#include <rte_table_hash.h>
#include <rte_byteorder.h>
+#include <rte_string_fns.h>
#include <pipeline.h>
#include "pipeline_flow_classification_be.h"
@@ -280,8 +281,7 @@ pipeline_fc_parse_args(struct pipeline_flow_classification *p,
"\"%s\" is too long", params->name,
arg_name);
- snprintf(key_mask_str, mask_str_len + 1, "%s",
- arg_value);
+ strlcpy(key_mask_str, arg_value, mask_str_len + 1);
continue;
}
diff --git a/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c b/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c
index b2bbaedda..1c44e75df 100644
--- a/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c
+++ b/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c
@@ -8,6 +8,7 @@
#include <rte_common.h>
#include <rte_malloc.h>
#include <rte_byteorder.h>
+#include <rte_string_fns.h>
#include <rte_table_stub.h>
#include <rte_table_hash.h>
#include <rte_pipeline.h>
@@ -524,8 +525,7 @@ pipeline_passthrough_parse_args(struct pipeline_passthrough_params *p,
"\"%s\" too long", params->name,
arg_name);
- snprintf(dma_mask_str, mask_str_len + 1,
- "%s", arg_value);
+ strlcpy(dma_mask_str, arg_value, mask_str_len + 1);
p->dma_enabled = 1;
diff --git a/examples/load_balancer/config.c b/examples/load_balancer/config.c
index b5b66368d..972c85c5b 100644
--- a/examples/load_balancer/config.c
+++ b/examples/load_balancer/config.c
@@ -121,7 +121,7 @@ str_to_unsigned_array(
int i, num_splits = 0;
/* copy s so we don't modify original string */
- snprintf(str, sizeof(str), "%s", s);
+ strlcpy(str, s, sizeof(str));
num_splits = rte_strsplit(str, sizeof(str), splits, num_vals, separator);
errno = 0;
diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index e88e4e110..961f9befd 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -251,7 +251,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
}
#ifdef RTE_LIBRTE_CMDLINE_DEBUG
- snprintf(debug_buf, (linelen>64 ? 64 : linelen), "%s", buf);
+ strlcpy(debug_buf, buf, (linelen > 64 ? 64 : linelen));
debug_printf("Parse line : len=%d, <%s>\n", linelen, debug_buf);
#endif
@@ -436,7 +436,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
if ((unsigned)(comp_len + 1) > size)
return 0;
- snprintf(dst, size, "%s", comp_buf);
+ strlcpy(dst, comp_buf, size);
dst[comp_len] = 0;
return 2;
}
@@ -513,7 +513,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
continue;
}
(*state)++;
- l=snprintf(dst, size, "%s", tmpbuf);
+ l=strlcpy(dst, tmpbuf, size);
if (l>=0 && token_hdr.ops->get_help) {
token_hdr.ops->get_help(token_p, tmpbuf,
sizeof(tmpbuf));
diff --git a/lib/librte_cmdline/cmdline_parse_etheraddr.c b/lib/librte_cmdline/cmdline_parse_etheraddr.c
index 8d2811926..24e04755c 100644
--- a/lib/librte_cmdline/cmdline_parse_etheraddr.c
+++ b/lib/librte_cmdline/cmdline_parse_etheraddr.c
@@ -102,7 +102,7 @@ cmdline_parse_etheraddr(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
(token_len != ETHER_ADDRSTRLENSHORT - 1))
return -1;
- snprintf(ether_str, token_len+1, "%s", buf);
+ strlcpy(ether_str, buf, token_len + 1);
tmp = my_ether_aton(ether_str);
if (tmp == NULL)
diff --git a/lib/librte_cmdline/cmdline_parse_ipaddr.c b/lib/librte_cmdline/cmdline_parse_ipaddr.c
index ae6ea1007..d34436abc 100644
--- a/lib/librte_cmdline/cmdline_parse_ipaddr.c
+++ b/lib/librte_cmdline/cmdline_parse_ipaddr.c
@@ -277,7 +277,7 @@ cmdline_parse_ipaddr(cmdline_parse_token_hdr_t *tk, const char *buf, void *res,
if (token_len >= INET6_ADDRSTRLEN+4)
return -1;
- snprintf(ip_str, token_len+1, "%s", buf);
+ strlcpy(ip_str, buf, token_len + 1);
/* convert the network prefix */
if (tk2->ipaddr_data.flags & CMDLINE_IPADDR_NETWORK) {
diff --git a/lib/librte_cmdline/cmdline_parse_portlist.c b/lib/librte_cmdline/cmdline_parse_portlist.c
index 5952f3438..ad43b522e 100644
--- a/lib/librte_cmdline/cmdline_parse_portlist.c
+++ b/lib/librte_cmdline/cmdline_parse_portlist.c
@@ -94,7 +94,7 @@ cmdline_parse_portlist(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
if (token_len >= PORTLIST_TOKEN_SIZE)
return -1;
- snprintf(portlist_str, token_len+1, "%s", buf);
+ strlcpy(portlist_str, buf, token_len + 1);
if (pl) {
pl->map = 0;
diff --git a/lib/librte_cmdline/cmdline_parse_string.c b/lib/librte_cmdline/cmdline_parse_string.c
index abde0412a..9cf41d0f7 100644
--- a/lib/librte_cmdline/cmdline_parse_string.c
+++ b/lib/librte_cmdline/cmdline_parse_string.c
@@ -125,10 +125,10 @@ cmdline_parse_string(cmdline_parse_token_hdr_t *tk, const char *buf, void *res,
if (res) {
if ((sd->str != NULL) && (strcmp(sd->str, TOKEN_STRING_MULTI) == 0))
/* we are sure that token_len is < STR_MULTI_TOKEN_SIZE-1 */
- snprintf(res, STR_MULTI_TOKEN_SIZE, "%s", buf);
+ strlcpy(res, buf, STR_MULTI_TOKEN_SIZE);
else
/* we are sure that token_len is < STR_TOKEN_SIZE-1 */
- snprintf(res, STR_TOKEN_SIZE, "%s", buf);
+ strlcpy(res, buf, STR_TOKEN_SIZE);
*((char *)res + token_len) = 0;
}
diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
index 3e022d510..0943851cc 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -36,6 +36,7 @@
#include <rte_bus.h>
#include <rte_debug.h>
+#include <rte_string_fns.h>
#include "eal_private.h"
@@ -212,7 +213,7 @@ rte_bus_find_by_device_name(const char *str)
char name[RTE_DEV_NAME_MAX_LEN];
char *c;
- snprintf(name, sizeof(name), "%s", str);
+ strlcpy(name, str, sizeof(name));
c = strchr(name, ',');
if (c != NULL)
c[0] = '\0';
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)
+
+#endif /* RTE_USE_LIBBSD */
+#endif /* BSDAPP */
+
#ifdef __cplusplus
}
#endif
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index ec8a5d84c..41a9deeba 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -17,6 +17,7 @@
#include <rte_lcore.h>
#include <rte_log.h>
#include <rte_errno.h>
+#include <rte_string_fns.h>
#include "rte_pdump.h"
@@ -401,9 +402,9 @@ pdump_get_socket_path(char *buffer, int bufsz, enum rte_pdump_socktype type)
int ret = 0;
if (type == RTE_PDUMP_SOCKET_SERVER && server_socket_dir[0] != 0)
- snprintf(dir, sizeof(dir), "%s", server_socket_dir);
+ strlcpy(dir, server_socket_dir, sizeof(dir));
else if (type == RTE_PDUMP_SOCKET_CLIENT && client_socket_dir[0] != 0)
- snprintf(dir, sizeof(dir), "%s", client_socket_dir);
+ strlcpy(dir, client_socket_dir, sizeof(dir));
else {
if (getuid() != 0) {
dir_home = getenv(SOCKET_PATH_HOME);
@@ -891,10 +892,10 @@ rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type)
if (path != NULL) {
if (type == RTE_PDUMP_SOCKET_SERVER) {
count = sizeof(server_socket_dir);
- ret = snprintf(server_socket_dir, count, "%s", path);
+ ret = strlcpy(server_socket_dir, path, count);
} else {
count = sizeof(client_socket_dir);
- ret = snprintf(client_socket_dir, count, "%s", path);
+ ret = strlcpy(client_socket_dir, path, count);
}
if (ret < 0 || ret >= count) {
diff --git a/test/test/test_cmdline_cirbuf.c b/test/test/test_cmdline_cirbuf.c
index e9193f66c..8ac326cb0 100644
--- a/test/test/test_cmdline_cirbuf.c
+++ b/test/test/test_cmdline_cirbuf.c
@@ -483,7 +483,7 @@ test_cirbuf_string_get_del_partial(void)
memset(tmp, 0, sizeof(tmp));
memset(tmp2, 0, sizeof(tmp));
- snprintf(tmp2, sizeof(tmp2), "%s", CIRBUF_STR_HEAD);
+ strlcpy(tmp2, CIRBUF_STR_HEAD, sizeof(tmp2));
/*
* initialize circular buffer
diff --git a/test/test/test_eal_flags.c b/test/test/test_eal_flags.c
index 37c42efe8..93eb7a481 100644
--- a/test/test/test_eal_flags.c
+++ b/test/test/test_eal_flags.c
@@ -1151,11 +1151,12 @@ test_memory_flags(void)
/* add one extra socket */
for (i = 0; i < num_sockets + 1; i++) {
snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem, DEFAULT_MEM_SIZE);
- snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "%s", buf);
+ strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
if (num_sockets + 1 - i > 1) {
snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
- snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "%s", buf);
+ strlcpy(invalid_socket_mem, buf,
+ sizeof(invalid_socket_mem));
}
}
@@ -1167,11 +1168,12 @@ test_memory_flags(void)
/* add one extra socket */
for (i = 0; i < num_sockets; i++) {
snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem, DEFAULT_MEM_SIZE);
- snprintf(valid_socket_mem, sizeof(valid_socket_mem), "%s", buf);
+ strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
if (num_sockets - i > 1) {
snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
- snprintf(valid_socket_mem, sizeof(valid_socket_mem), "%s", buf);
+ strlcpy(valid_socket_mem, buf,
+ sizeof(valid_socket_mem));
}
}
diff --git a/test/test/test_malloc.c b/test/test/test_malloc.c
index d23192cf1..ccc5feaec 100644
--- a/test/test/test_malloc.c
+++ b/test/test/test_malloc.c
@@ -378,7 +378,7 @@ test_realloc(void)
printf("NULL pointer returned from rte_zmalloc\n");
return -1;
}
- snprintf(ptr1, size1, "%s" ,hello_str);
+ strlcpy(ptr1, hello_str, size1);
char *ptr2 = rte_realloc(ptr1, size2, RTE_CACHE_LINE_SIZE);
if (!ptr2){
rte_free(ptr1);
--
2.14.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [RFC PATCH] use strlcpy for string copies
2018-02-20 17:07 [dpdk-dev] [RFC PATCH] use strlcpy for string copies Bruce Richardson
@ 2018-02-20 23:01 ` Stephen Hemminger
2018-02-23 17:18 ` Adrien Mazarguil
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2018-02-20 23:01 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
On Tue, 20 Feb 2018 17:07:27 +0000
Bruce Richardson <bruce.richardson@intel.com> 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>
Looks good, especially not reinventing strlcpy and using libbsd.
Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [RFC PATCH] use strlcpy for string copies
2018-02-20 17:07 [dpdk-dev] [RFC PATCH] use strlcpy for string copies Bruce Richardson
2018-02-20 23:01 ` Stephen Hemminger
@ 2018-02-23 17:18 ` Adrien Mazarguil
2018-02-23 18:11 ` Matteo Croce
2018-03-12 11:32 ` [dpdk-dev] [PATCH 1/2] add support for strlcpy function Bruce Richardson
3 siblings, 0 replies; 9+ messages in thread
From: Adrien Mazarguil @ 2018-02-23 17:18 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [RFC PATCH] use strlcpy for string copies
2018-02-20 17:07 [dpdk-dev] [RFC PATCH] use strlcpy for string copies Bruce Richardson
2018-02-20 23:01 ` Stephen Hemminger
2018-02-23 17:18 ` Adrien Mazarguil
@ 2018-02-23 18:11 ` Matteo Croce
2018-03-12 11:32 ` [dpdk-dev] [PATCH 1/2] add support for strlcpy function Bruce Richardson
3 siblings, 0 replies; 9+ messages in thread
From: Matteo Croce @ 2018-02-23 18:11 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
On Tue, Feb 20, 2018 at 6:07 PM, Bruce Richardson
<bruce.richardson@intel.com> 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>
> ---
> app/pdump/main.c | 11 +++++------
> app/test-pmd/parameters.c | 5 ++---
> config/common_base | 1 +
> config/meson.build | 7 +++++++
> devtools/cocci/strlcpy.cocci | 8 ++++++++
> drivers/net/bonding/rte_eth_bond_pmd.c | 2 +-
> drivers/net/failsafe/failsafe_args.c | 5 +++--
> drivers/net/mlx4/mlx4_ethdev.c | 2 +-
> drivers/net/mlx5/mlx5_ethdev.c | 2 +-
> drivers/net/mrvl/mrvl_qos.c | 2 +-
> drivers/net/tap/rte_eth_tap.c | 5 +++--
> .../ip_pipeline/pipeline/pipeline_flow_classification_be.c | 4 ++--
> examples/ip_pipeline/pipeline/pipeline_passthrough_be.c | 4 ++--
> examples/load_balancer/config.c | 2 +-
> lib/librte_cmdline/cmdline_parse.c | 6 +++---
> lib/librte_cmdline/cmdline_parse_etheraddr.c | 2 +-
> lib/librte_cmdline/cmdline_parse_ipaddr.c | 2 +-
> lib/librte_cmdline/cmdline_parse_portlist.c | 2 +-
> lib/librte_cmdline/cmdline_parse_string.c | 4 ++--
> lib/librte_eal/common/eal_common_bus.c | 3 ++-
> lib/librte_eal/common/include/rte_string_fns.h | 14 ++++++++++++++
> lib/librte_pdump/rte_pdump.c | 9 +++++----
> test/test/test_cmdline_cirbuf.c | 2 +-
> test/test/test_eal_flags.c | 10 ++++++----
> test/test/test_malloc.c | 2 +-
> 25 files changed, 75 insertions(+), 41 deletions(-)
> create mode 100644 devtools/cocci/strlcpy.cocci
>
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index f6865bdbd..d29de0321 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -24,6 +24,7 @@
> #include <rte_kvargs.h>
> #include <rte_mempool.h>
> #include <rte_ring.h>
> +#include <rte_string_fns.h>
> #include <rte_pdump.h>
>
> #define CMD_LINE_OPT_PDUMP "pdump"
> @@ -408,17 +409,15 @@ launch_args_parse(int argc, char **argv, char *prgname)
> if (!strncmp(long_option[option_index].name,
> CMD_LINE_OPT_SER_SOCK_PATH,
> sizeof(CMD_LINE_OPT_SER_SOCK_PATH))) {
> - snprintf(server_socket_path,
> - sizeof(server_socket_path), "%s",
> - optarg);
> + strlcpy(server_socket_path, optarg,
> + sizeof(server_socket_path));
> }
>
> if (!strncmp(long_option[option_index].name,
> CMD_LINE_OPT_CLI_SOCK_PATH,
> sizeof(CMD_LINE_OPT_CLI_SOCK_PATH))) {
> - snprintf(client_socket_path,
> - sizeof(client_socket_path), "%s",
> - optarg);
> + strlcpy(client_socket_path, optarg,
> + sizeof(client_socket_path));
> }
>
> break;
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index 97d22b860..2192bdcdf 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -658,9 +658,8 @@ launch_args_parse(int argc, char** argv)
> if (!strcmp(lgopts[opt_idx].name, "cmdline-file")) {
> printf("CLI commands to be read from %s\n",
> optarg);
> - snprintf(cmdline_filename,
> - sizeof(cmdline_filename), "%s",
> - optarg);
> + strlcpy(cmdline_filename, optarg,
> + sizeof(cmdline_filename));
> }
> if (!strcmp(lgopts[opt_idx].name, "auto-start")) {
> printf("Auto-start selected\n");
> diff --git a/config/common_base b/config/common_base
> index ad03cf433..20d4cbcf2 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -76,6 +76,7 @@ CONFIG_RTE_EAL_VFIO=n
> CONFIG_RTE_MAX_VFIO_GROUPS=64
> CONFIG_RTE_MALLOC_DEBUG=n
> CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n
> +CONFIG_RTE_USE_LIBBSD=n
>
> #
> # Recognize/ignore the AVX/AVX512 CPU flags for performance/power testing.
> diff --git a/config/meson.build b/config/meson.build
> index f8c67578d..77af5d897 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -38,6 +38,13 @@ if numa_dep.found() and cc.has_header('numaif.h')
> dpdk_extra_ldflags += '-lnuma'
> endif
>
> +# check for strlcpy
> +if host_machine.system() == 'linux' and cc.find_library('bsd', required: false).found()
> + dpdk_conf.set('RTE_USE_LIBBSD', 1)
> + add_project_link_arguments('-lbsd', language: 'c')
> + dpdk_extra_ldflags += '-lbsd'
> +endif
> +
> # add -include rte_config to cflags
> add_project_arguments('-include', 'rte_config.h', language: 'c')
>
> diff --git a/devtools/cocci/strlcpy.cocci b/devtools/cocci/strlcpy.cocci
> new file mode 100644
> index 000000000..335e27128
> --- /dev/null
> +++ b/devtools/cocci/strlcpy.cocci
> @@ -0,0 +1,8 @@
> +@use_strlcpy@
> +identifier src, dst;
> +expression size;
> +@@
> +(
> +- snprintf(dst, size, "%s", src)
> ++ strlcpy(dst, src, size)
> +)
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index c34c3251f..f958ece72 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -617,7 +617,7 @@ mode6_debug(const char __attribute__((unused)) *info, struct ether_hdr *eth_h,
> uint16_t offset = get_vlan_offset(eth_h, ðer_type);
>
> #ifdef RTE_LIBRTE_BOND_DEBUG_ALB
> - snprintf(buf, 16, "%s", info);
> + strlcpy(buf, info, 16);
> #endif
>
> if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4)) {
> diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
> index 366dbea16..ea934f92a 100644
> --- a/drivers/net/failsafe/failsafe_args.c
> +++ b/drivers/net/failsafe/failsafe_args.c
> @@ -14,6 +14,7 @@
> #include <rte_devargs.h>
> #include <rte_malloc.h>
> #include <rte_kvargs.h>
> +#include <rte_string_fns.h>
>
> #include "failsafe_private.h"
>
> @@ -340,7 +341,7 @@ fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])
> a = b + 1;
> }
> out:
> - snprintf(params, DEVARGS_MAXLEN, "%s", buffer);
> + strlcpy(params, buffer, DEVARGS_MAXLEN);
> return 0;
> }
>
> @@ -392,7 +393,7 @@ failsafe_args_parse(struct rte_eth_dev *dev, const char *params)
> ret = 0;
> priv->subs_tx = FAILSAFE_MAX_ETHPORTS;
> /* default parameters */
> - n = snprintf(mut_params, sizeof(mut_params), "%s", params);
> + n = strlcpy(mut_params, params, sizeof(mut_params));
> if (n >= sizeof(mut_params)) {
> ERROR("Parameter string too long (>=%zu)",
> sizeof(mut_params));
> 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') {
> 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')
> diff --git a/drivers/net/mrvl/mrvl_qos.c b/drivers/net/mrvl/mrvl_qos.c
> index fbb368131..edc4d92d6 100644
> --- a/drivers/net/mrvl/mrvl_qos.c
> +++ b/drivers/net/mrvl/mrvl_qos.c
> @@ -190,7 +190,7 @@ get_entry_values(const char *entry, uint8_t *tab,
> return -1;
>
> /* Copy the entry to safely use rte_strsplit(). */
> - snprintf(entry_cpy, RTE_DIM(entry_cpy), "%s", entry);
> + strlcpy(entry_cpy, entry, RTE_DIM(entry_cpy));
>
> /*
> * If there are more tokens than array size, rte_strsplit will
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index f09db0ea9..fbba9aa63 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -15,6 +15,7 @@
> #include <rte_net.h>
> #include <rte_debug.h>
> #include <rte_ip.h>
> +#include <rte_string_fns.h>
>
> #include <sys/types.h>
> #include <sys/stat.h>
> @@ -1548,7 +1549,7 @@ set_interface_name(const char *key __rte_unused,
> char *name = (char *)extra_args;
>
> if (value)
> - snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s", value);
> + strlcpy(name, value, RTE_ETH_NAME_MAX_LEN - 1);
> else
> snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s%d",
> DEFAULT_TAP_NAME, (tap_unit - 1));
> @@ -1564,7 +1565,7 @@ set_remote_iface(const char *key __rte_unused,
> char *name = (char *)extra_args;
>
> if (value)
> - snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s", value);
> + strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
>
> return 0;
> }
> diff --git a/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c b/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c
> index 097ec3469..3e26ae86b 100644
> --- a/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c
> +++ b/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c
> @@ -8,6 +8,7 @@
> #include <rte_malloc.h>
> #include <rte_table_hash.h>
> #include <rte_byteorder.h>
> +#include <rte_string_fns.h>
> #include <pipeline.h>
>
> #include "pipeline_flow_classification_be.h"
> @@ -280,8 +281,7 @@ pipeline_fc_parse_args(struct pipeline_flow_classification *p,
> "\"%s\" is too long", params->name,
> arg_name);
>
> - snprintf(key_mask_str, mask_str_len + 1, "%s",
> - arg_value);
> + strlcpy(key_mask_str, arg_value, mask_str_len + 1);
>
> continue;
> }
> diff --git a/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c b/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c
> index b2bbaedda..1c44e75df 100644
> --- a/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c
> +++ b/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c
> @@ -8,6 +8,7 @@
> #include <rte_common.h>
> #include <rte_malloc.h>
> #include <rte_byteorder.h>
> +#include <rte_string_fns.h>
> #include <rte_table_stub.h>
> #include <rte_table_hash.h>
> #include <rte_pipeline.h>
> @@ -524,8 +525,7 @@ pipeline_passthrough_parse_args(struct pipeline_passthrough_params *p,
> "\"%s\" too long", params->name,
> arg_name);
>
> - snprintf(dma_mask_str, mask_str_len + 1,
> - "%s", arg_value);
> + strlcpy(dma_mask_str, arg_value, mask_str_len + 1);
>
> p->dma_enabled = 1;
>
> diff --git a/examples/load_balancer/config.c b/examples/load_balancer/config.c
> index b5b66368d..972c85c5b 100644
> --- a/examples/load_balancer/config.c
> +++ b/examples/load_balancer/config.c
> @@ -121,7 +121,7 @@ str_to_unsigned_array(
> int i, num_splits = 0;
>
> /* copy s so we don't modify original string */
> - snprintf(str, sizeof(str), "%s", s);
> + strlcpy(str, s, sizeof(str));
> num_splits = rte_strsplit(str, sizeof(str), splits, num_vals, separator);
>
> errno = 0;
> diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
> index e88e4e110..961f9befd 100644
> --- a/lib/librte_cmdline/cmdline_parse.c
> +++ b/lib/librte_cmdline/cmdline_parse.c
> @@ -251,7 +251,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
> }
>
> #ifdef RTE_LIBRTE_CMDLINE_DEBUG
> - snprintf(debug_buf, (linelen>64 ? 64 : linelen), "%s", buf);
> + strlcpy(debug_buf, buf, (linelen > 64 ? 64 : linelen));
> debug_printf("Parse line : len=%d, <%s>\n", linelen, debug_buf);
> #endif
>
> @@ -436,7 +436,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
> if ((unsigned)(comp_len + 1) > size)
> return 0;
>
> - snprintf(dst, size, "%s", comp_buf);
> + strlcpy(dst, comp_buf, size);
> dst[comp_len] = 0;
> return 2;
> }
> @@ -513,7 +513,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
> continue;
> }
> (*state)++;
> - l=snprintf(dst, size, "%s", tmpbuf);
> + l=strlcpy(dst, tmpbuf, size);
> if (l>=0 && token_hdr.ops->get_help) {
> token_hdr.ops->get_help(token_p, tmpbuf,
> sizeof(tmpbuf));
> diff --git a/lib/librte_cmdline/cmdline_parse_etheraddr.c b/lib/librte_cmdline/cmdline_parse_etheraddr.c
> index 8d2811926..24e04755c 100644
> --- a/lib/librte_cmdline/cmdline_parse_etheraddr.c
> +++ b/lib/librte_cmdline/cmdline_parse_etheraddr.c
> @@ -102,7 +102,7 @@ cmdline_parse_etheraddr(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
> (token_len != ETHER_ADDRSTRLENSHORT - 1))
> return -1;
>
> - snprintf(ether_str, token_len+1, "%s", buf);
> + strlcpy(ether_str, buf, token_len + 1);
>
> tmp = my_ether_aton(ether_str);
> if (tmp == NULL)
> diff --git a/lib/librte_cmdline/cmdline_parse_ipaddr.c b/lib/librte_cmdline/cmdline_parse_ipaddr.c
> index ae6ea1007..d34436abc 100644
> --- a/lib/librte_cmdline/cmdline_parse_ipaddr.c
> +++ b/lib/librte_cmdline/cmdline_parse_ipaddr.c
> @@ -277,7 +277,7 @@ cmdline_parse_ipaddr(cmdline_parse_token_hdr_t *tk, const char *buf, void *res,
> if (token_len >= INET6_ADDRSTRLEN+4)
> return -1;
>
> - snprintf(ip_str, token_len+1, "%s", buf);
> + strlcpy(ip_str, buf, token_len + 1);
>
> /* convert the network prefix */
> if (tk2->ipaddr_data.flags & CMDLINE_IPADDR_NETWORK) {
> diff --git a/lib/librte_cmdline/cmdline_parse_portlist.c b/lib/librte_cmdline/cmdline_parse_portlist.c
> index 5952f3438..ad43b522e 100644
> --- a/lib/librte_cmdline/cmdline_parse_portlist.c
> +++ b/lib/librte_cmdline/cmdline_parse_portlist.c
> @@ -94,7 +94,7 @@ cmdline_parse_portlist(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
> if (token_len >= PORTLIST_TOKEN_SIZE)
> return -1;
>
> - snprintf(portlist_str, token_len+1, "%s", buf);
> + strlcpy(portlist_str, buf, token_len + 1);
>
> if (pl) {
> pl->map = 0;
> diff --git a/lib/librte_cmdline/cmdline_parse_string.c b/lib/librte_cmdline/cmdline_parse_string.c
> index abde0412a..9cf41d0f7 100644
> --- a/lib/librte_cmdline/cmdline_parse_string.c
> +++ b/lib/librte_cmdline/cmdline_parse_string.c
> @@ -125,10 +125,10 @@ cmdline_parse_string(cmdline_parse_token_hdr_t *tk, const char *buf, void *res,
> if (res) {
> if ((sd->str != NULL) && (strcmp(sd->str, TOKEN_STRING_MULTI) == 0))
> /* we are sure that token_len is < STR_MULTI_TOKEN_SIZE-1 */
> - snprintf(res, STR_MULTI_TOKEN_SIZE, "%s", buf);
> + strlcpy(res, buf, STR_MULTI_TOKEN_SIZE);
> else
> /* we are sure that token_len is < STR_TOKEN_SIZE-1 */
> - snprintf(res, STR_TOKEN_SIZE, "%s", buf);
> + strlcpy(res, buf, STR_TOKEN_SIZE);
>
> *((char *)res + token_len) = 0;
> }
> diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> index 3e022d510..0943851cc 100644
> --- a/lib/librte_eal/common/eal_common_bus.c
> +++ b/lib/librte_eal/common/eal_common_bus.c
> @@ -36,6 +36,7 @@
>
> #include <rte_bus.h>
> #include <rte_debug.h>
> +#include <rte_string_fns.h>
>
> #include "eal_private.h"
>
> @@ -212,7 +213,7 @@ rte_bus_find_by_device_name(const char *str)
> char name[RTE_DEV_NAME_MAX_LEN];
> char *c;
>
> - snprintf(name, sizeof(name), "%s", str);
> + strlcpy(name, str, sizeof(name));
> c = strchr(name, ',');
> if (c != NULL)
> c[0] = '\0';
> 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)
> +
> +#endif /* RTE_USE_LIBBSD */
> +#endif /* BSDAPP */
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
> index ec8a5d84c..41a9deeba 100644
> --- a/lib/librte_pdump/rte_pdump.c
> +++ b/lib/librte_pdump/rte_pdump.c
> @@ -17,6 +17,7 @@
> #include <rte_lcore.h>
> #include <rte_log.h>
> #include <rte_errno.h>
> +#include <rte_string_fns.h>
>
> #include "rte_pdump.h"
>
> @@ -401,9 +402,9 @@ pdump_get_socket_path(char *buffer, int bufsz, enum rte_pdump_socktype type)
> int ret = 0;
>
> if (type == RTE_PDUMP_SOCKET_SERVER && server_socket_dir[0] != 0)
> - snprintf(dir, sizeof(dir), "%s", server_socket_dir);
> + strlcpy(dir, server_socket_dir, sizeof(dir));
> else if (type == RTE_PDUMP_SOCKET_CLIENT && client_socket_dir[0] != 0)
> - snprintf(dir, sizeof(dir), "%s", client_socket_dir);
> + strlcpy(dir, client_socket_dir, sizeof(dir));
> else {
> if (getuid() != 0) {
> dir_home = getenv(SOCKET_PATH_HOME);
> @@ -891,10 +892,10 @@ rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type)
> if (path != NULL) {
> if (type == RTE_PDUMP_SOCKET_SERVER) {
> count = sizeof(server_socket_dir);
> - ret = snprintf(server_socket_dir, count, "%s", path);
> + ret = strlcpy(server_socket_dir, path, count);
> } else {
> count = sizeof(client_socket_dir);
> - ret = snprintf(client_socket_dir, count, "%s", path);
> + ret = strlcpy(client_socket_dir, path, count);
> }
>
> if (ret < 0 || ret >= count) {
> diff --git a/test/test/test_cmdline_cirbuf.c b/test/test/test_cmdline_cirbuf.c
> index e9193f66c..8ac326cb0 100644
> --- a/test/test/test_cmdline_cirbuf.c
> +++ b/test/test/test_cmdline_cirbuf.c
> @@ -483,7 +483,7 @@ test_cirbuf_string_get_del_partial(void)
> memset(tmp, 0, sizeof(tmp));
> memset(tmp2, 0, sizeof(tmp));
>
> - snprintf(tmp2, sizeof(tmp2), "%s", CIRBUF_STR_HEAD);
> + strlcpy(tmp2, CIRBUF_STR_HEAD, sizeof(tmp2));
>
> /*
> * initialize circular buffer
> diff --git a/test/test/test_eal_flags.c b/test/test/test_eal_flags.c
> index 37c42efe8..93eb7a481 100644
> --- a/test/test/test_eal_flags.c
> +++ b/test/test/test_eal_flags.c
> @@ -1151,11 +1151,12 @@ test_memory_flags(void)
> /* add one extra socket */
> for (i = 0; i < num_sockets + 1; i++) {
> snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem, DEFAULT_MEM_SIZE);
> - snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "%s", buf);
> + strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
>
> if (num_sockets + 1 - i > 1) {
> snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
> - snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "%s", buf);
> + strlcpy(invalid_socket_mem, buf,
> + sizeof(invalid_socket_mem));
> }
> }
>
> @@ -1167,11 +1168,12 @@ test_memory_flags(void)
> /* add one extra socket */
> for (i = 0; i < num_sockets; i++) {
> snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem, DEFAULT_MEM_SIZE);
> - snprintf(valid_socket_mem, sizeof(valid_socket_mem), "%s", buf);
> + strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
>
> if (num_sockets - i > 1) {
> snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
> - snprintf(valid_socket_mem, sizeof(valid_socket_mem), "%s", buf);
> + strlcpy(valid_socket_mem, buf,
> + sizeof(valid_socket_mem));
> }
> }
>
> diff --git a/test/test/test_malloc.c b/test/test/test_malloc.c
> index d23192cf1..ccc5feaec 100644
> --- a/test/test/test_malloc.c
> +++ b/test/test/test_malloc.c
> @@ -378,7 +378,7 @@ test_realloc(void)
> printf("NULL pointer returned from rte_zmalloc\n");
> return -1;
> }
> - snprintf(ptr1, size1, "%s" ,hello_str);
> + strlcpy(ptr1, hello_str, size1);
> char *ptr2 = rte_realloc(ptr1, size2, RTE_CACHE_LINE_SIZE);
> if (!ptr2){
> rte_free(ptr1);
> --
> 2.14.3
>
Safety first, looks good to me.
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply [flat|nested] 9+ messages in thread
* [dpdk-dev] [PATCH 1/2] add support for strlcpy function
2018-02-20 17:07 [dpdk-dev] [RFC PATCH] use strlcpy for string copies Bruce Richardson
` (2 preceding siblings ...)
2018-02-23 18:11 ` Matteo Croce
@ 2018-03-12 11:32 ` Bruce Richardson
2018-03-12 11:33 ` [dpdk-dev] [PATCH 2/2] convert snprintf to strlcpy Bruce Richardson
2018-03-12 11:51 ` [dpdk-dev] [PATCH 1/2] add support for strlcpy function Bruce Richardson
3 siblings, 2 replies; 9+ messages in thread
From: Bruce Richardson @ 2018-03-12 11:32 UTC (permalink / raw)
To: dev; +Cc: Bruce Richardson
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, 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 patch 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.
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
----
RFC->v1:
* split patch into 2, to separate out snprintf replacement
* added missing include in mlx drivers
* adding linking support against libbsd when feature is enabled
* make strlcpy an inline function rather than macro
* fix support on BSD where strict posix compliance is requested and
therefore strlcpy is not available.
---
config/common_base | 1 +
config/meson.build | 7 +++++++
devtools/cocci/strlcpy.cocci | 8 +++++++
lib/librte_eal/common/include/rte_string_fns.h | 29 ++++++++++++++++++++++++++
mk/rte.app.mk | 3 +++
5 files changed, 48 insertions(+)
create mode 100644 devtools/cocci/strlcpy.cocci
diff --git a/config/common_base b/config/common_base
index ad03cf433..20d4cbcf2 100644
--- a/config/common_base
+++ b/config/common_base
@@ -76,6 +76,7 @@ CONFIG_RTE_EAL_VFIO=n
CONFIG_RTE_MAX_VFIO_GROUPS=64
CONFIG_RTE_MALLOC_DEBUG=n
CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n
+CONFIG_RTE_USE_LIBBSD=n
#
# Recognize/ignore the AVX/AVX512 CPU flags for performance/power testing.
diff --git a/config/meson.build b/config/meson.build
index f8c67578d..77af5d897 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -38,6 +38,13 @@ if numa_dep.found() and cc.has_header('numaif.h')
dpdk_extra_ldflags += '-lnuma'
endif
+# check for strlcpy
+if host_machine.system() == 'linux' and cc.find_library('bsd', required: false).found()
+ dpdk_conf.set('RTE_USE_LIBBSD', 1)
+ add_project_link_arguments('-lbsd', language: 'c')
+ dpdk_extra_ldflags += '-lbsd'
+endif
+
# add -include rte_config to cflags
add_project_arguments('-include', 'rte_config.h', language: 'c')
diff --git a/devtools/cocci/strlcpy.cocci b/devtools/cocci/strlcpy.cocci
new file mode 100644
index 000000000..335e27128
--- /dev/null
+++ b/devtools/cocci/strlcpy.cocci
@@ -0,0 +1,8 @@
+@use_strlcpy@
+identifier src, dst;
+expression size;
+@@
+(
+- snprintf(dst, size, "%s", src)
++ strlcpy(dst, src, size)
+)
diff --git a/lib/librte_eal/common/include/rte_string_fns.h b/lib/librte_eal/common/include/rte_string_fns.h
index e97047a47..f1f5b17b8 100644
--- a/lib/librte_eal/common/include/rte_string_fns.h
+++ b/lib/librte_eal/common/include/rte_string_fns.h
@@ -45,6 +45,35 @@ int
rte_strsplit(char *string, int stringlen,
char **tokens, int maxtokens, char delim);
+/**
+ * @internal
+ * DPDK-specific version of strlcpy for systems without
+ * libc or libbsd copies of the function
+ */
+static inline size_t
+rte_strlcpy(char *dst, const char *src, size_t size)
+{
+ return snprintf(dst, size, "%s", src);
+}
+
+/* pull in a strlcpy function */
+#ifdef RTE_EXEC_ENV_BSDAPP
+#include <string.h>
+#ifndef __BSD_VISIBLE /* non-standard functions are hidden */
+#define strlcpy(dst, src, size) rte_strlcpy(dst, src, size)
+#endif
+
+
+#else /* non-BSD platforms */
+#ifdef RTE_USE_LIBBSD
+#include <bsd/string.h>
+
+#else /* no BSD header files, create own */
+#define strlcpy(dst, src, size) rte_strlcpy(dst, src, size)
+
+#endif /* RTE_USE_LIBBSD */
+#endif /* BSDAPP */
+
#ifdef __cplusplus
}
#endif
diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 3eb41d176..39511da93 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -248,6 +248,9 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_EAL) += -lrt
ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP)$(CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES),yy)
_LDLIBS-$(CONFIG_RTE_LIBRTE_EAL) += -lnuma
endif
+ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP)$(CONFIG_RTE_USE_LIBBSD),yy)
+_LDLIBS-$(CONFIG_RTE_LIBRTE_EAL) += -lbsd
+endif
_LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED) += -lm
_LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED) += -lrt
_LDLIBS-$(CONFIG_RTE_LIBRTE_MEMBER) += -lm
--
2.14.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [dpdk-dev] [PATCH 2/2] convert snprintf to strlcpy
2018-03-12 11:32 ` [dpdk-dev] [PATCH 1/2] add support for strlcpy function Bruce Richardson
@ 2018-03-12 11:33 ` 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
1 sibling, 2 replies; 9+ messages in thread
From: Bruce Richardson @ 2018-03-12 11:33 UTC (permalink / raw)
To: dev; +Cc: Bruce Richardson
Since we have support for the strlcpy function in DPDK, replace all
instances where a string is copied using snprintf.
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
app/pdump/main.c | 11 +++++------
app/test-pmd/parameters.c | 5 ++---
drivers/net/bonding/rte_eth_bond_pmd.c | 3 ++-
drivers/net/failsafe/failsafe_args.c | 5 +++--
drivers/net/mlx4/mlx4_ethdev.c | 3 ++-
drivers/net/mlx5/mlx5_ethdev.c | 3 ++-
drivers/net/mrvl/mrvl_qos.c | 2 +-
drivers/net/tap/rte_eth_tap.c | 5 +++--
.../ip_pipeline/pipeline/pipeline_flow_classification_be.c | 4 ++--
examples/ip_pipeline/pipeline/pipeline_passthrough_be.c | 4 ++--
examples/load_balancer/config.c | 2 +-
lib/librte_cmdline/cmdline_parse.c | 6 +++---
lib/librte_cmdline/cmdline_parse_etheraddr.c | 2 +-
lib/librte_cmdline/cmdline_parse_ipaddr.c | 2 +-
lib/librte_cmdline/cmdline_parse_portlist.c | 2 +-
lib/librte_cmdline/cmdline_parse_string.c | 4 ++--
lib/librte_eal/common/eal_common_bus.c | 3 ++-
lib/librte_pdump/rte_pdump.c | 9 +++++----
test/test/test_cmdline_cirbuf.c | 2 +-
test/test/test_eal_flags.c | 10 ++++++----
test/test/test_malloc.c | 2 +-
21 files changed, 48 insertions(+), 41 deletions(-)
diff --git a/app/pdump/main.c b/app/pdump/main.c
index f6865bdbd..d29de0321 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -24,6 +24,7 @@
#include <rte_kvargs.h>
#include <rte_mempool.h>
#include <rte_ring.h>
+#include <rte_string_fns.h>
#include <rte_pdump.h>
#define CMD_LINE_OPT_PDUMP "pdump"
@@ -408,17 +409,15 @@ launch_args_parse(int argc, char **argv, char *prgname)
if (!strncmp(long_option[option_index].name,
CMD_LINE_OPT_SER_SOCK_PATH,
sizeof(CMD_LINE_OPT_SER_SOCK_PATH))) {
- snprintf(server_socket_path,
- sizeof(server_socket_path), "%s",
- optarg);
+ strlcpy(server_socket_path, optarg,
+ sizeof(server_socket_path));
}
if (!strncmp(long_option[option_index].name,
CMD_LINE_OPT_CLI_SOCK_PATH,
sizeof(CMD_LINE_OPT_CLI_SOCK_PATH))) {
- snprintf(client_socket_path,
- sizeof(client_socket_path), "%s",
- optarg);
+ strlcpy(client_socket_path, optarg,
+ sizeof(client_socket_path));
}
break;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 97d22b860..2192bdcdf 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -658,9 +658,8 @@ launch_args_parse(int argc, char** argv)
if (!strcmp(lgopts[opt_idx].name, "cmdline-file")) {
printf("CLI commands to be read from %s\n",
optarg);
- snprintf(cmdline_filename,
- sizeof(cmdline_filename), "%s",
- optarg);
+ strlcpy(cmdline_filename, optarg,
+ sizeof(cmdline_filename));
}
if (!strcmp(lgopts[opt_idx].name, "auto-start")) {
printf("Auto-start selected\n");
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index c34c3251f..f7e73351e 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -17,6 +17,7 @@
#include <rte_bus_vdev.h>
#include <rte_alarm.h>
#include <rte_cycles.h>
+#include <rte_string_fns.h>
#include "rte_eth_bond.h"
#include "rte_eth_bond_private.h"
@@ -617,7 +618,7 @@ mode6_debug(const char __attribute__((unused)) *info, struct ether_hdr *eth_h,
uint16_t offset = get_vlan_offset(eth_h, ðer_type);
#ifdef RTE_LIBRTE_BOND_DEBUG_ALB
- snprintf(buf, 16, "%s", info);
+ strlcpy(buf, info, 16);
#endif
if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4)) {
diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
index 366dbea16..ea934f92a 100644
--- a/drivers/net/failsafe/failsafe_args.c
+++ b/drivers/net/failsafe/failsafe_args.c
@@ -14,6 +14,7 @@
#include <rte_devargs.h>
#include <rte_malloc.h>
#include <rte_kvargs.h>
+#include <rte_string_fns.h>
#include "failsafe_private.h"
@@ -340,7 +341,7 @@ fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])
a = b + 1;
}
out:
- snprintf(params, DEVARGS_MAXLEN, "%s", buffer);
+ strlcpy(params, buffer, DEVARGS_MAXLEN);
return 0;
}
@@ -392,7 +393,7 @@ failsafe_args_parse(struct rte_eth_dev *dev, const char *params)
ret = 0;
priv->subs_tx = FAILSAFE_MAX_ETHPORTS;
/* default parameters */
- n = snprintf(mut_params, sizeof(mut_params), "%s", params);
+ n = strlcpy(mut_params, params, sizeof(mut_params));
if (n >= sizeof(mut_params)) {
ERROR("Parameter string too long (>=%zu)",
sizeof(mut_params));
diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c
index 3bc692731..44863183e 100644
--- a/drivers/net/mlx4/mlx4_ethdev.c
+++ b/drivers/net/mlx4/mlx4_ethdev.c
@@ -39,6 +39,7 @@
#include <rte_ether.h>
#include <rte_flow.h>
#include <rte_pci.h>
+#include <rte_string_fns.h>
#include "mlx4.h"
#include "mlx4_flow.h"
@@ -120,7 +121,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') {
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 666507691..0ee2d88ce 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -34,6 +34,7 @@
#include <rte_interrupts.h>
#include <rte_alarm.h>
#include <rte_malloc.h>
+#include <rte_string_fns.h>
#include "mlx5.h"
#include "mlx5_glue.h"
@@ -163,7 +164,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')
diff --git a/drivers/net/mrvl/mrvl_qos.c b/drivers/net/mrvl/mrvl_qos.c
index fbb368131..edc4d92d6 100644
--- a/drivers/net/mrvl/mrvl_qos.c
+++ b/drivers/net/mrvl/mrvl_qos.c
@@ -190,7 +190,7 @@ get_entry_values(const char *entry, uint8_t *tab,
return -1;
/* Copy the entry to safely use rte_strsplit(). */
- snprintf(entry_cpy, RTE_DIM(entry_cpy), "%s", entry);
+ strlcpy(entry_cpy, entry, RTE_DIM(entry_cpy));
/*
* If there are more tokens than array size, rte_strsplit will
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f09db0ea9..fbba9aa63 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -15,6 +15,7 @@
#include <rte_net.h>
#include <rte_debug.h>
#include <rte_ip.h>
+#include <rte_string_fns.h>
#include <sys/types.h>
#include <sys/stat.h>
@@ -1548,7 +1549,7 @@ set_interface_name(const char *key __rte_unused,
char *name = (char *)extra_args;
if (value)
- snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s", value);
+ strlcpy(name, value, RTE_ETH_NAME_MAX_LEN - 1);
else
snprintf(name, RTE_ETH_NAME_MAX_LEN - 1, "%s%d",
DEFAULT_TAP_NAME, (tap_unit - 1));
@@ -1564,7 +1565,7 @@ set_remote_iface(const char *key __rte_unused,
char *name = (char *)extra_args;
if (value)
- snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s", value);
+ strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
return 0;
}
diff --git a/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c b/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c
index 097ec3469..3e26ae86b 100644
--- a/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c
+++ b/examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c
@@ -8,6 +8,7 @@
#include <rte_malloc.h>
#include <rte_table_hash.h>
#include <rte_byteorder.h>
+#include <rte_string_fns.h>
#include <pipeline.h>
#include "pipeline_flow_classification_be.h"
@@ -280,8 +281,7 @@ pipeline_fc_parse_args(struct pipeline_flow_classification *p,
"\"%s\" is too long", params->name,
arg_name);
- snprintf(key_mask_str, mask_str_len + 1, "%s",
- arg_value);
+ strlcpy(key_mask_str, arg_value, mask_str_len + 1);
continue;
}
diff --git a/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c b/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c
index b2bbaedda..1c44e75df 100644
--- a/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c
+++ b/examples/ip_pipeline/pipeline/pipeline_passthrough_be.c
@@ -8,6 +8,7 @@
#include <rte_common.h>
#include <rte_malloc.h>
#include <rte_byteorder.h>
+#include <rte_string_fns.h>
#include <rte_table_stub.h>
#include <rte_table_hash.h>
#include <rte_pipeline.h>
@@ -524,8 +525,7 @@ pipeline_passthrough_parse_args(struct pipeline_passthrough_params *p,
"\"%s\" too long", params->name,
arg_name);
- snprintf(dma_mask_str, mask_str_len + 1,
- "%s", arg_value);
+ strlcpy(dma_mask_str, arg_value, mask_str_len + 1);
p->dma_enabled = 1;
diff --git a/examples/load_balancer/config.c b/examples/load_balancer/config.c
index b5b66368d..972c85c5b 100644
--- a/examples/load_balancer/config.c
+++ b/examples/load_balancer/config.c
@@ -121,7 +121,7 @@ str_to_unsigned_array(
int i, num_splits = 0;
/* copy s so we don't modify original string */
- snprintf(str, sizeof(str), "%s", s);
+ strlcpy(str, s, sizeof(str));
num_splits = rte_strsplit(str, sizeof(str), splits, num_vals, separator);
errno = 0;
diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index e88e4e110..961f9befd 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -251,7 +251,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
}
#ifdef RTE_LIBRTE_CMDLINE_DEBUG
- snprintf(debug_buf, (linelen>64 ? 64 : linelen), "%s", buf);
+ strlcpy(debug_buf, buf, (linelen > 64 ? 64 : linelen));
debug_printf("Parse line : len=%d, <%s>\n", linelen, debug_buf);
#endif
@@ -436,7 +436,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
if ((unsigned)(comp_len + 1) > size)
return 0;
- snprintf(dst, size, "%s", comp_buf);
+ strlcpy(dst, comp_buf, size);
dst[comp_len] = 0;
return 2;
}
@@ -513,7 +513,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
continue;
}
(*state)++;
- l=snprintf(dst, size, "%s", tmpbuf);
+ l=strlcpy(dst, tmpbuf, size);
if (l>=0 && token_hdr.ops->get_help) {
token_hdr.ops->get_help(token_p, tmpbuf,
sizeof(tmpbuf));
diff --git a/lib/librte_cmdline/cmdline_parse_etheraddr.c b/lib/librte_cmdline/cmdline_parse_etheraddr.c
index 8d2811926..24e04755c 100644
--- a/lib/librte_cmdline/cmdline_parse_etheraddr.c
+++ b/lib/librte_cmdline/cmdline_parse_etheraddr.c
@@ -102,7 +102,7 @@ cmdline_parse_etheraddr(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
(token_len != ETHER_ADDRSTRLENSHORT - 1))
return -1;
- snprintf(ether_str, token_len+1, "%s", buf);
+ strlcpy(ether_str, buf, token_len + 1);
tmp = my_ether_aton(ether_str);
if (tmp == NULL)
diff --git a/lib/librte_cmdline/cmdline_parse_ipaddr.c b/lib/librte_cmdline/cmdline_parse_ipaddr.c
index ae6ea1007..d34436abc 100644
--- a/lib/librte_cmdline/cmdline_parse_ipaddr.c
+++ b/lib/librte_cmdline/cmdline_parse_ipaddr.c
@@ -277,7 +277,7 @@ cmdline_parse_ipaddr(cmdline_parse_token_hdr_t *tk, const char *buf, void *res,
if (token_len >= INET6_ADDRSTRLEN+4)
return -1;
- snprintf(ip_str, token_len+1, "%s", buf);
+ strlcpy(ip_str, buf, token_len + 1);
/* convert the network prefix */
if (tk2->ipaddr_data.flags & CMDLINE_IPADDR_NETWORK) {
diff --git a/lib/librte_cmdline/cmdline_parse_portlist.c b/lib/librte_cmdline/cmdline_parse_portlist.c
index 5952f3438..ad43b522e 100644
--- a/lib/librte_cmdline/cmdline_parse_portlist.c
+++ b/lib/librte_cmdline/cmdline_parse_portlist.c
@@ -94,7 +94,7 @@ cmdline_parse_portlist(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
if (token_len >= PORTLIST_TOKEN_SIZE)
return -1;
- snprintf(portlist_str, token_len+1, "%s", buf);
+ strlcpy(portlist_str, buf, token_len + 1);
if (pl) {
pl->map = 0;
diff --git a/lib/librte_cmdline/cmdline_parse_string.c b/lib/librte_cmdline/cmdline_parse_string.c
index abde0412a..9cf41d0f7 100644
--- a/lib/librte_cmdline/cmdline_parse_string.c
+++ b/lib/librte_cmdline/cmdline_parse_string.c
@@ -125,10 +125,10 @@ cmdline_parse_string(cmdline_parse_token_hdr_t *tk, const char *buf, void *res,
if (res) {
if ((sd->str != NULL) && (strcmp(sd->str, TOKEN_STRING_MULTI) == 0))
/* we are sure that token_len is < STR_MULTI_TOKEN_SIZE-1 */
- snprintf(res, STR_MULTI_TOKEN_SIZE, "%s", buf);
+ strlcpy(res, buf, STR_MULTI_TOKEN_SIZE);
else
/* we are sure that token_len is < STR_TOKEN_SIZE-1 */
- snprintf(res, STR_TOKEN_SIZE, "%s", buf);
+ strlcpy(res, buf, STR_TOKEN_SIZE);
*((char *)res + token_len) = 0;
}
diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
index 3e022d510..0943851cc 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -36,6 +36,7 @@
#include <rte_bus.h>
#include <rte_debug.h>
+#include <rte_string_fns.h>
#include "eal_private.h"
@@ -212,7 +213,7 @@ rte_bus_find_by_device_name(const char *str)
char name[RTE_DEV_NAME_MAX_LEN];
char *c;
- snprintf(name, sizeof(name), "%s", str);
+ strlcpy(name, str, sizeof(name));
c = strchr(name, ',');
if (c != NULL)
c[0] = '\0';
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index ec8a5d84c..41a9deeba 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -17,6 +17,7 @@
#include <rte_lcore.h>
#include <rte_log.h>
#include <rte_errno.h>
+#include <rte_string_fns.h>
#include "rte_pdump.h"
@@ -401,9 +402,9 @@ pdump_get_socket_path(char *buffer, int bufsz, enum rte_pdump_socktype type)
int ret = 0;
if (type == RTE_PDUMP_SOCKET_SERVER && server_socket_dir[0] != 0)
- snprintf(dir, sizeof(dir), "%s", server_socket_dir);
+ strlcpy(dir, server_socket_dir, sizeof(dir));
else if (type == RTE_PDUMP_SOCKET_CLIENT && client_socket_dir[0] != 0)
- snprintf(dir, sizeof(dir), "%s", client_socket_dir);
+ strlcpy(dir, client_socket_dir, sizeof(dir));
else {
if (getuid() != 0) {
dir_home = getenv(SOCKET_PATH_HOME);
@@ -891,10 +892,10 @@ rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type)
if (path != NULL) {
if (type == RTE_PDUMP_SOCKET_SERVER) {
count = sizeof(server_socket_dir);
- ret = snprintf(server_socket_dir, count, "%s", path);
+ ret = strlcpy(server_socket_dir, path, count);
} else {
count = sizeof(client_socket_dir);
- ret = snprintf(client_socket_dir, count, "%s", path);
+ ret = strlcpy(client_socket_dir, path, count);
}
if (ret < 0 || ret >= count) {
diff --git a/test/test/test_cmdline_cirbuf.c b/test/test/test_cmdline_cirbuf.c
index e9193f66c..8ac326cb0 100644
--- a/test/test/test_cmdline_cirbuf.c
+++ b/test/test/test_cmdline_cirbuf.c
@@ -483,7 +483,7 @@ test_cirbuf_string_get_del_partial(void)
memset(tmp, 0, sizeof(tmp));
memset(tmp2, 0, sizeof(tmp));
- snprintf(tmp2, sizeof(tmp2), "%s", CIRBUF_STR_HEAD);
+ strlcpy(tmp2, CIRBUF_STR_HEAD, sizeof(tmp2));
/*
* initialize circular buffer
diff --git a/test/test/test_eal_flags.c b/test/test/test_eal_flags.c
index 37c42efe8..93eb7a481 100644
--- a/test/test/test_eal_flags.c
+++ b/test/test/test_eal_flags.c
@@ -1151,11 +1151,12 @@ test_memory_flags(void)
/* add one extra socket */
for (i = 0; i < num_sockets + 1; i++) {
snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem, DEFAULT_MEM_SIZE);
- snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "%s", buf);
+ strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
if (num_sockets + 1 - i > 1) {
snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
- snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "%s", buf);
+ strlcpy(invalid_socket_mem, buf,
+ sizeof(invalid_socket_mem));
}
}
@@ -1167,11 +1168,12 @@ test_memory_flags(void)
/* add one extra socket */
for (i = 0; i < num_sockets; i++) {
snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem, DEFAULT_MEM_SIZE);
- snprintf(valid_socket_mem, sizeof(valid_socket_mem), "%s", buf);
+ strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
if (num_sockets - i > 1) {
snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
- snprintf(valid_socket_mem, sizeof(valid_socket_mem), "%s", buf);
+ strlcpy(valid_socket_mem, buf,
+ sizeof(valid_socket_mem));
}
}
diff --git a/test/test/test_malloc.c b/test/test/test_malloc.c
index d23192cf1..ccc5feaec 100644
--- a/test/test/test_malloc.c
+++ b/test/test/test_malloc.c
@@ -378,7 +378,7 @@ test_realloc(void)
printf("NULL pointer returned from rte_zmalloc\n");
return -1;
}
- snprintf(ptr1, size1, "%s" ,hello_str);
+ strlcpy(ptr1, hello_str, size1);
char *ptr2 = rte_realloc(ptr1, size2, RTE_CACHE_LINE_SIZE);
if (!ptr2){
rte_free(ptr1);
--
2.14.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] convert snprintf to strlcpy
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
1 sibling, 0 replies; 9+ messages in thread
From: Bruce Richardson @ 2018-03-12 11:50 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger
On Mon, Mar 12, 2018 at 11:33:00AM +0000, Bruce Richardson wrote:
> Since we have support for the strlcpy function in DPDK, replace all
> instances where a string is copied using snprintf.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
Forgot to include Stephen's review from RFC, so adding here for
patchwork to pick up:
Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] add support for strlcpy function
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:51 ` Bruce Richardson
1 sibling, 0 replies; 9+ messages in thread
From: Bruce Richardson @ 2018-03-12 11:51 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger
On Mon, Mar 12, 2018 at 11:32:59AM +0000, Bruce Richardson wrote:
> 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, 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 patch 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.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>
> ----
> RFC->v1:
> * split patch into 2, to separate out snprintf replacement
> * added missing include in mlx drivers
> * adding linking support against libbsd when feature is enabled
> * make strlcpy an inline function rather than macro
> * fix support on BSD where strict posix compliance is requested and
> therefore strlcpy is not available.
> ---
Forgot to include Stephen's reviewed-by from RFC:
Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] convert snprintf to strlcpy
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
1 sibling, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2018-04-04 13:26 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
12/03/2018 12:33, Bruce Richardson:
> Since we have support for the strlcpy function in DPDK, replace all
> instances where a string is copied using snprintf.
Added occurences (from recent merge):
drivers/net/mvpp2/mrvl_qos.c
drivers/net/tap/rte_eth_tap.c
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Applied (and rebased), thanks
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-04-04 13:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-20 17:07 [dpdk-dev] [RFC PATCH] use strlcpy for string copies Bruce Richardson
2018-02-20 23:01 ` Stephen Hemminger
2018-02-23 17:18 ` Adrien Mazarguil
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
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).