linux-kernel - Re: [PATCH] tools/iio/iio

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:   Sat, 14 Jan 2023 16:41:14 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Yulong Zhang <yulong.zhang@...oak.net>
Cc:     linux-iio@...r.kernel.org, lars@...afoo.de,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tools/iio/iio_utils:fix memory leak

On Thu, 12 Jan 2023 19:01:22 +0800
Yulong Zhang <yulong.zhang@...oak.net> wrote:

> 1.fopen sysfsfp without fclose
> 2.asprintf filename without free
> 
> Signed-off-by: Yulong Zhang <yulong.zhang@...oak.net>
Good spot.  One suggestion below on how to tidy up the code
whilst fixing this.

> ---
>  tools/iio/iio_utils.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> index 8d35893b2..38e9352e5 100644
> --- a/tools/iio/iio_utils.c
> +++ b/tools/iio/iio_utils.c
> @@ -264,6 +264,8 @@ int iioutils_get_param_float(float *output, const char *param_name,
>  			if (fscanf(sysfsfp, "%f", output) != 1)
>  				ret = errno ? -errno : -ENODATA;
>  
> +			fclose(sysfsfp);
> +
>  			break;
>  		}
>  error_free_filename:
> @@ -444,6 +446,7 @@ int build_channel_array(const char *device_dir, int buffer_idx,
>  				count--;
>  				goto error_cleanup_array;
>  			}
> +			free(filename);

Instead of doing it here, why not move the free as early as possible (and hence
drop the need to do it in most of the error paths.
			sysfsfp = fopen(filename, "r");
		+	free(filename);
			if (!sysfsfp) {
				ret = -errno;
		-		free(filename)
... and the other cases that follow.

The same could be done for the case below, at the minor cost of not
having the filename for the print statement (which I think would be fine).

>  
>  			ret = asprintf(&filename,
>  				       "%s/%s_index",