[iproute PATCH v2 0/7] Covscan: Misc fixes

[iproute PATCH v2 0/7] Covscan: Misc fixes

[Phil Sutter]

This series collects patches from v1 addressing miscellaneous issues
detected by covscan.

No changes to the actual patches, just splitting into smaller series.

Phil Sutter (7):
  nstat: Avoid passing negative fd to fdopen()
  ss: Make sure index variable is >= 0
  ss: Make sure scanned index value to unix_state_map is sane
  netem/maketable: Check return value of fscanf()
  lib/bpf: Check return value of write()
  lib/fs: Fix and simplify make_path()
  lib/libnetlink: Don't pass NULL parameter to memcpy()

 lib/bpf.c         |  3 ++-
 lib/fs.c          | 20 +++++---------------
 lib/libnetlink.c  |  6 ++++--
 misc/nstat.c      | 15 +++++++++++----
 misc/ss.c         |  5 +++--
 netem/maketable.c |  4 ++--
 6 files changed, 27 insertions(+), 26 deletions(-)

-- 
2.13.1

[iproute PATCH v2 1/7] nstat: Avoid passing negative fd to fdopen()

[Phil Sutter]

Introduce a wrapper which does the sanity checking and returns NULL
in case fd is invalid.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/nstat.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/misc/nstat.c b/misc/nstat.c
index 1212b1f2c8128..7cdde75a56e4e 100644
--- a/misc/nstat.c
+++ b/misc/nstat.c
@@ -252,9 +252,16 @@ static void load_ugly_table(FILE *fp)
 	}
 }
 
+static FILE *fdopen_null(int fd, const char *mode)
+{
+	if (fd < 0)
+		return NULL;
+	return fdopen(fd, mode);
+}
+
 static void load_sctp_snmp(void)
 {
-	FILE *fp = fdopen(net_sctp_snmp_open(), "r");
+	FILE *fp = fdopen_null(net_sctp_snmp_open(), "r");
 
 	if (fp) {
 		load_good_table(fp);
@@ -264,7 +271,7 @@ static void load_sctp_snmp(void)
 
 static void load_snmp(void)
 {
-	FILE *fp = fdopen(net_snmp_open(), "r");
+	FILE *fp = fdopen_null(net_snmp_open(), "r");
 
 	if (fp) {
 		load_ugly_table(fp);
@@ -274,7 +281,7 @@ static void load_snmp(void)
 
 static void load_snmp6(void)
 {
-	FILE *fp = fdopen(net_snmp6_open(), "r");
+	FILE *fp = fdopen_null(net_snmp6_open(), "r");
 
 	if (fp) {
 		load_good_table(fp);
@@ -284,7 +291,7 @@ static void load_snmp6(void)
 
 static void load_netstat(void)
 {
-	FILE *fp = fdopen(net_netstat_open(), "r");
+	FILE *fp = fdopen_null(net_netstat_open(), "r");
 
 	if (fp) {
 		load_ugly_table(fp);
-- 
2.13.1

Re: [iproute PATCH v2 1/7] nstat: Avoid passing negative fd to fdopen()

[Stephen Hemminger]

On Mon, 21 Aug 2017 19:08:07 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Introduce a wrapper which does the sanity checking and returns NULL
> in case fd is invalid.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  misc/nstat.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/misc/nstat.c b/misc/nstat.c
> index 1212b1f2c8128..7cdde75a56e4e 100644
> --- a/misc/nstat.c
> +++ b/misc/nstat.c
> @@ -252,9 +252,16 @@ static void load_ugly_table(FILE *fp)
>  	}
>  }
>  
> +static FILE *fdopen_null(int fd, const char *mode)
> +{
> +	if (fd < 0)
> +		return NULL;
> +	return fdopen(fd, mode);
> +}
> +
>  static void load_sctp_snmp(void)
>  {
> -	FILE *fp = fdopen(net_sctp_snmp_open(), "r");
> +	FILE *fp = fdopen_null(net_sctp_snmp_open(), "r");
>  
>  	if (fp) {
>  		load_good_table(fp);
> @@ -264,7 +271,7 @@ static void load_sctp_snmp(void)
>  
>  static void load_snmp(void)
>  {
> -	FILE *fp = fdopen(net_snmp_open(), "r");
> +	FILE *fp = fdopen_null(net_snmp_open(), "r");
>  
>  	if (fp) {
>  		load_ugly_table(fp);
> @@ -274,7 +281,7 @@ static void load_snmp(void)
>  
>  static void load_snmp6(void)
>  {
> -	FILE *fp = fdopen(net_snmp6_open(), "r");
> +	FILE *fp = fdopen_null(net_snmp6_open(), "r");
>  
>  	if (fp) {
>  		load_good_table(fp);
> @@ -284,7 +291,7 @@ static void load_snmp6(void)
>  
>  static void load_netstat(void)
>  {
> -	FILE *fp = fdopen(net_netstat_open(), "r");
> +	FILE *fp = fdopen_null(net_netstat_open(), "r");
>  
>  	if (fp) {
>  		load_ugly_table(fp);

Why not just fix it at the source of the open.
I.e 
static FILE *generic_proc_open(condt char * env, const char *name)
{
...
	return fopen(p, "r");
}

Re: [iproute PATCH v2 1/7] nstat: Avoid passing negative fd to fdopen()

[Phil Sutter]

On Mon, Aug 21, 2017 at 05:23:23PM -0700, Stephen Hemminger wrote:
> On Mon, 21 Aug 2017 19:08:07 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > Introduce a wrapper which does the sanity checking and returns NULL
> > in case fd is invalid.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  misc/nstat.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/misc/nstat.c b/misc/nstat.c
> > index 1212b1f2c8128..7cdde75a56e4e 100644
> > --- a/misc/nstat.c
> > +++ b/misc/nstat.c
> > @@ -252,9 +252,16 @@ static void load_ugly_table(FILE *fp)
> >  	}
> >  }
> >  
> > +static FILE *fdopen_null(int fd, const char *mode)
> > +{
> > +	if (fd < 0)
> > +		return NULL;
> > +	return fdopen(fd, mode);
> > +}
> > +
> >  static void load_sctp_snmp(void)
> >  {
> > -	FILE *fp = fdopen(net_sctp_snmp_open(), "r");
> > +	FILE *fp = fdopen_null(net_sctp_snmp_open(), "r");
> >  
> >  	if (fp) {
> >  		load_good_table(fp);
[...]
> 
> Why not just fix it at the source of the open.
> I.e 
> static FILE *generic_proc_open(condt char * env, const char *name)
> {
> ...
> 	return fopen(p, "r");
> }

What should it do? All the load_*() functions are supposed to be
fault-tolerant, so calling abort() or something is not an option.

Thanks, Phil

[iproute PATCH v2 2/7] ss: Make sure index variable is >= 0

[Phil Sutter]

This shouldn't happen but relying upon external data without checking
may lead to unexpected results.

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

diff --git a/misc/ss.c b/misc/ss.c
index 10360e5a04ff8..1ee02d73b2d7f 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2004,7 +2004,7 @@ static void tcp_timer_print(struct tcpstat *s)
 		"unknown"
 	};
 
-	if (s->timer) {
+	if (s->timer >= 0) {
 		if (s->timer > 4)
 			s->timer = 5;
 		printf(" timer:(%s,%s,%d)",
-- 
2.13.1

Re: [iproute PATCH v2 2/7] ss: Make sure index variable is >= 0

[Stephen Hemminger]

On Mon, 21 Aug 2017 19:08:08 +0200
Phil Sutter <phil@nwl.cc> wrote:

> This shouldn't happen but relying upon external data without checking
> may lead to unexpected results.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  misc/ss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/misc/ss.c b/misc/ss.c
> index 10360e5a04ff8..1ee02d73b2d7f 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -2004,7 +2004,7 @@ static void tcp_timer_print(struct tcpstat *s)
>  		"unknown"
>  	};
>  
> -	if (s->timer) {
> +	if (s->timer >= 0) {
>  		if (s->timer > 4)
>  			s->timer = 5;
>  		printf(" timer:(%s,%s,%d)",

Let's go one step deeper on this.
Why is s->timer an int, it should be unsigned. In which case the code in
print would not have to change.

[iproute PATCH v2 3/7] ss: Make sure scanned index value to unix_state_map is sane

[Phil Sutter]

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

diff --git a/misc/ss.c b/misc/ss.c
index 1ee02d73b2d7f..6c091a694231e 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -3151,7 +3151,8 @@ static int unix_show(struct filter *f)
 
 		if (flags & (1 << 16)) {
 			u->state = SS_LISTEN;
-		} else {
+		} else if (u->state > 0 &&
+			   u->state <= ARRAY_SIZE(unix_state_map)) {
 			u->state = unix_state_map[u->state-1];
 			if (u->type == SOCK_DGRAM && u->state == SS_CLOSE && u->rport)
 				u->state = SS_ESTABLISHED;
-- 
2.13.1

[iproute PATCH v2 4/7] netem/maketable: Check return value of fscanf()

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 netem/maketable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/netem/maketable.c b/netem/maketable.c
index 6aff927be7040..517f1dc461e8a 100644
--- a/netem/maketable.c
+++ b/netem/maketable.c
@@ -38,8 +38,8 @@ readdoubles(FILE *fp, int *number)
 	}
 
 	for (i=0; i<limit; ++i){
-		fscanf(fp, "%lf", &x[i]);
-		if (feof(fp))
+		if (fscanf(fp, "%lf", &x[i]) != 1 ||
+		    feof(fp))
 			break;
 		++n;
 	}
-- 
2.13.1

[iproute PATCH v2 5/7] lib/bpf: Check return value of write()

[Phil Sutter]

This is merely to silence the compiler warning. If write to stderr
failed, assume that printing an error message will fail as well so don't
even try.

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

diff --git a/lib/bpf.c b/lib/bpf.c
index 4f52ad4a8f023..be42348b3cc37 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -588,7 +588,8 @@ int bpf_trace_pipe(void)
 
 		ret = read(fd, buff, sizeof(buff) - 1);
 		if (ret > 0) {
-			write(2, buff, ret);
+			if (write(STDERR_FILENO, buff, ret) != ret)
+				return -1;
 			fflush(stderr);
 		}
 	}
-- 
2.13.1

[iproute PATCH v2 6/7] lib/fs: Fix and simplify make_path()

[Phil Sutter]

Calling stat() before mkdir() is racey: The entry might change in
between. Also, the call to stat() seems to exist only to check if the
directory exists already. So simply call mkdir() unconditionally and
catch only errors other than EEXIST.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/fs.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/lib/fs.c b/lib/fs.c
index c59ac564581d0..7083bf2e23bb7 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -102,7 +102,6 @@ out:
 int make_path(const char *path, mode_t mode)
 {
 	char *dir, *delim;
-	struct stat sbuf;
 	int rc = -1;
 
 	delim = dir = strdup(path);
@@ -120,20 +119,11 @@ int make_path(const char *path, mode_t mode)
 		if (delim)
 			*delim = '\0';
 
-		if (stat(dir, &sbuf) != 0) {
-			if (errno != ENOENT) {
-				fprintf(stderr,
-					"stat failed for %s: %s\n",
-					dir, strerror(errno));
-				goto out;
-			}
-
-			if (mkdir(dir, mode) != 0) {
-				fprintf(stderr,
-					"mkdir failed for %s: %s\n",
-					dir, strerror(errno));
-				goto out;
-			}
+		rc = mkdir(dir, mode);
+		if (mkdir(dir, mode) != 0 && errno != EEXIST) {
+			fprintf(stderr, "mkdir failed for %s: %s\n",
+				dir, strerror(errno));
+			goto out;
 		}
 
 		if (delim == NULL)
-- 
2.13.1

[iproute PATCH v2 7/7] lib/libnetlink: Don't pass NULL parameter to memcpy()

[Phil Sutter]

Both addattr_l() and rta_addattr_l() may be called with NULL data
pointer and 0 alen parameters. Avoid calling memcpy() in that case.

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

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 874e660be7eb4..fbe719ee10449 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -871,7 +871,8 @@ int addattr_l(struct nlmsghdr *n, int maxlen, int type, const void *data,
 	rta = NLMSG_TAIL(n);
 	rta->rta_type = type;
 	rta->rta_len = len;
-	memcpy(RTA_DATA(rta), data, alen);
+	if (alen)
+		memcpy(RTA_DATA(rta), data, alen);
 	n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(len);
 	return 0;
 }
@@ -958,7 +959,8 @@ int rta_addattr_l(struct rtattr *rta, int maxlen, int type,
 	subrta = (struct rtattr *)(((char *)rta) + RTA_ALIGN(rta->rta_len));
 	subrta->rta_type = type;
 	subrta->rta_len = len;
-	memcpy(RTA_DATA(subrta), data, alen);
+	if (alen)
+		memcpy(RTA_DATA(subrta), data, alen);
 	rta->rta_len = NLMSG_ALIGN(rta->rta_len) + RTA_ALIGN(len);
 	return 0;
 }
-- 
2.13.1