From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH] examples/ipsec-secgw: replace strncpy with strlcpy Date: Wed, 9 May 2018 14:35:50 +0100 Message-ID: <20180509133549.GA25048@bricha3-MOBL.ger.corp.intel.com> References: <1525865729-16086-1-git-send-email-reshma.pattan@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, stable@dpdk.org, "Zhang,Roy Fan" To: Reshma Pattan Return-path: Content-Disposition: inline In-Reply-To: <1525865729-16086-1-git-send-email-reshma.pattan@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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: stable@dpdk.org > CC: Zhang,Roy Fan > > Signed-off-by: Reshma Pattan > --- > 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 > #include > +#include > > #include > #include > @@ -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 >