On Wed, May 09, 2018 at 12:35:27PM +0100, Reshma Pattan wrote: > Use strlcpy instead of strncpy. > > Fixes: 0d547ed037 ("examples/ipsec-secgw: support configuration file") > Fixes: 07b156199f ("examples/ipsec-secgw: fix configuration string > termination") > Fixes: a1469c319f ("examples/ipsec-secgw: fix configuration parsing") > Cc: sta...@dpdk.org > CC: Zhang,Roy Fan <roy.fan.zh...@intel.com> > > Signed-off-by: Reshma Pattan <reshma.pat...@intel.com> > --- > examples/ipsec-secgw/parser.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/examples/ipsec-secgw/parser.c b/examples/ipsec-secgw/parser.c > index 2403b564d..9ccd5ea72 100644 > --- a/examples/ipsec-secgw/parser.c > +++ b/examples/ipsec-secgw/parser.c > @@ -3,6 +3,7 @@ > */ > #include <rte_common.h> > #include <rte_crypto.h> > +#include <rte_string_fns.h> > > #include <cmdline_parse_string.h> > #include <cmdline_parse_num.h> > @@ -212,14 +213,14 @@ parse_ipv4_addr(const char *token, struct in_addr > *ipv4, uint32_t *mask) > > pch = strchr(token, '/'); > if (pch != NULL) { > - strncpy(ip_str, token, pch - token); > + strlcpy(ip_str, token, pch - token);
While this is fixing the compiler error, it's not really doing any bounds checking for overflow on the destination buffer. Ideally, the final parameter should be something like: min(pch - token, sizeof(ip_str)) > pch += 1; > if (is_str_num(pch) != 0) > return -EINVAL; > if (mask) > *mask = atoi(pch); > } else { > - strncpy(ip_str, token, sizeof(ip_str) - 1); > + strlcpy(ip_str, token, sizeof(ip_str) - 1); Since the original code was using strncpy, it's possible the "- 1" was an incorrect attempt to make strncpy safe. Therefore, did you check to see if it's possible to drop the -1 in the strlcpy case? > if (mask) > *mask = 0; > } > @@ -241,14 +242,14 @@ parse_ipv6_addr(const char *token, struct in6_addr > *ipv6, uint32_t *mask) > > pch = strchr(token, '/'); > if (pch != NULL) { > - strncpy(ip_str, token, pch - token); > + strlcpy(ip_str, token, pch - token); As before, this doesn't do proper bounds checking. > pch += 1; > if (is_str_num(pch) != 0) > return -EINVAL; > if (mask) > *mask = atoi(pch); > } else { > - strncpy(ip_str, token, sizeof(ip_str) - 1); > + strlcpy(ip_str, token, sizeof(ip_str) - 1); As before, can we remove the -1? > if (mask) > *mask = 0; > } > @@ -515,7 +516,7 @@ parse_cfg_file(const char *cfg_filename) > goto error_exit; > } > > - strncpy(str + strlen(str), oneline, > + strlcpy(str + strlen(str), oneline, > strlen(oneline)); This doesn't do bounds checking, and since it just uses strlen to find the bounds it can just be replaced by a strcpy() - which will also be more efficient too, since it would only scan the string once, rather than twice as here. So, either add in a proper bounds check on the destination buffer, or if a bounds check is not necessary, just replace with strcpy to show its not actually needing a bounds check. > > continue; > @@ -528,7 +529,7 @@ parse_cfg_file(const char *cfg_filename) > cfg_filename, line_num); > goto error_exit; > } > - strncpy(str + strlen(str), oneline, > + strlcpy(str + strlen(str), oneline, > strlen(oneline)); As above. > > str[strlen(str)] = '\n'; > -- > 2.14.3 >