From: Bruce Richardson <bruce.richardson@intel.com>
To: Reshma Pattan <reshma.pattan@intel.com>
Cc: dev@dpdk.org, stable@dpdk.org, "Zhang,Roy Fan" <roy.fan.zhang@intel.com>
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/ipsec-secgw: replace strncpy with strlcpy
Date: Wed, 9 May 2018 14:35:50 +0100 [thread overview]
Message-ID: <20180509133549.GA25048@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <1525865729-16086-1-git-send-email-reshma.pattan@intel.com>
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 <roy.fan.zhang@intel.com>
>
> Signed-off-by: Reshma Pattan <reshma.pattan@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
>
next prev parent reply other threads:[~2018-05-09 13:35 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-09 11:35 [dpdk-stable] " Reshma Pattan
2018-05-09 11:35 ` [dpdk-stable] [PATCH] examples/quota_watermark: fix gcc 8.0.1 cast between incompatible types Reshma Pattan
2018-05-11 15:48 ` De Lara Guarch, Pablo
2018-05-13 21:45 ` Thomas Monjalon
2018-05-09 11:35 ` [dpdk-stable] [PATCH] examples/vhost_scsi: replace strncpy with strlcpy Reshma Pattan
2018-05-09 13:37 ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
2018-05-09 16:38 ` Pattan, Reshma
2018-05-10 12:24 ` Pattan, Reshma
2018-05-10 12:05 ` [dpdk-stable] [PATCH v2] " Reshma Pattan
2018-05-10 13:31 ` Bruce Richardson
2018-05-13 21:50 ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
2018-05-09 13:35 ` Bruce Richardson [this message]
2018-05-09 15:56 ` [dpdk-stable] [PATCH v2] examples/ipsec-secgw: " Reshma Pattan
2018-05-09 16:11 ` [dpdk-stable] [PATCH v3] " Reshma Pattan
2018-05-11 16:38 ` De Lara Guarch, Pablo
2018-05-13 21:52 ` Thomas Monjalon
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=20180509133549.GA25048@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=reshma.pattan@intel.com \
--cc=roy.fan.zhang@intel.com \
--cc=stable@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).