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 <ivec...@redhat.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
signature.asc
Description: This is a digitally signed message part
