Skip to contentSkip to conversation

string truncation warning fixes

PATCH In review Stephen Hemminger 2 participants 11 messages Mar 19, 2018 – Mar 20, 2018

Subsystems (3)
Networking Network drivers Library code
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

cover string truncation warning fixes

Building iproute2 with gcc-8 shows some new warning about string truncation. These are obvious patches to fix most of these. Still need to fix lib/bpf.c and tc/m_ematch.c

Stephen Hemminger (5):
ip: use strlcpy() to avoid truncation
tunnel: use strlcpy to avoid strncpy warnings
bridge: avoid snprint truncation on time
pedit: fix strncpy warning
namespace: limit length of network namespace

 bridge/mdb.c    |  4 ++--
 ip/iplink.c     | 14 +++++++-------
 ip/tunnel.c     | 12 ++++++------
 lib/namespace.c | 14 +++++++++++---
 misc/nstat.c    |  4 ++--
 misc/ss.c       |  2 +-
 tc/m_ematch.c   |  2 +-
 tc/m_pedit.c    |  2 +-
 tc/tc_class.c   |  5 +++--
 9 files changed, 34 insertions(+), 25 deletions(-)
Signature · 1 line
2.16.2
author
#1 lore raw

PATCH 1/5 ip: use strlcpy() to avoid truncation

This fixes gcc-8 warnings about strncpy bounds by using strlcpy instead.

Signed-off-by Stephen Hemminger <[email protected]>
---
 ip/iplink.c     | 14 +++++++-------
 lib/namespace.c |  6 ++++--
 misc/nstat.c    |  4 ++--
 misc/ss.c       |  2 +-
 tc/m_ematch.c   |  2 +-
 tc/tc_class.c   |  5 +++--
 6 files changed, 18 insertions(+), 15 deletions(-)
ip/iplink.c +7 -7
index d401311bcad9..cca530eeeb09 100644
@@ -1114,7 +1114,7 @@ static int do_chflags(const char *dev, __u32 flags, __u32 mask)
 	int fd;
 	int err;
 
-	strncpy(ifr.ifr_name, dev, IFNAMSIZ);
+	strlcpy(ifr.ifr_name, dev, IFNAMSIZ);
 	fd = get_ctl_fd();
 	if (fd < 0)
 		return -1;

@@ -1141,8 +1141,8 @@ static int do_changename(const char *dev, const char *newdev)
 	int fd;
 	int err;
 
-	strncpy(ifr.ifr_name, dev, IFNAMSIZ);
-	strncpy(ifr.ifr_newname, newdev, IFNAMSIZ);
+	strlcpy(ifr.ifr_name, dev, IFNAMSIZ);
+	strlcpy(ifr.ifr_newname, newdev, IFNAMSIZ);
 	fd = get_ctl_fd();
 	if (fd < 0)
 		return -1;

@@ -1165,7 +1165,7 @@ static int set_qlen(const char *dev, int qlen)
 	if (s < 0)
 		return -1;
 
-	strncpy(ifr.ifr_name, dev, IFNAMSIZ);
+	strlcpy(ifr.ifr_name, dev, IFNAMSIZ);
 	if (ioctl(s, SIOCSIFTXQLEN, &ifr) < 0) {
 		perror("SIOCSIFXQLEN");
 		close(s);

@@ -1185,7 +1185,7 @@ static int set_mtu(const char *dev, int mtu)
 	if (s < 0)
 		return -1;
 
-	strncpy(ifr.ifr_name, dev, IFNAMSIZ);
+	strlcpy(ifr.ifr_name, dev, IFNAMSIZ);
 	if (ioctl(s, SIOCSIFMTU, &ifr) < 0) {
 		perror("SIOCSIFMTU");
 		close(s);

@@ -1212,7 +1212,7 @@ static int get_address(const char *dev, int *htype)
 		return -1;
 	}
 
-	strncpy(ifr.ifr_name, dev, IFNAMSIZ);
+	strlcpy(ifr.ifr_name, dev, IFNAMSIZ);
 	if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
 		perror("SIOCGIFINDEX");
 		close(s);

@@ -1243,7 +1243,7 @@ static int parse_address(const char *dev, int hatype, int halen,
 	int alen;
 
 	memset(ifr, 0, sizeof(*ifr));
-	strncpy(ifr->ifr_name, dev, IFNAMSIZ);
+	strlcpy(ifr->ifr_name, dev, IFNAMSIZ);
 	ifr->ifr_hwaddr.sa_family = hatype;
 	alen = ll_addr_a2n(ifr->ifr_hwaddr.sa_data, 14, lla);
 	if (alen < 0)
lib/namespace.c +4 -2
index 6f3356d0fa08..682634028587 100644
@@ -23,7 +23,8 @@ static void bind_etc(const char *name)
 	struct dirent *entry;
 	DIR *dir;
 
-	snprintf(etc_netns_path, sizeof(etc_netns_path), "%s/%s", NETNS_ETC_DIR, name);
+	snprintf(etc_netns_path, sizeof(etc_netns_path), "%s/%s",
+		 NETNS_ETC_DIR, name);
 	dir = opendir(etc_netns_path);
 	if (!dir)
 		return;

@@ -33,7 +34,8 @@ static void bind_etc(const char *name)
 			continue;
 		if (strcmp(entry->d_name, "..") == 0)
 			continue;
-		snprintf(netns_name, sizeof(netns_name), "%s/%s", etc_netns_path, entry->d_name);
+		snprintf(netns_name, sizeof(netns_name),
+			 "%s/%s", etc_netns_path, entry->d_name);
 		snprintf(etc_name, sizeof(etc_name), "/etc/%s", entry->d_name);
 		if (mount(netns_name, etc_name, "none", MS_BIND, NULL) < 0) {
 			fprintf(stderr, "Bind %s -> %s failed: %s\n",
misc/nstat.c +2 -2
index a4dd405d43a9..433a1f483be3 100644
@@ -178,12 +178,12 @@ static int count_spaces(const char *line)
 
 static void load_ugly_table(FILE *fp)
 {
-	char buf[4096];
+	char buf[2048];
 	struct nstat_ent *db = NULL;
 	struct nstat_ent *n;
 
 	while (fgets(buf, sizeof(buf), fp) != NULL) {
-		char idbuf[sizeof(buf)];
+		char idbuf[4096];
 		int  off;
 		char *p;
 		int count1, count2, skip = 0;
misc/ss.c +1 -1
index e087bef739b0..a03fa4a7c174 100644
@@ -4032,7 +4032,7 @@ static int netlink_show_one(struct filter *f,
 
 		if (!pid) {
 			done = 1;
-			strncpy(procname, "kernel", 6);
+			strncpy(procname, "kernel", 7);
 		} else if (pid > 0) {
 			FILE *fp;
tc/m_ematch.c +1 -1
index d2bb5c380382..ee8981f0146c 100644
@@ -161,7 +161,7 @@ static struct ematch_util *get_ematch_kind(char *kind)
 
 static struct ematch_util *get_ematch_kind_num(__u16 kind)
 {
-	char name[32];
+	char name[512];
 
 	if (lookup_map(kind, name, sizeof(name), EMATCH_MAP) < 0)
 		return NULL;
tc/tc_class.c +3 -2
index 1b214b82c702..91802518bb27 100644
@@ -219,7 +219,7 @@ static void graph_cls_show(FILE *fp, char *buf, struct hlist_head *root_list,
 	char cls_id_str[256] = {};
 	struct rtattr *tb[TCA_MAX + 1];
 	struct qdisc_util *q;
-	char str[100] = {};
+	char str[300] = {};
 
 	hlist_for_each_safe(n, tmp_cls, root_list) {
 		struct hlist_node *c, *tmp_chld;

@@ -242,7 +242,8 @@ static void graph_cls_show(FILE *fp, char *buf, struct hlist_head *root_list,
 		graph_indent(buf, cls, 0, 0);
 
 		print_tc_classid(cls_id_str, sizeof(cls_id_str), cls->id);
-		sprintf(str, "+---(%s)", cls_id_str);
+		snprintf(str, sizeof(str),
+			 "+---(%s)", cls_id_str);
 		strcat(buf, str);
 
 		parse_rtattr(tb, TCA_MAX, (struct rtattr *)cls->data,
Signature · 1 line
2.16.2
#2 lore raw

Re: [PATCH iproute2 1/5] ip: use strlcpy() to avoid truncation

Quoted 15 lines · Stephen Hemminger
lib/namespace.c +2 -1
index 6f3356d0fa08..682634028587 100644
@@ -23,7 +23,8 @@ static void bind_etc(const char *name)
 	struct dirent *entry;
 	DIR *dir;
 
-	snprintf(etc_netns_path, sizeof(etc_netns_path), "%s/%s", NETNS_ETC_DIR, name);
+	snprintf(etc_netns_path, sizeof(etc_netns_path), "%s/%s",
+		 NETNS_ETC_DIR, name);
 	dir = opendir(etc_netns_path);
 	if (!dir)
 		return;

lib/namespace.c static void bind_etc(const char *name) · line 41

Quoted 9 lines
lib/namespace.c +2 -1
@@ -33,7 +34,8 @@ static void bind_etc(const char *name)
 			continue;
 		if (strcmp(entry->d_name, "..") == 0)
 			continue;
-		snprintf(netns_name, sizeof(netns_name), "%s/%s", etc_netns_path, entry->d_name);
+		snprintf(netns_name, sizeof(netns_name),
+			 "%s/%s", etc_netns_path, entry->d_name);
 		snprintf(etc_name, sizeof(etc_name), "/etc/%s", entry->d_name);
 		if (mount(netns_name, etc_name, "none", MS_BIND, NULL) < 0) {
 			fprintf(stderr, "Bind %s -> %s failed: %s\n",

above is unrelated to strncpy -> strlcpy change And pretty much everything below as well.

Quoted 6 lines
misc/nstat.c +0 -0
index a4dd405d43a9..433a1f483be3 100644
@@ -178,12 +178,12 @@ static int count_spaces(const char *line)

misc/nstat.c static int count_spaces(const char *line) · line 186

Quoted 10 lines
misc/nstat.c +2 -2
 static void load_ugly_table(FILE *fp)
 {
-	char buf[4096];
+	char buf[2048];
 	struct nstat_ent *db = NULL;
 	struct nstat_ent *n;
 
 	while (fgets(buf, sizeof(buf), fp) != NULL) {
-		char idbuf[sizeof(buf)];
+		char idbuf[4096];

Also, I don't understand why you flipped the sizes with idbuf twice as large as buf.

Quoted 23 lines
misc/nstat.c +0 -0
 		int  off;
 		char *p;
 		int count1, count2, skip = 0;
misc/ss.c +1 -1
index e087bef739b0..a03fa4a7c174 100644
@@ -4032,7 +4032,7 @@ static int netlink_show_one(struct filter *f,
 
 		if (!pid) {
 			done = 1;
-			strncpy(procname, "kernel", 6);
+			strncpy(procname, "kernel", 7);
 
 
 		} else if (pid > 0) {
misc/ss.c +0 -0
 			FILE *fp;
tc/m_ematch.c +0 -0
index d2bb5c380382..ee8981f0146c 100644

tc/m_ematch.c static struct ematch_util *get_ematch_kind(char *kind) · line 164

Quoted 5 lines
tc/m_ematch.c +1 -1
@@ -161,7 +161,7 @@ static struct ematch_util *get_ematch_kind(char *kind)
 
 static struct ematch_util *get_ematch_kind_num(__u16 kind)
 {
-	char name[32];
+	char name[512];

why the size bump?

tc/tc_class.c static void graph_cls_show(FILE *fp, char *buf, struct hlist_head *root_list, · line 222

Quoted 13 lines
tc/m_ematch.c +0 -0
 	if (lookup_map(kind, name, sizeof(name), EMATCH_MAP) < 0)
 		return NULL;
tc/tc_class.c +1 -1
index 1b214b82c702..91802518bb27 100644
@@ -219,7 +219,7 @@ static void graph_cls_show(FILE *fp, char *buf, struct hlist_head *root_list,
 	char cls_id_str[256] = {};
 	struct rtattr *tb[TCA_MAX + 1];
 	struct qdisc_util *q;
-	char str[100] = {};
+	char str[300] = {};

and here.

tc/tc_class.c static void graph_cls_show(FILE *fp, char *buf, struct hlist_head *root_list, · line 246

Quoted 10 lines
tc/tc_class.c +2 -1
 	hlist_for_each_safe(n, tmp_cls, root_list) {
 		struct hlist_node *c, *tmp_chld;

@@ -242,7 +242,8 @@ static void graph_cls_show(FILE *fp, char *buf, struct hlist_head *root_list,
 		graph_indent(buf, cls, 0, 0);
 
 		print_tc_classid(cls_id_str, sizeof(cls_id_str), cls->id);
-		sprintf(str, "+---(%s)", cls_id_str);
+		snprintf(str, sizeof(str),
+			 "+---(%s)", cls_id_str);

That can all fit on one line.

Quoted 4 lines
tc/tc_class.c +0 -0
 		strcat(buf, str);
 
 		parse_rtattr(tb, TCA_MAX, (struct rtattr *)cls->data,
author
#3 lore raw

PATCH 2/5 tunnel: use strlcpy to avoid strncpy warnings

In reply to Stephen Hemminger

Fixes warnings about strncpy size by using strlcpy.

tunnel.c: In function ‘tnl_gen_ioctl’:
tunnel.c:145:2: warning: ‘strncpy’ specified bound 16 equals destination size [-Wstringop-truncation]
strncpy(ifr.ifr_name, name, IFNAMSIZ);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by Stephen Hemminger <[email protected]>
---
 ip/tunnel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
ip/tunnel.c +6 -6
index 948d5f7c90f6..abd9fa2ffe0c 100644
@@ -64,7 +64,7 @@ int tnl_get_ioctl(const char *basedev, void *p)
 	int fd;
 	int err;
 
-	strncpy(ifr.ifr_name, basedev, IFNAMSIZ);
+	strlcpy(ifr.ifr_name, basedev, IFNAMSIZ);
 	ifr.ifr_ifru.ifru_data = (void *)p;
 
 	fd = socket(preferred_family, SOCK_DGRAM, 0);

@@ -89,9 +89,9 @@ int tnl_add_ioctl(int cmd, const char *basedev, const char *name, void *p)
 	int err;
 
 	if (cmd == SIOCCHGTUNNEL && name[0])
-		strncpy(ifr.ifr_name, name, IFNAMSIZ);
+		strlcpy(ifr.ifr_name, name, IFNAMSIZ);
 	else
-		strncpy(ifr.ifr_name, basedev, IFNAMSIZ);
+		strlcpy(ifr.ifr_name, basedev, IFNAMSIZ);
 	ifr.ifr_ifru.ifru_data = p;
 
 	fd = socket(preferred_family, SOCK_DGRAM, 0);

@@ -115,9 +115,9 @@ int tnl_del_ioctl(const char *basedev, const char *name, void *p)
 	int err;
 
 	if (name[0])
-		strncpy(ifr.ifr_name, name, IFNAMSIZ);
+		strlcpy(ifr.ifr_name, name, IFNAMSIZ);
 	else
-		strncpy(ifr.ifr_name, basedev, IFNAMSIZ);
+		strlcpy(ifr.ifr_name, basedev, IFNAMSIZ);
 
 	ifr.ifr_ifru.ifru_data = p;
 

@@ -142,7 +142,7 @@ static int tnl_gen_ioctl(int cmd, const char *name,
 	int fd;
 	int err;
 
-	strncpy(ifr.ifr_name, name, IFNAMSIZ);
+	strlcpy(ifr.ifr_name, name, IFNAMSIZ);
 	ifr.ifr_ifru.ifru_data = p;
 
 	fd = socket(preferred_family, SOCK_DGRAM, 0);
Signature · 1 line
2.16.2
Acked-by on v1 2/5
#4 lore raw

Re: [PATCH iproute2 2/5] tunnel: use strlcpy to avoid strncpy warnings

Quoted 7 lines · Stephen Hemminger

Fixes warnings about strncpy size by using strlcpy.

tunnel.c: In function ‘tnl_gen_ioctl’:
tunnel.c:145:2: warning: ‘strncpy’ specified bound 16 equals destination size [-Wstringop-truncation]
strncpy(ifr.ifr_name, name, IFNAMSIZ);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Quoted 5 lines
Signed-off-by Stephen Hemminger <[email protected]>
---
 ip/tunnel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Acked-by David Ahern <[email protected]>
author
#5 lore raw

PATCH 3/5 bridge: avoid snprint truncation on time

In reply to Stephen Hemminger

This fixes new gcc warning about possible string overflow.

mdb.c: In function ‘__print_router_port_stats’:
mdb.c:61:11: warning: ‘%.2i’ directive output may be truncated writing between 2 and 7 bytes into a region of size between 0 and 4 [-Wformat-truncation=]

      "%4i.%.2i", (int)tv.tv_sec,
           ^~~~

Note: already fixed in iproute2-next.

Signed-off-by Stephen Hemminger <[email protected]>
---
 bridge/mdb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
bridge/mdb.c +2 -2
index 58c20b82b8a6..659cac3ff20a 100644
@@ -55,7 +55,7 @@ static void __print_router_port_stats(FILE *f, struct rtattr *pattr)
 		__jiffies_to_tv(&tv,
 				rta_getattr_u32(tb[MDBA_ROUTER_PATTR_TIMER]));
 		if (jw_global) {
-			char formatted_time[9];
+			char formatted_time[32];
 
 			snprintf(formatted_time, sizeof(formatted_time),
 				 "%4i.%.2i", (int)tv.tv_sec,

@@ -184,7 +184,7 @@ static void print_mdb_entry(FILE *f, int ifindex, struct br_mdb_entry *e,
 
 		__jiffies_to_tv(&tv, rta_getattr_u32(tb[MDBA_MDB_EATTR_TIMER]));
 		if (jw_global) {
-			char formatted_time[9];
+			char formatted_time[32];
 
 			snprintf(formatted_time, sizeof(formatted_time),
 				 "%4i.%.2i", (int)tv.tv_sec,
Signature · 1 line
2.16.2
Acked-by on v1 3/5
#6 lore raw

Re: [PATCH iproute2 3/5] bridge: avoid snprint truncation on time

Quoted 7 lines · Stephen Hemminger

This fixes new gcc warning about possible string overflow.

mdb.c: In function ‘__print_router_port_stats’:
mdb.c:61:11: warning: ‘%.2i’ directive output may be truncated writing between 2 and 7 bytes into a region of size between 0 and 4 [-Wformat-truncation=]

      "%4i.%.2i", (int)tv.tv_sec,
           ^~~~
Quoted 7 lines

Note: already fixed in iproute2-next.

Signed-off-by Stephen Hemminger <[email protected]>
---
 bridge/mdb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Acked-by David Ahern <[email protected]>
author
#7 lore raw

PATCH 4/5 pedit: fix strncpy warning

In reply to Stephen Hemminger

Newer versions of Gcc warn about string truncation. Fix by using strlcpy.

Signed-off-by Stephen Hemminger <[email protected]>
---
 tc/m_pedit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tc/m_pedit.c +1 -1
index 26549eeea899..8577f875a7c0 100644
@@ -111,7 +111,7 @@ reg:
 noexist:
 	p = calloc(1, sizeof(*p));
 	if (p) {
-		strncpy(p->id, str, sizeof(p->id) - 1);
+		strlcpy(p->id, str, sizeof(p->id));
 		p->parse_peopt = pedit_parse_nopopt;
 		goto reg;
 	}
Signature · 1 line
2.16.2
author
#9 lore raw

PATCH 5/5 namespace: limit length of network namespace

In reply to Stephen Hemminger

Avoid running into buffer overflows with excessively long network namespace. Fixes Gcc-8 warning about possible snprintf truncation.

Signed-off-by Stephen Hemminger <[email protected]>
---
 lib/namespace.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
lib/namespace.c +7 -1
index 682634028587..ce5683a5f4e6 100644
@@ -18,7 +18,7 @@
 static void bind_etc(const char *name)
 {
 	char etc_netns_path[PATH_MAX];
-	char netns_name[PATH_MAX];
+	char netns_name[2*PATH_MAX];
 	char etc_name[PATH_MAX];
 	struct dirent *entry;
 	DIR *dir;

@@ -52,6 +52,12 @@ int netns_switch(char *name)
 	unsigned long mountflags = 0;
 	struct statvfs fsstat;
 
+	if (strlen(name) >= NAME_MAX) {
+		fprintf(stderr, "Network namespace name too long\"%s\"\n",
+			name);
+		return -1;
+	}
+
 	snprintf(net_path, sizeof(net_path), "%s/%s", NETNS_RUN_DIR, name);
 	netns = open(net_path, O_RDONLY | O_CLOEXEC);
 	if (netns < 0) {
Signature · 1 line
2.16.2
#10 lore raw

Re: [PATCH iproute2 5/5] namespace: limit length of network namespace

Quoted 25 lines · Stephen Hemminger

Avoid running into buffer overflows with excessively long network namespace. Fixes Gcc-8 warning about possible snprintf truncation.

Signed-off-by Stephen Hemminger <[email protected]>
---
 lib/namespace.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
lib/namespace.c +1 -1
index 682634028587..ce5683a5f4e6 100644
@@ -18,7 +18,7 @@
 static void bind_etc(const char *name)
 {
 	char etc_netns_path[PATH_MAX];
-	char netns_name[PATH_MAX];
+	char netns_name[2*PATH_MAX];
 	char etc_name[PATH_MAX];
 	struct dirent *entry;
 	DIR *dir;

@@ -52,6 +52,12 @@ int netns_switch(char *name)
 	unsigned long mountflags = 0;
 	struct statvfs fsstat;

lib/namespace.c int netns_switch(char *name) · line 63

Quoted 10 lines
lib/namespace.c +6 -0
 
+	if (strlen(name) >= NAME_MAX) {
+		fprintf(stderr, "Network namespace name too long\"%s\"\n",
+			name);
+		return -1;
+	}
+
 	snprintf(net_path, sizeof(net_path), "%s/%s", NETNS_RUN_DIR, name);
 	netns = open(net_path, O_RDONLY | O_CLOEXEC);
 	if (netns < 0) {

Since PATH_MAX is a Linux limit for file paths, why not ensure strlen(name) + strlen(NETNS_RUN_DIR) + 2 <= PATH_MAX