[PATCH 0/7] ethtool: misc fixes for Coverity issues

[PATCH 0/7] ethtool: misc fixes for Coverity issues

[John W. Linville]

I requested a Coverity scan of the current ethtool sources. There
weren't many issues identified, but it was non-zero.

Most/all of the resource leak issues can be explained away as
unimportant due to the fact that ethtool invocations are short-term
processes. Nevertheless, I prefer to have those issues "fixed" both
to avoid them as distractions and just to promote better coding
practices overall.

These are "fresh off the press" -- I'll re-review them again before
applying and pushing them. I appreciate any extra eyeballs that can
review them as well.

John

[PATCH 1/7] ethtool: avoid NULL pointer dereference in do_permaddr

[John W. Linville]

Coverity issue: 1363118
Fixes: fef1c4a19703 ("ethtool: add get permanent address option")

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 ethtool.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index 6287b559c699..aa3ef5ed2f75 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3941,6 +3941,11 @@ static int do_permaddr(struct cmd_context *ctx)
 	struct ethtool_perm_addr *epaddr;
 
 	epaddr = malloc(sizeof(struct ethtool_perm_addr) + MAX_ADDR_LEN);
+	if (!epaddr) {
+		perror("Cannot allocate memory for operation");
+		return 1;
+	}
+
 	epaddr->cmd = ETHTOOL_GPERMADDR;
 	epaddr->size = MAX_ADDR_LEN;
 
-- 
2.7.4

[PATCH 2/7] ethtool: avoid resource leak of strings in do_gprivflags

[John W. Linville]

Coverity issue: 1363119
Fixes: e1ee596326ae ("Add support for querying and setting private flags")

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 ethtool.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index aa3ef5ed2f75..0885a61097ad 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4205,7 +4205,7 @@ static int do_gprivflags(struct cmd_context *ctx)
 	struct ethtool_gstrings *strings;
 	struct ethtool_value flags;
 	unsigned int i;
-	int max_len = 0, cur_len;
+	int max_len = 0, cur_len, rc;
 
 	if (ctx->argc != 0)
 		exit_bad_args();
@@ -4215,11 +4215,13 @@ static int do_gprivflags(struct cmd_context *ctx)
 				1);
 	if (!strings) {
 		perror("Cannot get private flag names");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 	if (strings->len == 0) {
 		fprintf(stderr, "No private flags defined\n");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 	if (strings->len > 32) {
 		/* ETHTOOL_GPFLAGS can only cover 32 flags */
@@ -4230,7 +4232,8 @@ static int do_gprivflags(struct cmd_context *ctx)
 	flags.cmd = ETHTOOL_GPFLAGS;
 	if (send_ioctl(ctx, &flags)) {
 		perror("Cannot get private flags");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 
 	/* Find longest string and align all strings accordingly */
@@ -4248,7 +4251,12 @@ static int do_gprivflags(struct cmd_context *ctx)
 		       (const char *)strings->data + i * ETH_GSTRING_LEN,
 		       (flags.data & (1U << i)) ? "on" : "off");
 
-	return 0;
+	rc = 0;
+
+err:
+	if (strings)
+		free(strings);
+	return rc;
 }
 
 static int do_sprivflags(struct cmd_context *ctx)
-- 
2.7.4

Re: [PATCH 2/7] ethtool: avoid resource leak of strings in do_gprivflags

[Jarod Wilson]

On Fri, Sep 30, 2016 at 03:56:16PM -0400, John W. Linville wrote:
> Coverity issue: 1363119
> Fixes: e1ee596326ae ("Add support for querying and setting private flags")
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
>  ethtool.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index aa3ef5ed2f75..0885a61097ad 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -4205,7 +4205,7 @@ static int do_gprivflags(struct cmd_context *ctx)
>  	struct ethtool_gstrings *strings;
>  	struct ethtool_value flags;
>  	unsigned int i;
> -	int max_len = 0, cur_len;
> +	int max_len = 0, cur_len, rc;
>  
>  	if (ctx->argc != 0)
>  		exit_bad_args();
> @@ -4215,11 +4215,13 @@ static int do_gprivflags(struct cmd_context *ctx)
>  				1);
>  	if (!strings) {
>  		perror("Cannot get private flag names");
> -		return 1;
> +		rc = 1;
> +		goto err;

This goto looks redundant, since all you're doing at err is re-checking if
strings is non-null to free it.

>  	if (strings->len == 0) {
>  		fprintf(stderr, "No private flags defined\n");
> -		return 1;
> +		rc = 1;
> +		goto err;
>  	}
>  	if (strings->len > 32) {
>  		/* ETHTOOL_GPFLAGS can only cover 32 flags */
> @@ -4230,7 +4232,8 @@ static int do_gprivflags(struct cmd_context *ctx)
>  	flags.cmd = ETHTOOL_GPFLAGS;
>  	if (send_ioctl(ctx, &flags)) {
>  		perror("Cannot get private flags");
> -		return 1;
> +		rc = 1;
> +		goto err;
>  	}
>  
>  	/* Find longest string and align all strings accordingly */
> @@ -4248,7 +4251,12 @@ static int do_gprivflags(struct cmd_context *ctx)
>  		       (const char *)strings->data + i * ETH_GSTRING_LEN,
>  		       (flags.data & (1U << i)) ? "on" : "off");
>  
> -	return 0;
> +	rc = 0;
> +
> +err:
> +	if (strings)
> +		free(strings);
> +	return rc;
>  }
>  
>  static int do_sprivflags(struct cmd_context *ctx)
> -- 
> 2.7.4
> 

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 2/7] ethtool: avoid resource leak of strings in do_gprivflags

[John W. Linville]

On Mon, Oct 03, 2016 at 01:54:26PM -0400, Jarod Wilson wrote:
> On Fri, Sep 30, 2016 at 03:56:16PM -0400, John W. Linville wrote:
> > Coverity issue: 1363119
> > Fixes: e1ee596326ae ("Add support for querying and setting private flags")
> > 
> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> > ---
> >  ethtool.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/ethtool.c b/ethtool.c
> > index aa3ef5ed2f75..0885a61097ad 100644
> > --- a/ethtool.c
> > +++ b/ethtool.c
> > @@ -4205,7 +4205,7 @@ static int do_gprivflags(struct cmd_context *ctx)
> >  	struct ethtool_gstrings *strings;
> >  	struct ethtool_value flags;
> >  	unsigned int i;
> > -	int max_len = 0, cur_len;
> > +	int max_len = 0, cur_len, rc;
> >  
> >  	if (ctx->argc != 0)
> >  		exit_bad_args();
> > @@ -4215,11 +4215,13 @@ static int do_gprivflags(struct cmd_context *ctx)
> >  				1);
> >  	if (!strings) {
> >  		perror("Cannot get private flag names");
> > -		return 1;
> > +		rc = 1;
> > +		goto err;
> 
> This goto looks redundant, since all you're doing at err is re-checking if
> strings is non-null to free it.

True, the original return is just as good there. And since strings
is non-NULL for everything after that, the NULL check at error can
be eliminated as well...

Thanks!

John

> >  	if (strings->len == 0) {
> >  		fprintf(stderr, "No private flags defined\n");
> > -		return 1;
> > +		rc = 1;
> > +		goto err;
> >  	}
> >  	if (strings->len > 32) {
> >  		/* ETHTOOL_GPFLAGS can only cover 32 flags */
> > @@ -4230,7 +4232,8 @@ static int do_gprivflags(struct cmd_context *ctx)
> >  	flags.cmd = ETHTOOL_GPFLAGS;
> >  	if (send_ioctl(ctx, &flags)) {
> >  		perror("Cannot get private flags");
> > -		return 1;
> > +		rc = 1;
> > +		goto err;
> >  	}
> >  
> >  	/* Find longest string and align all strings accordingly */
> > @@ -4248,7 +4251,12 @@ static int do_gprivflags(struct cmd_context *ctx)
> >  		       (const char *)strings->data + i * ETH_GSTRING_LEN,
> >  		       (flags.data & (1U << i)) ? "on" : "off");
> >  
> > -	return 0;
> > +	rc = 0;
> > +
> > +err:
> > +	if (strings)
> > +		free(strings);
> > +	return rc;
> >  }
> >  
> >  static int do_sprivflags(struct cmd_context *ctx)
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Jarod Wilson
> jarod@redhat.com
> 
> 
> 

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH v2 2/7] ethtool: avoid resource leak of strings in do_gprivflags

[John W. Linville]

Coverity issue: 1363119
Fixes: e1ee596326ae ("Add support for querying and setting private flags")

Signed-off-by: John W. Linville <linville@tuxdriver.com>
Reviewed-by: Greg Rose <grose@lightfleet.com>
---
 ethtool.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index aa3ef5ed2f75..7ce5667fcdd7 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4205,7 +4205,7 @@ static int do_gprivflags(struct cmd_context *ctx)
 	struct ethtool_gstrings *strings;
 	struct ethtool_value flags;
 	unsigned int i;
-	int max_len = 0, cur_len;
+	int max_len = 0, cur_len, rc;
 
 	if (ctx->argc != 0)
 		exit_bad_args();
@@ -4219,7 +4219,8 @@ static int do_gprivflags(struct cmd_context *ctx)
 	}
 	if (strings->len == 0) {
 		fprintf(stderr, "No private flags defined\n");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 	if (strings->len > 32) {
 		/* ETHTOOL_GPFLAGS can only cover 32 flags */
@@ -4230,7 +4231,8 @@ static int do_gprivflags(struct cmd_context *ctx)
 	flags.cmd = ETHTOOL_GPFLAGS;
 	if (send_ioctl(ctx, &flags)) {
 		perror("Cannot get private flags");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 
 	/* Find longest string and align all strings accordingly */
@@ -4248,7 +4250,11 @@ static int do_gprivflags(struct cmd_context *ctx)
 		       (const char *)strings->data + i * ETH_GSTRING_LEN,
 		       (flags.data & (1U << i)) ? "on" : "off");
 
-	return 0;
+	rc = 0;
+
+err:
+	free(strings);
+	return rc;
 }
 
 static int do_sprivflags(struct cmd_context *ctx)
-- 
2.7.4

[PATCH 3/7] ethtool: plug resource leaks of defs and features in do_gfeatures

[John W. Linville]

Coverity issues: 1363120, 1363121
Fixes: 6042804cf6ec ("Change -k/-K options to use ETHTOOL_{G,S}FEATURES")

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 ethtool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index 0885a61097ad..09486615a5bb 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2233,10 +2233,13 @@ static int do_gfeatures(struct cmd_context *ctx)
 	features = get_features(ctx, defs);
 	if (!features) {
 		fprintf(stdout, "no feature info available\n");
+		free(defs);
 		return 1;
 	}
 
 	dump_features(defs, features, NULL);
+	free(features);
+	free(defs);
 	return 0;
 }
 
-- 
2.7.4

[PATCH 4/7] ethtool: fix leakage of defs resources in do_sfeatures

[John W. Linville]

Coverity issue: 1363123
Fixes: 6042804cf6ecc ("Change -k/-K options to use ETHTOOL_{G,S}FEATURES")

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 ethtool.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 09486615a5bb..2fd3393ca5f6 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2253,7 +2253,7 @@ static int do_sfeatures(struct cmd_context *ctx)
 	struct cmdline_info *cmdline_features;
 	struct feature_state *old_state, *new_state;
 	struct ethtool_value eval;
-	int err;
+	int err, rc;
 	int i, j;
 
 	defs = get_feature_defs(ctx);
@@ -2267,7 +2267,8 @@ static int do_sfeatures(struct cmd_context *ctx)
 				   sizeof(efeatures->features[0]));
 		if (!efeatures) {
 			perror("Cannot parse arguments");
-			return 1;
+			rc = 1;
+			goto err;
 		}
 		efeatures->cmd = ETHTOOL_SFEATURES;
 		efeatures->size = FEATURE_BITS_TO_BLOCKS(defs->n_features);
@@ -2285,7 +2286,8 @@ static int do_sfeatures(struct cmd_context *ctx)
 				  sizeof(cmdline_features[0]));
 	if (!cmdline_features) {
 		perror("Cannot parse arguments");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 	for (i = 0; i < ARRAY_SIZE(off_flag_def); i++)
 		flag_to_cmdline_info(off_flag_def[i].short_name,
@@ -2304,12 +2306,15 @@ static int do_sfeatures(struct cmd_context *ctx)
 
 	if (!any_changed) {
 		fprintf(stdout, "no features changed\n");
-		return 0;
+		rc = 0;
+		goto err;
 	}
 
 	old_state = get_features(ctx, defs);
-	if (!old_state)
-		return 1;
+	if (!old_state) {
+		rc = 1;
+		goto err;
+	}
 
 	if (efeatures) {
 		/* For each offload that the user specified, update any
@@ -2353,7 +2358,8 @@ static int do_sfeatures(struct cmd_context *ctx)
 		err = send_ioctl(ctx, efeatures);
 		if (err < 0) {
 			perror("Cannot set device feature settings");
-			return 1;
+			rc = 1;
+			goto err;
 		}
 	} else {
 		for (i = 0; i < ARRAY_SIZE(off_flag_def); i++) {
@@ -2368,7 +2374,8 @@ static int do_sfeatures(struct cmd_context *ctx)
 					fprintf(stderr,
 						"Cannot set device %s settings: %m\n",
 						off_flag_def[i].long_name);
-					return 1;
+					rc = 1;
+					goto err;
 				}
 			}
 		}
@@ -2382,15 +2389,18 @@ static int do_sfeatures(struct cmd_context *ctx)
 			err = send_ioctl(ctx, &eval);
 			if (err) {
 				perror("Cannot set device flag settings");
-				return 92;
+				rc = 92;
+				goto err;
 			}
 		}
 	}
 
 	/* Compare new state with requested state */
 	new_state = get_features(ctx, defs);
-	if (!new_state)
-		return 1;
+	if (!new_state) {
+		rc = 1;
+		goto err;
+	}
 	any_changed = new_state->off_flags != old_state->off_flags;
 	any_mismatch = (new_state->off_flags !=
 			((old_state->off_flags & ~off_flags_mask) |
@@ -2409,13 +2419,18 @@ static int do_sfeatures(struct cmd_context *ctx)
 		if (!any_changed) {
 			fprintf(stderr,
 				"Could not change any device features\n");
-			return 1;
+			rc = 1;
+			goto err;
 		}
 		printf("Actual changes:\n");
 		dump_features(defs, new_state, old_state);
 	}
 
-	return 0;
+	rc = 0;
+
+err:
+	free(defs);
+	return rc;
 }
 
 static struct ethtool_link_usettings *
-- 
2.7.4

[PATCH 5/7] ethtool: fix leakage of efeatures resources in do_sfeatures

[John W. Linville]

Coverity issue: 1363122
Fixes: 6042804cf6ecc ("Change -k/-K options to use ETHTOOL_{G,S}FEATURES")

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 ethtool.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index 2fd3393ca5f6..840a5da157a9 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2430,6 +2430,8 @@ static int do_sfeatures(struct cmd_context *ctx)
 
 err:
 	free(defs);
+	if (efeatures)
+		free(efeatures);
 	return rc;
 }
 
-- 
2.7.4

[PATCH 6/7] ethtool: fix leakage of strings resources in do_sprivflags

[John W. Linville]

Coverity issue: 1363124
Fixes: e1ee596326ae ("Add support for querying and setting private flags")

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 ethtool.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 840a5da157a9..406cfd2f95a5 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4285,7 +4285,7 @@ static int do_sprivflags(struct cmd_context *ctx)
 	struct cmdline_info *cmdline;
 	struct ethtool_value flags;
 	u32 wanted_flags = 0, seen_flags = 0;
-	int any_changed;
+	int any_changed, rc;
 	unsigned int i;
 
 	strings = get_stringset(ctx, ETH_SS_PRIV_FLAGS,
@@ -4297,7 +4297,8 @@ static int do_sprivflags(struct cmd_context *ctx)
 	}
 	if (strings->len == 0) {
 		fprintf(stderr, "No private flags defined\n");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 	if (strings->len > 32) {
 		/* ETHTOOL_{G,S}PFLAGS can only cover 32 flags */
@@ -4308,7 +4309,8 @@ static int do_sprivflags(struct cmd_context *ctx)
 	cmdline = calloc(strings->len, sizeof(*cmdline));
 	if (!cmdline) {
 		perror("Cannot parse arguments");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 	for (i = 0; i < strings->len; i++) {
 		cmdline[i].name = ((const char *)strings->data +
@@ -4324,17 +4326,22 @@ static int do_sprivflags(struct cmd_context *ctx)
 	flags.cmd = ETHTOOL_GPFLAGS;
 	if (send_ioctl(ctx, &flags)) {
 		perror("Cannot get private flags");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 
 	flags.cmd = ETHTOOL_SPFLAGS;
 	flags.data = (flags.data & ~seen_flags) | wanted_flags;
 	if (send_ioctl(ctx, &flags)) {
 		perror("Cannot set private flags");
-		return 1;
+		rc = 1;
+		goto err;
 	}
 
-	return 0;
+	rc = 0;
+err:
+	free(strings);
+	return rc;
 }
 
 static int do_tsinfo(struct cmd_context *ctx)
-- 
2.7.4

[PATCH 7/7] ethtool: fix leakage of strings resources in get_feature_defs

[John W. Linville]

Coverity issue: 1363125
Fixes: 6042804cf6ec ("Change -k/-K options to use ETHTOOL_{G,S}FEATURES")

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 ethtool.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ethtool.c b/ethtool.c
index 406cfd2f95a5..5b83a0041a95 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1715,8 +1715,10 @@ static struct feature_defs *get_feature_defs(struct cmd_context *ctx)
 	}
 
 	defs = malloc(sizeof(*defs) + sizeof(defs->def[0]) * n_features);
-	if (!defs)
+	if (!defs) {
+		free(names);
 		return NULL;
+	}
 
 	defs->n_features = n_features;
 	memset(defs->off_flag_matched, 0, sizeof(defs->off_flag_matched));
-- 
2.7.4

Re: [PATCH 0/7] ethtool: misc fixes for Coverity issues

[Greg]

On Fri, 2016-09-30 at 15:56 -0400, John W. Linville wrote:
> I requested a Coverity scan of the current ethtool sources. There
> weren't many issues identified, but it was non-zero.
> 
> Most/all of the resource leak issues can be explained away as
> unimportant due to the fact that ethtool invocations are short-term
> processes. Nevertheless, I prefer to have those issues "fixed" both
> to avoid them as distractions and just to promote better coding
> practices overall.

The patch series looks good to me.

Reviewed-by: Greg Rose <grose@lightfleet.com>

> 
> These are "fresh off the press" -- I'll re-review them again before
> applying and pushing them. I appreciate any extra eyeballs that can
> review them as well.
> 
> John
> --
> John W. Linville		Someday the world will need a hero, and you
> linville@tuxdriver.com			might be all we have.  Be ready.
>