From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f196.google.com (mail-wr0-f196.google.com [209.85.128.196]) by dpdk.org (Postfix) with ESMTP id 849784C9D for ; Fri, 23 Feb 2018 18:18:23 +0100 (CET) Received: by mail-wr0-f196.google.com with SMTP id n7so14861089wrn.5 for ; Fri, 23 Feb 2018 09:18:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=0ekOTFhx7eTUeUG8FmQI8qbYR2IDVkzlS8rlN5T3xLo=; b=a6Pi1XaomeWs6aYyOd+3GVJd2NlG0wEVHajF7then0tru8+PeIX8ZqHSJioMsZHsNF qBsekCGD7jJ47enHPiwu+xGXHstOBuhOWAxFoLocUv6bq7SZXi/FjQ0Esy0jhugZy9vx 7INecx0yVVwgg75vI+dIsORKviC+TseVMWeDGNN5eVqIpDZB3BrBvx2sdbRxMpLlueFW KcQ+lYBnPB+tMIE/iNCY1eHBHO7CMvgeAMV1+f4VuR25T5RUpQuXfGOHGbSCYh+RB/60 qD4lgvwtov1vc0fYTOomoL2blT6WwiFByAx4/f2sJHtsj2Tv395LcviGrNKOX1ZrsUmq 9v9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=0ekOTFhx7eTUeUG8FmQI8qbYR2IDVkzlS8rlN5T3xLo=; b=Rj2Qfl0UkUDjp2p+Y+VmPVezWnsIiXmaA8DShT2m0RcwPdd9qqh4e4v8V8lwq08xVk Tk93fpeV42X22leW6A6920dL400V/V3md3VnW8Hxcf4er+RvhEkmqdY+5KCaPcV+l2PH O9UUnH+YQBEEC9xwgj0sMEsz3bERIQh3h6DbJjt2uUPw/TqnKTf0SwSrL9B9zZFL4j0P d38jAPnKfbRbsqNehqaKIOOZXK5uJLDloptevH78f/xlR2BMvAmJc0uf237RbQwcB4k6 GDAz1q7gkcIjVuriEqy1AsKeHoH3BJnO5XCG4jU+bUDPQF4a+ypG4Pg8LHCz2YquLBJw SJcQ== X-Gm-Message-State: APf1xPD0xPx05FHwJjo1KZcNl830F1Flqny2x/LOGkBbojx1h2Bv+5r+ 0tB1k/JZoKUn0U9cawy1t8liEw== X-Google-Smtp-Source: AH8x224VpFbFtNzciYVgaPCHqToiX1B+KPpgsQVMx78SV4jEwXh1vJxTuNwRDkWrTD/RaezEoUBDXw== X-Received: by 10.223.178.26 with SMTP id u26mr2421591wra.63.1519406303217; Fri, 23 Feb 2018 09:18:23 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id s81sm3546672wma.45.2018.02.23.09.18.21 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 23 Feb 2018 09:18:22 -0800 (PST) Date: Fri, 23 Feb 2018 18:18:09 +0100 From: Adrien Mazarguil To: Bruce Richardson Cc: dev@dpdk.org Message-ID: <20180223171809.GM4256@6wind.com> References: <20180220170727.220340-1-bruce.richardson@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180220170727.220340-1-bruce.richardson@intel.com> Subject: Re: [dpdk-dev] [RFC PATCH] use strlcpy for string copies 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: Fri, 23 Feb 2018 17:18:23 -0000 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 > * 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 OK with the RFC, a few comments below regarding mlx4, mlx5 and the definition itself though. > 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 > 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). > 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 > + > +#else /* non-BSD platforms */ > +#ifdef RTE_USE_LIBBSD > +#include > + > +#else /* no BSD header files, create own */ > +#define strlcpy(dst, src, size) snprintf(dst, size, "%s", src) Missing #include 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