From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 0E5851B644; Wed, 9 May 2018 15:35:56 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 May 2018 06:35:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,381,1520924400"; d="scan'208";a="39899403" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.55]) by orsmga008.jf.intel.com with SMTP; 09 May 2018 06:35:52 -0700 Received: by (sSMTP sendmail emulation); Wed, 09 May 2018 14:35:51 +0100 Date: Wed, 9 May 2018 14:35:50 +0100 From: Bruce Richardson To: Reshma Pattan Cc: dev@dpdk.org, stable@dpdk.org, "Zhang,Roy Fan" 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 Content-Disposition: inline In-Reply-To: <1525865729-16086-1-git-send-email-reshma.pattan@intel.com> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: replace strncpy with strlcpy X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 May 2018 13:35:57 -0000 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 >