[iproute PATCH 0/6] strlcpy() and strlcat() for iproute2

[iproute PATCH 0/6] strlcpy() and strlcat() for iproute2

[Phil Sutter]

The following series adds my own implementations of strlcpy() and
strlcat() in patch 1 and changes the code to make use of them in the
following patches but the last two: Patch 5 just eliminates a line of
useless code I found while searching for potential users of the
introduced functions, patch 6 sanitizes a call to strncpy() in
misc/lnstat_util.c without using strlcpy() since lnstat is not being
linked against libutil.

I implemented both functions solely based on information in libbsd's man
pages, so they are safe to be released under the GPL.

Phil Sutter (6):
  utils: Implement strlcpy() and strlcat()
  Convert the obvious cases to strlcpy()
  Convert harmful calls to strncpy() to strlcpy()
  ipxfrm: Replace STRBUF_CAT macro with strlcat()
  tc_util: No need to terminate an snprintf'ed buffer
  lnstat_util: Make sure buffer is NUL-terminated

 genl/ctrl.c           |  2 +-
 include/utils.h       |  3 +++
 ip/ipnetns.c          |  3 +--
 ip/iproute_lwtunnel.c |  3 +--
 ip/ipvrf.c            |  5 ++---
 ip/ipxfrm.c           | 21 +++++----------------
 ip/xfrm_state.c       |  2 +-
 lib/bpf.c             |  3 +--
 lib/fs.c              |  3 +--
 lib/inet_proto.c      |  3 +--
 lib/utils.c           | 19 +++++++++++++++++++
 misc/lnstat_util.c    |  3 ++-
 misc/ss.c             |  3 +--
 tc/em_ipset.c         |  3 +--
 tc/tc_util.c          |  1 -
 15 files changed, 40 insertions(+), 37 deletions(-)

-- 
2.13.1

[iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()

[Phil Sutter]

By making use of strncpy(), both implementations are really simple so
there is no need to add libbsd as additional dependency.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/utils.h |  3 +++
 lib/utils.c     | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/utils.h b/include/utils.h
index f665d9001806f..9c2f9fc257fba 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -252,4 +252,7 @@ int make_path(const char *path, mode_t mode);
 char *find_cgroup2_mount(void);
 int get_command_name(const char *pid, char *comm, size_t len);
 
+size_t strlcpy(char *dst, const char *src, size_t size);
+size_t strlcat(char *dst, const char *src, size_t size);
+
 #endif /* __UTILS_H__ */
diff --git a/lib/utils.c b/lib/utils.c
index 002063075fd61..c95780e725252 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1238,3 +1238,22 @@ int get_real_family(int rtm_type, int rtm_family)
 
 	return rtm_family;
 }
+
+size_t strlcpy(char *dst, const char *src, size_t size)
+{
+	if (size) {
+		strncpy(dst, src, size - 1);
+		dst[size - 1] = '\0';
+	}
+	return strlen(src);
+}
+
+size_t strlcat(char *dst, const char *src, size_t size)
+{
+	size_t dlen = strlen(dst);
+
+	if (dlen > size)
+		return dlen + strlen(src);
+
+	return dlen + strlcpy(dst + dlen, src, size - dlen);
+}
-- 
2.13.1

RE: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()

[David Laight]

From: Phil Sutter
> Sent: 01 September 2017 17:53
> By making use of strncpy(), both implementations are really simple so
> there is no need to add libbsd as additional dependency.
> 
...
> +
> +size_t strlcpy(char *dst, const char *src, size_t size)
> +{
> +	if (size) {
> +		strncpy(dst, src, size - 1);
> +		dst[size - 1] = '\0';
> +	}
> +	return strlen(src);
> +}

Except that isn't really strlcpy().
Better would be:
	len = strlen(src) + 1;
	if (len <= size)
		memcpy(dst, src, len);
	else if (size) {
		dst[size - 1] = 0;
		memcpy(dst, src, size - 1);
	}
	return len - 1;

WTF strlcpy() has that return value I don't know.

	David

Re: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()

[Phil Sutter]

On Mon, Sep 04, 2017 at 02:49:20PM +0000, David Laight wrote:
> From: Phil Sutter
> > Sent: 01 September 2017 17:53
> > By making use of strncpy(), both implementations are really simple so
> > there is no need to add libbsd as additional dependency.
> > 
> ...
> > +
> > +size_t strlcpy(char *dst, const char *src, size_t size)
> > +{
> > +	if (size) {
> > +		strncpy(dst, src, size - 1);
> > +		dst[size - 1] = '\0';
> > +	}
> > +	return strlen(src);
> > +}
> 
> Except that isn't really strlcpy().
> Better would be:
> 	len = strlen(src) + 1;
> 	if (len <= size)
> 		memcpy(dst, src, len);
> 	else if (size) {
> 		dst[size - 1] = 0;
> 		memcpy(dst, src, size - 1);
> 	}
> 	return len - 1;

Please elaborate: Why isn't my version "really" strlcpy()? Why is your
proposed version better?

Thanks, Phil

Re: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()

[Stephen Hemminger]

On Mon, 4 Sep 2017 17:00:15 +0200
Phil Sutter <phil@nwl.cc> wrote:

> On Mon, Sep 04, 2017 at 02:49:20PM +0000, David Laight wrote:
> > From: Phil Sutter  
> > > Sent: 01 September 2017 17:53
> > > By making use of strncpy(), both implementations are really simple so
> > > there is no need to add libbsd as additional dependency.
> > >   
> > ...  
> > > +
> > > +size_t strlcpy(char *dst, const char *src, size_t size)
> > > +{
> > > +	if (size) {
> > > +		strncpy(dst, src, size - 1);
> > > +		dst[size - 1] = '\0';
> > > +	}
> > > +	return strlen(src);
> > > +}  
> > 
> > Except that isn't really strlcpy().
> > Better would be:
> > 	len = strlen(src) + 1;
> > 	if (len <= size)
> > 		memcpy(dst, src, len);
> > 	else if (size) {
> > 		dst[size - 1] = 0;
> > 		memcpy(dst, src, size - 1);
> > 	}
> > 	return len - 1;  
> 
> Please elaborate: Why isn't my version "really" strlcpy()? Why is your
> proposed version better?
> 
> Thanks, Phil

Linux kernel:
size_t strlcpy(char *dest, const char *src, size_t size)
{
	size_t ret = strlen(src);

	if (size) {
		size_t len = (ret >= size) ? size - 1 : ret;
		memcpy(dest, src, len);
		dest[len] = '\0';
	}
	return ret;
}

FreeBSD:
size_t
strlcpy(char * __restrict dst, const char * __restrict src, size_t dsize)
{
	const char *osrc = src;
	size_t nleft = dsize;

	/* Copy as many bytes as will fit. */
	if (nleft != 0) {
		while (--nleft != 0) {
			if ((*dst++ = *src++) == '\0')
				break;
		}
	}

	/* Not enough room in dst, add NUL and traverse rest of src. */
	if (nleft == 0) {
		if (dsize != 0)
			*dst = '\0';		/* NUL-terminate dst */
		while (*src++)
			;
	}

	return(src - osrc - 1);	/* count does not include NUL */
}


They all give the same results for some basic tests.
Test			FreeBSD		Linux		Iproute2
"",0:           	0 "JUNK"      	0 "JUNK"      	0 "JUNK"      
"",1:           	0 ""          	0 ""          	0 ""          
"",8:           	0 ""          	0 ""          	0 ""          
"foo",0:        	3 "JUNK"      	3 "JUNK"      	3 "JUNK"      
"foo",3:        	3 "fo"        	3 "fo"        	3 "fo"        
"foo",4:        	3 "foo"       	3 "foo"       	3 "foo"       
"foo",8:        	3 "foo"       	3 "foo"       	3 "foo"       
"longstring",0: 	10 "JUNK"     	10 "JUNK"     	10 "JUNK"     
"longstring",8: 	10 "longstr"  	10 "longstr"  	10 "longstr"  

RE: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()

[David Laight]

From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: 04 September 2017 19:25
> On Mon, 4 Sep 2017 17:00:15 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > On Mon, Sep 04, 2017 at 02:49:20PM +0000, David Laight wrote:
> > > From: Phil Sutter
> > > > Sent: 01 September 2017 17:53
> > > > By making use of strncpy(), both implementations are really simple so
> > > > there is no need to add libbsd as additional dependency.
> > > >
> > > ...
> > > > +
> > > > +size_t strlcpy(char *dst, const char *src, size_t size)
> > > > +{
> > > > +	if (size) {
> > > > +		strncpy(dst, src, size - 1);
> > > > +		dst[size - 1] = '\0';
> > > > +	}
> > > > +	return strlen(src);
> > > > +}
> > >
> > > Except that isn't really strlcpy().
> > > Better would be:
> > > 	len = strlen(src) + 1;
> > > 	if (len <= size)
> > > 		memcpy(dst, src, len);
> > > 	else if (size) {
> > > 		dst[size - 1] = 0;
> > > 		memcpy(dst, src, size - 1);
> > > 	}
> > > 	return len - 1;
> >
> > Please elaborate: Why isn't my version "really" strlcpy()? Why is your
> > proposed version better?
> >
> > Thanks, Phil
> 
> Linux kernel:
> size_t strlcpy(char *dest, const char *src, size_t size)
> {
> 	size_t ret = strlen(src);
> 
> 	if (size) {
> 		size_t len = (ret >= size) ? size - 1 : ret;
> 		memcpy(dest, src, len);
> 		dest[len] = '\0';
> 	}
> 	return ret;
> }
> 
> FreeBSD:
> size_t
> strlcpy(char * __restrict dst, const char * __restrict src, size_t dsize)
> {
> 	const char *osrc = src;
> 	size_t nleft = dsize;
> 
> 	/* Copy as many bytes as will fit. */
> 	if (nleft != 0) {
> 		while (--nleft != 0) {
> 			if ((*dst++ = *src++) == '\0')
> 				break;
> 		}
> 	}
> 
> 	/* Not enough room in dst, add NUL and traverse rest of src. */
> 	if (nleft == 0) {
> 		if (dsize != 0)
> 			*dst = '\0';		/* NUL-terminate dst */
> 		while (*src++)
> 			;
> 	}
> 
> 	return(src - osrc - 1);	/* count does not include NUL */
> }
> 
> 
> They all give the same results for some basic tests.
> Test			FreeBSD		Linux		Iproute2
> "",0:           	0 "JUNK"      	0 "JUNK"      	0 "JUNK"
> "",1:           	0 ""          	0 ""          	0 ""
> "",8:           	0 ""          	0 ""          	0 ""
> "foo",0:        	3 "JUNK"      	3 "JUNK"      	3 "JUNK"
> "foo",3:        	3 "fo"        	3 "fo"        	3 "fo"
> "foo",4:        	3 "foo"       	3 "foo"       	3 "foo"
> "foo",8:        	3 "foo"       	3 "foo"       	3 "foo"
> "longstring",0: 	10 "JUNK"     	10 "JUNK"     	10 "JUNK"
> "longstring",8: 	10 "longstr"  	10 "longstr"  	10 "longstr"

You need to look at the contents of the destination buffer after the
first '\0'.
strlcpy() shouldn't change it.

	David

Re: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()

[Stephen Hemminger]

On Wed, 6 Sep 2017 13:59:27 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: 04 September 2017 19:25
> > On Mon, 4 Sep 2017 17:00:15 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> >   
> > > On Mon, Sep 04, 2017 at 02:49:20PM +0000, David Laight wrote:  
> > > > From: Phil Sutter  
> > > > > Sent: 01 September 2017 17:53
> > > > > By making use of strncpy(), both implementations are really simple so
> > > > > there is no need to add libbsd as additional dependency.
> > > > >  
> > > > ...  
> > > > > +
> > > > > +size_t strlcpy(char *dst, const char *src, size_t size)
> > > > > +{
> > > > > +	if (size) {
> > > > > +		strncpy(dst, src, size - 1);
> > > > > +		dst[size - 1] = '\0';
> > > > > +	}
> > > > > +	return strlen(src);
> > > > > +}  
> > > >
> > > > Except that isn't really strlcpy().
> > > > Better would be:
> > > > 	len = strlen(src) + 1;
> > > > 	if (len <= size)
> > > > 		memcpy(dst, src, len);
> > > > 	else if (size) {
> > > > 		dst[size - 1] = 0;
> > > > 		memcpy(dst, src, size - 1);
> > > > 	}
> > > > 	return len - 1;  
> > >
> > > Please elaborate: Why isn't my version "really" strlcpy()? Why is your
> > > proposed version better?
> > >
> > > Thanks, Phil  
> > 
> > Linux kernel:
> > size_t strlcpy(char *dest, const char *src, size_t size)
> > {
> > 	size_t ret = strlen(src);
> > 
> > 	if (size) {
> > 		size_t len = (ret >= size) ? size - 1 : ret;
> > 		memcpy(dest, src, len);
> > 		dest[len] = '\0';
> > 	}
> > 	return ret;
> > }
> > 
> > FreeBSD:
> > size_t
> > strlcpy(char * __restrict dst, const char * __restrict src, size_t dsize)
> > {
> > 	const char *osrc = src;
> > 	size_t nleft = dsize;
> > 
> > 	/* Copy as many bytes as will fit. */
> > 	if (nleft != 0) {
> > 		while (--nleft != 0) {
> > 			if ((*dst++ = *src++) == '\0')
> > 				break;
> > 		}
> > 	}
> > 
> > 	/* Not enough room in dst, add NUL and traverse rest of src. */
> > 	if (nleft == 0) {
> > 		if (dsize != 0)
> > 			*dst = '\0';		/* NUL-terminate dst */
> > 		while (*src++)
> > 			;
> > 	}
> > 
> > 	return(src - osrc - 1);	/* count does not include NUL */
> > }
> > 
> > 
> > They all give the same results for some basic tests.
> > Test			FreeBSD		Linux		Iproute2
> > "",0:           	0 "JUNK"      	0 "JUNK"      	0 "JUNK"
> > "",1:           	0 ""          	0 ""          	0 ""
> > "",8:           	0 ""          	0 ""          	0 ""
> > "foo",0:        	3 "JUNK"      	3 "JUNK"      	3 "JUNK"
> > "foo",3:        	3 "fo"        	3 "fo"        	3 "fo"
> > "foo",4:        	3 "foo"       	3 "foo"       	3 "foo"
> > "foo",8:        	3 "foo"       	3 "foo"       	3 "foo"
> > "longstring",0: 	10 "JUNK"     	10 "JUNK"     	10 "JUNK"
> > "longstring",8: 	10 "longstr"  	10 "longstr"  	10 "longstr"  
> 
> You need to look at the contents of the destination buffer after the
> first '\0'.
> strlcpy() shouldn't change it.
> 
> 	David

Zeroing the bytes after the first null character should not be a big issue
other than a few nanoseconds extra work.

[iproute PATCH] utils: Review strlcpy() and strlcat()

[Phil Sutter]

As David Laight correctly pointed out, the first version of strlcpy()
modified dst buffer behind the string copied into it. Fix this by
writing NUL to the byte immediately following src string instead of to
the last byte in dst. Doing so also allows to reduce overhead by using
memcpy().

Improve strlcat() by avoiding the call to strlcpy() if dst string is
already full, not just as sanity check.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/utils.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/utils.c b/lib/utils.c
index 330ab073c2068..bbd3cbc46a0e5 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1233,18 +1233,22 @@ int get_real_family(int rtm_type, int rtm_family)
 
 size_t strlcpy(char *dst, const char *src, size_t size)
 {
+	size_t srclen = strlen(src);
+
 	if (size) {
-		strncpy(dst, src, size - 1);
-		dst[size - 1] = '\0';
+		size_t minlen = min(srclen, size - 1);
+
+		memcpy(dst, src, minlen);
+		dst[minlen] = '\0';
 	}
-	return strlen(src);
+	return srclen;
 }
 
 size_t strlcat(char *dst, const char *src, size_t size)
 {
 	size_t dlen = strlen(dst);
 
-	if (dlen > size)
+	if (dlen >= size)
 		return dlen + strlen(src);
 
 	return dlen + strlcpy(dst + dlen, src, size - dlen);
-- 
2.13.1

[iproute PATCH 2/6] Convert the obvious cases to strlcpy()

[Phil Sutter]

This converts the typical idiom of manually terminating the buffer after
a call to strncpy().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipnetns.c          | 3 +--
 ip/iproute_lwtunnel.c | 3 +--
 ip/ipvrf.c            | 3 +--
 lib/bpf.c             | 3 +--
 lib/fs.c              | 3 +--
 lib/inet_proto.c      | 3 +--
 misc/ss.c             | 3 +--
 tc/em_ipset.c         | 3 +--
 8 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 9ee1fe6a51f9c..afb4978a5be7d 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -518,8 +518,7 @@ int netns_identify_pid(const char *pidstr, char *name, int len)
 
 		if ((st.st_dev == netst.st_dev) &&
 		    (st.st_ino == netst.st_ino)) {
-			strncpy(name, entry->d_name, len - 1);
-			name[len - 1] = '\0';
+			strlcpy(name, entry->d_name, len);
 		}
 	}
 	closedir(dir);
diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
index 1a3dc4d4c0ed9..4c2d3b07e3b5c 100644
--- a/ip/iproute_lwtunnel.c
+++ b/ip/iproute_lwtunnel.c
@@ -325,8 +325,7 @@ static int parse_encap_seg6(struct rtattr *rta, size_t len, int *argcp,
 				invarg("\"segs\" provided before \"mode\"\n",
 				       *argv);
 
-			strncpy(segbuf, *argv, 1024);
-			segbuf[1023] = 0;
+			strlcpy(segbuf, *argv, 1024);
 		} else if (strcmp(*argv, "hmac") == 0) {
 			NEXT_ARG();
 			if (hmac_ok++)
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index e6fad32abd956..b74c501e0970c 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -336,8 +336,7 @@ static int vrf_path(char *vpath, size_t len)
 		if (vrf)
 			*vrf = '\0';
 
-		strncpy(vpath, start, len - 1);
-		vpath[len - 1] = '\0';
+		strlcpy(vpath, start, len);
 
 		/* if vrf path is just / then return nothing */
 		if (!strcmp(vpath, "/"))
diff --git a/lib/bpf.c b/lib/bpf.c
index 0bd0a95eafe6c..c180934acc7dc 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -512,8 +512,7 @@ static const char *bpf_find_mntpt_single(unsigned long magic, char *mnt,
 
 	ret = bpf_valid_mntpt(mntpt, magic);
 	if (!ret) {
-		strncpy(mnt, mntpt, len - 1);
-		mnt[len - 1] = 0;
+		strlcpy(mnt, mntpt, len);
 		return mnt;
 	}
 
diff --git a/lib/fs.c b/lib/fs.c
index ebe05cd44e11b..86efd4ed2ed80 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -172,8 +172,7 @@ int get_command_name(const char *pid, char *comm, size_t len)
 		if (nl)
 			*nl = '\0';
 
-		strncpy(comm, name, len - 1);
-		comm[len - 1] = '\0';
+		strlcpy(comm, name, len);
 		break;
 	}
 
diff --git a/lib/inet_proto.c b/lib/inet_proto.c
index 53c029039b6d5..bdfd52fdafe5a 100644
--- a/lib/inet_proto.c
+++ b/lib/inet_proto.c
@@ -38,8 +38,7 @@ const char *inet_proto_n2a(int proto, char *buf, int len)
 			free(ncache);
 		icache = proto;
 		ncache = strdup(pe->p_name);
-		strncpy(buf, pe->p_name, len - 1);
-		buf[len - 1] = '\0';
+		strlcpy(buf, pe->p_name, len);
 		return buf;
 	}
 	snprintf(buf, len, "ipproto-%d", proto);
diff --git a/misc/ss.c b/misc/ss.c
index 2c9e80e696595..dd8dfaa4e70db 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -425,8 +425,7 @@ static void user_ent_hash_build(void)
 
 	user_ent_hash_build_init = 1;
 
-	strncpy(name, root, sizeof(name)-1);
-	name[sizeof(name)-1] = 0;
+	strlcpy(name, root, sizeof(name));
 
 	if (strlen(name) == 0 || name[strlen(name)-1] != '/')
 		strcat(name, "/");
diff --git a/tc/em_ipset.c b/tc/em_ipset.c
index b59756515d239..48b287f5ba3b2 100644
--- a/tc/em_ipset.c
+++ b/tc/em_ipset.c
@@ -145,8 +145,7 @@ get_set_byname(const char *setname, struct xt_set_info *info)
 	int res;
 
 	req.op = IP_SET_OP_GET_BYNAME;
-	strncpy(req.set.name, setname, IPSET_MAXNAMELEN);
-	req.set.name[IPSET_MAXNAMELEN - 1] = '\0';
+	strlcpy(req.set.name, setname, IPSET_MAXNAMELEN);
 	res = do_getsockopt(&req);
 	if (res != 0)
 		return -1;
-- 
2.13.1

Re: [iproute PATCH 2/6] Convert the obvious cases to strlcpy()

[Daniel Borkmann]

On 09/01/2017 06:52 PM, Phil Sutter wrote:
> This converts the typical idiom of manually terminating the buffer after
> a call to strncpy().
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>

For BPF loader bits:

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[iproute PATCH 3/6] Convert harmful calls to strncpy() to strlcpy()

[Phil Sutter]

This patch converts spots where manual buffer termination was missing to
strlcpy() since that does what is needed.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 genl/ctrl.c     | 2 +-
 ip/ipvrf.c      | 2 +-
 ip/xfrm_state.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/genl/ctrl.c b/genl/ctrl.c
index 6abd52582d0d3..448988eb90e2b 100644
--- a/genl/ctrl.c
+++ b/genl/ctrl.c
@@ -313,7 +313,7 @@ static int ctrl_list(int cmd, int argc, char **argv)
 
 		if (matches(*argv, "name") == 0) {
 			NEXT_ARG();
-			strncpy(d, *argv, sizeof (d) - 1);
+			strlcpy(d, *argv, sizeof(d));
 			addattr_l(nlh, 128, CTRL_ATTR_FAMILY_NAME,
 				  d, strlen(d) + 1);
 		} else if (matches(*argv, "id") == 0) {
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index b74c501e0970c..f9277e1e66bdf 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -74,7 +74,7 @@ static int vrf_identify(pid_t pid, char *name, size_t len)
 			if (end)
 				*end = '\0';
 
-			strncpy(name, vrf, len - 1);
+			strlcpy(name, vrf, len);
 			break;
 		}
 	}
diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index e11c93bf1c3b5..4483fb8f71d2c 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -125,7 +125,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg, enum xfrm_attr_type_t type,
 	fprintf(stderr, "warning: ALGO-NAME/ALGO-KEYMAT values will be sent to the kernel promiscuously! (verifying them isn't implemented yet)\n");
 #endif
 
-	strncpy(alg->alg_name, name, sizeof(alg->alg_name));
+	strlcpy(alg->alg_name, name, sizeof(alg->alg_name));
 
 	if (slen > 2 && strncmp(key, "0x", 2) == 0) {
 		/* split two chars "0x" from the top */
-- 
2.13.1

[iproute PATCH 4/6] ipxfrm: Replace STRBUF_CAT macro with strlcat()

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipxfrm.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/ip/ipxfrm.c b/ip/ipxfrm.c
index d5eb22e25476a..12c2f721571b6 100644
--- a/ip/ipxfrm.c
+++ b/ip/ipxfrm.c
@@ -40,17 +40,6 @@
 #include "ip_common.h"
 
 #define STRBUF_SIZE	(128)
-#define STRBUF_CAT(buf, str) \
-	do { \
-		int rest = sizeof(buf) - 1 - strlen(buf); \
-		if (rest > 0) { \
-			int len = strlen(str); \
-			if (len > rest) \
-				len = rest; \
-			strncat(buf, str, len); \
-			buf[sizeof(buf) - 1] = '\0'; \
-		} \
-	} while (0);
 
 struct xfrm_filter filter;
 
@@ -902,8 +891,8 @@ void xfrm_state_info_print(struct xfrm_usersa_info *xsinfo,
 			   prefix, title);
 
 	if (prefix)
-		STRBUF_CAT(buf, prefix);
-	STRBUF_CAT(buf, "\t");
+		strlcat(buf, prefix, sizeof(buf));
+	strlcat(buf, "\t", sizeof(buf));
 
 	fputs(buf, fp);
 	fprintf(fp, "replay-window %u ", xsinfo->replay_window);
@@ -944,7 +933,7 @@ void xfrm_state_info_print(struct xfrm_usersa_info *xsinfo,
 		char sbuf[STRBUF_SIZE];
 
 		memcpy(sbuf, buf, sizeof(sbuf));
-		STRBUF_CAT(sbuf, "sel ");
+		strlcat(sbuf, "sel ", sizeof(sbuf));
 
 		xfrm_selector_print(&xsinfo->sel, xsinfo->family, fp, sbuf);
 	}
@@ -992,8 +981,8 @@ void xfrm_policy_info_print(struct xfrm_userpolicy_info *xpinfo,
 	}
 
 	if (prefix)
-		STRBUF_CAT(buf, prefix);
-	STRBUF_CAT(buf, "\t");
+		strlcat(buf, prefix, sizeof(buf));
+	strlcat(buf, "\t", sizeof(buf));
 
 	fputs(buf, fp);
 	if (xpinfo->dir >= XFRM_POLICY_MAX) {
-- 
2.13.1

[iproute PATCH 5/6] tc_util: No need to terminate an snprintf'ed buffer

[Phil Sutter]

snprintf() won't leave the buffer unterminated, so manually terminating
is not necessary here.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/tc_util.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tc/tc_util.c b/tc/tc_util.c
index 371046839ba9f..50d355046bdad 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -434,7 +434,6 @@ static const char *action_n2a(int action)
 		return "trap";
 	default:
 		snprintf(buf, 64, "%d", action);
-		buf[63] = '\0';
 		return buf;
 	}
 }
-- 
2.13.1

[iproute PATCH 6/6] lnstat_util: Make sure buffer is NUL-terminated

[Phil Sutter]

Can't use strlcpy() here since lnstat is not linked against libutil.

While being at it, fix coding style in that chunk as well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/lnstat_util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/misc/lnstat_util.c b/misc/lnstat_util.c
index ec19238c24b94..c2dc42ec1ff12 100644
--- a/misc/lnstat_util.c
+++ b/misc/lnstat_util.c
@@ -150,7 +150,8 @@ static int lnstat_scan_compat_rtstat_fields(struct lnstat_file *lf)
 {
 	char buf[FGETS_BUF_SIZE];
 
-	strncpy(buf, RTSTAT_COMPAT_LINE, sizeof(buf)-1);
+	strncpy(buf, RTSTAT_COMPAT_LINE, sizeof(buf) - 1);
+	buf[sizeof(buf) - 1] = '\0';
 
 	return __lnstat_scan_fields(lf, buf);
 }
-- 
2.13.1

Re: [iproute PATCH 0/6] strlcpy() and strlcat() for iproute2

[Stephen Hemminger]

On Fri,  1 Sep 2017 18:52:50 +0200
Phil Sutter <phil@nwl.cc> wrote:

> The following series adds my own implementations of strlcpy() and
> strlcat() in patch 1 and changes the code to make use of them in the
> following patches but the last two: Patch 5 just eliminates a line of
> useless code I found while searching for potential users of the
> introduced functions, patch 6 sanitizes a call to strncpy() in
> misc/lnstat_util.c without using strlcpy() since lnstat is not being
> linked against libutil.
> 
> I implemented both functions solely based on information in libbsd's man
> pages, so they are safe to be released under the GPL.
> 
> Phil Sutter (6):
>   utils: Implement strlcpy() and strlcat()
>   Convert the obvious cases to strlcpy()
>   Convert harmful calls to strncpy() to strlcpy()
>   ipxfrm: Replace STRBUF_CAT macro with strlcat()
>   tc_util: No need to terminate an snprintf'ed buffer
>   lnstat_util: Make sure buffer is NUL-terminated
> 
>  genl/ctrl.c           |  2 +-
>  include/utils.h       |  3 +++
>  ip/ipnetns.c          |  3 +--
>  ip/iproute_lwtunnel.c |  3 +--
>  ip/ipvrf.c            |  5 ++---
>  ip/ipxfrm.c           | 21 +++++----------------
>  ip/xfrm_state.c       |  2 +-
>  lib/bpf.c             |  3 +--
>  lib/fs.c              |  3 +--
>  lib/inet_proto.c      |  3 +--
>  lib/utils.c           | 19 +++++++++++++++++++
>  misc/lnstat_util.c    |  3 ++-
>  misc/ss.c             |  3 +--
>  tc/em_ipset.c         |  3 +--
>  tc/tc_util.c          |  1 -
>  15 files changed, 40 insertions(+), 37 deletions(-)
> 

Applied, thanks.