lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sun, 26 Jun 2016 10:59:11 +0200
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Ivan Vecera <ivecera@...hat.com>, netdev@...r.kernel.org
Cc:	bwh@...nel.org
Subject: Re: [PATCH ethtool] ethtool.c: fix memory leaks

On Fri, 2016-03-18 at 13:24 +0100, Ivan Vecera wrote:
> Memory allocated at several places is not appropriately freed.

Given that ethtool is not a library or a long-running application - why
does that matter?

Ben.

> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
> ---
>  ethtool.c | 60 +++++++++++++++++++++++++++++++++++++++++++++------
> ---------
>  1 file changed, 45 insertions(+), 15 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 0cd0d4f..ca0bf28 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -2065,10 +2065,14 @@ 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;
>  }
>  
> @@ -2078,11 +2082,11 @@ static int do_sfeatures(struct cmd_context
> *ctx)
>  	int any_changed = 0, any_mismatch = 0;
>  	u32 off_flags_wanted = 0;
>  	u32 off_flags_mask = 0;
> -	struct ethtool_sfeatures *efeatures;
> +	struct ethtool_sfeatures *efeatures = NULL;
>  	struct cmdline_info *cmdline_features;
> -	struct feature_state *old_state, *new_state;
> +	struct feature_state *old_state = NULL, *new_state = NULL;
>  	struct ethtool_value eval;
> -	int err;
> +	int err, retval = 1;
>  	int i, j;
>  
>  	defs = get_feature_defs(ctx);
> @@ -2096,7 +2100,7 @@ static int do_sfeatures(struct cmd_context
> *ctx)
>  				   sizeof(efeatures->features[0]));
>  		if (!efeatures) {
>  			perror("Cannot parse arguments");
> -			return 1;
> +			goto finish;
>  		}
>  		efeatures->cmd = ETHTOOL_SFEATURES;
>  		efeatures->size = FEATURE_BITS_TO_BLOCKS(defs-
> >n_features);
> @@ -2114,7 +2118,7 @@ static int do_sfeatures(struct cmd_context
> *ctx)
>  				  sizeof(cmdline_features[0]));
>  	if (!cmdline_features) {
>  		perror("Cannot parse arguments");
> -		return 1;
> +		goto finish;
>  	}
>  	for (i = 0; i < ARRAY_SIZE(off_flag_def); i++)
>  		flag_to_cmdline_info(off_flag_def[i].short_name,
> @@ -2133,12 +2137,13 @@ static int do_sfeatures(struct cmd_context
> *ctx)
>  
>  	if (!any_changed) {
>  		fprintf(stdout, "no features changed\n");
> -		return 0;
> +		retval = 0;
> +		goto finish;
>  	}
>  
>  	old_state = get_features(ctx, defs);
>  	if (!old_state)
> -		return 1;
> +		goto finish;
>  
>  	if (efeatures) {
>  		/* For each offload that the user specified, update
> any
> @@ -2182,7 +2187,7 @@ static int do_sfeatures(struct cmd_context
> *ctx)
>  		err = send_ioctl(ctx, efeatures);
>  		if (err < 0) {
>  			perror("Cannot set device feature
> settings");
> -			return 1;
> +			goto finish;
>  		}
>  	} else {
>  		for (i = 0; i < ARRAY_SIZE(off_flag_def); i++) {
> @@ -2197,7 +2202,7 @@ 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;
> +					goto finish;
>  				}
>  			}
>  		}
> @@ -2211,7 +2216,8 @@ static int do_sfeatures(struct cmd_context
> *ctx)
>  			err = send_ioctl(ctx, &eval);
>  			if (err) {
>  				perror("Cannot set device flag
> settings");
> -				return 92;
> +				retval = 92;
> +				goto finish;
>  			}
>  		}
>  	}
> @@ -2219,7 +2225,7 @@ static int do_sfeatures(struct cmd_context
> *ctx)
>  	/* Compare new state with requested state */
>  	new_state = get_features(ctx, defs);
>  	if (!new_state)
> -		return 1;
> +		goto finish;
>  	any_changed = new_state->off_flags != old_state->off_flags;
>  	any_mismatch = (new_state->off_flags !=
>  			((old_state->off_flags & ~off_flags_mask) |
> @@ -2238,13 +2244,19 @@ static int do_sfeatures(struct cmd_context
> *ctx)
>  		if (!any_changed) {
>  			fprintf(stderr,
>  				"Could not change any device
> features\n");
> -			return 1;
> +			goto finish;
>  		}
>  		printf("Actual changes:\n");
>  		dump_features(defs, new_state, old_state);
>  	}
>  
> -	return 0;
> +	retval = 0;
> +finish:
> +	free(new_state);
> +	free(old_state);
> +	free(efeatures);
> +	free(defs);
> +	return retval;
>  }
>  
>  static int do_gset(struct cmd_context *ctx)
> @@ -2705,8 +2717,18 @@ static int do_gregs(struct cmd_context *ctx)
>  			return 75;
>  		}
>  
> -		regs = realloc(regs, sizeof(*regs) + st.st_size);
> -		regs->len = st.st_size;
> +		if (regs->len != st.st_size) {
> +			struct ethtool_regs *new_regs;
> +			new_regs = realloc(regs, sizeof(*regs) +
> st.st_size);
> +			if (!new_regs) {
> +				perror("Cannot allocate memory for
> register "
> +				       "dump");
> +				free(regs);
> +				return 73;
> +			}
> +			regs = new_regs;
> +			regs->len = st.st_size;
> +		}
>  		nread = fread(regs->data, regs->len, 1, f);
>  		fclose(f);
>  		if (nread != 1) {
> @@ -3775,6 +3797,7 @@ static int do_gprivflags(struct cmd_context
> *ctx)
>  	}
>  	if (strings->len == 0) {
>  		fprintf(stderr, "No private flags defined\n");
> +		free(strings);
>  		return 1;
>  	}
>  	if (strings->len > 32) {
> @@ -3786,6 +3809,7 @@ static int do_gprivflags(struct cmd_context
> *ctx)
>  	flags.cmd = ETHTOOL_GPFLAGS;
>  	if (send_ioctl(ctx, &flags)) {
>  		perror("Cannot get private flags");
> +		free(strings);
>  		return 1;
>  	}
>  
> @@ -3804,6 +3828,7 @@ static int do_gprivflags(struct cmd_context
> *ctx)
>  		       (const char *)strings->data + i *
> ETH_GSTRING_LEN,
>  		       (flags.data & (1U << i)) ? "on" : "off");
>  
> +	free(strings);
>  	return 0;
>  }
>  
> @@ -3825,6 +3850,7 @@ static int do_sprivflags(struct cmd_context
> *ctx)
>  	}
>  	if (strings->len == 0) {
>  		fprintf(stderr, "No private flags defined\n");
> +		free(strings);
>  		return 1;
>  	}
>  	if (strings->len > 32) {
> @@ -3836,6 +3862,7 @@ static int do_sprivflags(struct cmd_context
> *ctx)
>  	cmdline = calloc(strings->len, sizeof(*cmdline));
>  	if (!cmdline) {
>  		perror("Cannot parse arguments");
> +		free(strings);
>  		return 1;
>  	}
>  	for (i = 0; i < strings->len; i++) {
> @@ -3852,6 +3879,7 @@ static int do_sprivflags(struct cmd_context
> *ctx)
>  	flags.cmd = ETHTOOL_GPFLAGS;
>  	if (send_ioctl(ctx, &flags)) {
>  		perror("Cannot get private flags");
> +		free(strings);
>  		return 1;
>  	}
>  
> @@ -3859,9 +3887,11 @@ static int do_sprivflags(struct cmd_context
> *ctx)
>  	flags.data = (flags.data & ~seen_flags) | wanted_flags;
>  	if (send_ioctl(ctx, &flags)) {
>  		perror("Cannot set private flags");
> +		free(strings);
>  		return 1;
>  	}
>  
> +	free(strings);
>  	return 0;
>  }
>  
-- 

Ben Hutchings
compatible: Gracefully accepts erroneous data from any source

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists