* [PATCH] tools/iio/iio_utils:fix memory leak @ 2023-01-12 11:01 Yulong Zhang 2023-01-14 16:41 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Yulong Zhang @ 2023-01-12 11:01 UTC (permalink / raw) To: linux-iio; +Cc: jic23, lars, linux-kernel, Yulong Zhang 1.fopen sysfsfp without fclose 2.asprintf filename without free Signed-off-by: Yulong Zhang <yulong.zhang@metoak.net> --- 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); ret = asprintf(&filename, "%s/%s_index", -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/iio/iio_utils:fix memory leak 2023-01-12 11:01 [PATCH] tools/iio/iio_utils:fix memory leak Yulong Zhang @ 2023-01-14 16:41 ` Jonathan Cameron 2023-01-16 3:30 ` [PATCH v2] " Yulong Zhang 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2023-01-14 16:41 UTC (permalink / raw) To: Yulong Zhang; +Cc: linux-iio, lars, linux-kernel On Thu, 12 Jan 2023 19:01:22 +0800 Yulong Zhang <yulong.zhang@metoak.net> wrote: > 1.fopen sysfsfp without fclose > 2.asprintf filename without free > > Signed-off-by: Yulong Zhang <yulong.zhang@metoak.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", ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] tools/iio/iio_utils:fix memory leak 2023-01-14 16:41 ` Jonathan Cameron @ 2023-01-16 3:30 ` Yulong Zhang 2023-01-16 15:16 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Yulong Zhang @ 2023-01-16 3:30 UTC (permalink / raw) To: jic23; +Cc: lars, linux-iio, linux-kernel, yulong.zhang 1. fopen sysfs without fclose. 2. asprintf filename without free. 3. if asprintf return error,do not need to free the buffer. Signed-off-by: Yulong Zhang <yulong.zhang@metoak.net> --- Yes, you are right. Thanks you for the advice. I found another problem, when asprintf return error, still free the buffer.This is not correct,I've removed it. > sysfsfp = fopen(filename, "r"); > if (!sysfsfp) { > ret = -errno; > fprintf(stderr, "failed to open %s\n", > filename); > free(filename); > goto error_cleanup_array; > } > free(filename); Because in if statement still use filename,so I can't free filename before it.And I am not sure it can be removed,so I free filename after if statement. tools/iio/iio_utils.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c index 8d35893b2fa8..abec4331701c 100644 --- a/tools/iio/iio_utils.c +++ b/tools/iio/iio_utils.c @@ -264,6 +264,7 @@ 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: @@ -350,6 +351,7 @@ int build_channel_array(const char *device_dir, int buffer_idx, free(filename); goto error_close_dir; } + free(filename); errno = 0; if (fscanf(sysfsfp, "%i", &ret) != 1) { @@ -357,7 +359,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, if (fclose(sysfsfp)) perror("build_channel_array(): Failed to close file"); - free(filename); goto error_close_dir; } if (ret == 1) @@ -365,11 +366,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, if (fclose(sysfsfp)) { ret = -errno; - free(filename); goto error_close_dir; } - free(filename); } *ci_array = malloc(sizeof(**ci_array) * (*counter)); @@ -395,9 +394,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, } sysfsfp = fopen(filename, "r"); + free(filename); if (!sysfsfp) { ret = -errno; - free(filename); count--; goto error_cleanup_array; } @@ -405,20 +404,17 @@ int build_channel_array(const char *device_dir, int buffer_idx, errno = 0; if (fscanf(sysfsfp, "%i", ¤t_enabled) != 1) { ret = errno ? -errno : -ENODATA; - free(filename); count--; goto error_cleanup_array; } if (fclose(sysfsfp)) { ret = -errno; - free(filename); count--; goto error_cleanup_array; } if (!current_enabled) { - free(filename); count--; continue; } @@ -429,7 +425,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, strlen(ent->d_name) - strlen("_en")); if (!current->name) { - free(filename); ret = -ENOMEM; count--; goto error_cleanup_array; @@ -439,7 +434,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, ret = iioutils_break_up_name(current->name, ¤t->generic_name); if (ret) { - free(filename); free(current->name); count--; goto error_cleanup_array; @@ -450,7 +444,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, scan_el_dir, current->name); if (ret < 0) { - free(filename); ret = -ENOMEM; goto error_cleanup_array; } @@ -463,6 +456,7 @@ int build_channel_array(const char *device_dir, int buffer_idx, free(filename); goto error_cleanup_array; } + free(filename); errno = 0; if (fscanf(sysfsfp, "%u", ¤t->index) != 1) { @@ -470,17 +464,14 @@ int build_channel_array(const char *device_dir, int buffer_idx, if (fclose(sysfsfp)) perror("build_channel_array(): Failed to close file"); - free(filename); goto error_cleanup_array; } if (fclose(sysfsfp)) { ret = -errno; - free(filename); goto error_cleanup_array; } - free(filename); /* Find the scale */ ret = iioutils_get_param_float(¤t->scale, "scale", -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] tools/iio/iio_utils:fix memory leak 2023-01-16 3:30 ` [PATCH v2] " Yulong Zhang @ 2023-01-16 15:16 ` Jonathan Cameron 2023-01-17 2:51 ` [PATCH v3] " Yulong Zhang 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2023-01-16 15:16 UTC (permalink / raw) To: Yulong Zhang; +Cc: jic23, lars, linux-iio, linux-kernel On Mon, 16 Jan 2023 11:30:07 +0800 Yulong Zhang <yulong.zhang@metoak.net> wrote: > 1. fopen sysfs without fclose. > 2. asprintf filename without free. > 3. if asprintf return error,do not need to free the buffer. > > Signed-off-by: Yulong Zhang <yulong.zhang@metoak.net> > --- > Yes, you are right. Thanks you for the advice. > I found another problem, when asprintf return error, still free the > buffer.This is not correct,I've removed it. > > > sysfsfp = fopen(filename, "r"); > > if (!sysfsfp) { > > ret = -errno; > > fprintf(stderr, "failed to open %s\n", > > filename); > > free(filename); > > goto error_cleanup_array; > > } > > free(filename); > Because in if statement still use filename,so I can't free filename > before it.And I am not sure it can be removed,so I free filename after if > statement. Agreed. My suggestion is drop that error print to simplify the code. It isn't particularly important to know the full filename that we failed to open so I think dropping it is a reasonable thing to do. If you really care about the message then fprintf(stderr, "failed to open %s/%s_index\n", scan_el_dir, current->name); will work. > > tools/iio/iio_utils.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c > index 8d35893b2fa8..abec4331701c 100644 > --- a/tools/iio/iio_utils.c > +++ b/tools/iio/iio_utils.c > @@ -264,6 +264,7 @@ 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: > @@ -350,6 +351,7 @@ int build_channel_array(const char *device_dir, int buffer_idx, > free(filename); > goto error_close_dir; > } > + free(filename); > > errno = 0; > if (fscanf(sysfsfp, "%i", &ret) != 1) { > @@ -357,7 +359,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, > if (fclose(sysfsfp)) > perror("build_channel_array(): Failed to close file"); > > - free(filename); > goto error_close_dir; > } > if (ret == 1) > @@ -365,11 +366,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, > > if (fclose(sysfsfp)) { > ret = -errno; > - free(filename); > goto error_close_dir; > } > > - free(filename); > } > > *ci_array = malloc(sizeof(**ci_array) * (*counter)); > @@ -395,9 +394,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, > } > > sysfsfp = fopen(filename, "r"); > + free(filename); > if (!sysfsfp) { > ret = -errno; > - free(filename); > count--; > goto error_cleanup_array; > } > @@ -405,20 +404,17 @@ int build_channel_array(const char *device_dir, int buffer_idx, > errno = 0; > if (fscanf(sysfsfp, "%i", ¤t_enabled) != 1) { > ret = errno ? -errno : -ENODATA; > - free(filename); > count--; > goto error_cleanup_array; > } > > if (fclose(sysfsfp)) { > ret = -errno; > - free(filename); > count--; > goto error_cleanup_array; > } > > if (!current_enabled) { > - free(filename); > count--; > continue; > } > @@ -429,7 +425,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, > strlen(ent->d_name) - > strlen("_en")); > if (!current->name) { > - free(filename); > ret = -ENOMEM; > count--; > goto error_cleanup_array; > @@ -439,7 +434,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, > ret = iioutils_break_up_name(current->name, > ¤t->generic_name); > if (ret) { > - free(filename); > free(current->name); > count--; > goto error_cleanup_array; > @@ -450,7 +444,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, > scan_el_dir, > current->name); > if (ret < 0) { > - free(filename); > ret = -ENOMEM; > goto error_cleanup_array; > } > @@ -463,6 +456,7 @@ int build_channel_array(const char *device_dir, int buffer_idx, > free(filename); > goto error_cleanup_array; > } > + free(filename); > > errno = 0; > if (fscanf(sysfsfp, "%u", ¤t->index) != 1) { > @@ -470,17 +464,14 @@ int build_channel_array(const char *device_dir, int buffer_idx, > if (fclose(sysfsfp)) > perror("build_channel_array(): Failed to close file"); > > - free(filename); > goto error_cleanup_array; > } > > if (fclose(sysfsfp)) { > ret = -errno; > - free(filename); > goto error_cleanup_array; > } > > - free(filename); > /* Find the scale */ > ret = iioutils_get_param_float(¤t->scale, > "scale", ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] tools/iio/iio_utils:fix memory leak 2023-01-16 15:16 ` Jonathan Cameron @ 2023-01-17 2:51 ` Yulong Zhang 2023-01-21 17:53 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Yulong Zhang @ 2023-01-17 2:51 UTC (permalink / raw) To: jonathan.cameron; +Cc: jic23, lars, linux-iio, linux-kernel, yulong.zhang 1. fopen sysfs without fclose. 2. asprintf filename without free. 3. if asprintf return error,do not need to free the buffer. Signed-off-by: Yulong Zhang <yulong.zhang@metoak.net> --- tools/iio/iio_utils.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c index 8d35893b2fa8..6a00a6eecaef 100644 --- a/tools/iio/iio_utils.c +++ b/tools/iio/iio_utils.c @@ -264,6 +264,7 @@ 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: @@ -345,9 +346,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, } sysfsfp = fopen(filename, "r"); + free(filename); if (!sysfsfp) { ret = -errno; - free(filename); goto error_close_dir; } @@ -357,7 +358,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, if (fclose(sysfsfp)) perror("build_channel_array(): Failed to close file"); - free(filename); goto error_close_dir; } if (ret == 1) @@ -365,11 +365,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, if (fclose(sysfsfp)) { ret = -errno; - free(filename); goto error_close_dir; } - free(filename); } *ci_array = malloc(sizeof(**ci_array) * (*counter)); @@ -395,9 +393,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, } sysfsfp = fopen(filename, "r"); + free(filename); if (!sysfsfp) { ret = -errno; - free(filename); count--; goto error_cleanup_array; } @@ -405,20 +403,17 @@ int build_channel_array(const char *device_dir, int buffer_idx, errno = 0; if (fscanf(sysfsfp, "%i", ¤t_enabled) != 1) { ret = errno ? -errno : -ENODATA; - free(filename); count--; goto error_cleanup_array; } if (fclose(sysfsfp)) { ret = -errno; - free(filename); count--; goto error_cleanup_array; } if (!current_enabled) { - free(filename); count--; continue; } @@ -429,7 +424,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, strlen(ent->d_name) - strlen("_en")); if (!current->name) { - free(filename); ret = -ENOMEM; count--; goto error_cleanup_array; @@ -439,7 +433,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, ret = iioutils_break_up_name(current->name, ¤t->generic_name); if (ret) { - free(filename); free(current->name); count--; goto error_cleanup_array; @@ -450,17 +443,16 @@ int build_channel_array(const char *device_dir, int buffer_idx, scan_el_dir, current->name); if (ret < 0) { - free(filename); ret = -ENOMEM; goto error_cleanup_array; } sysfsfp = fopen(filename, "r"); + free(filename); if (!sysfsfp) { ret = -errno; - fprintf(stderr, "failed to open %s\n", - filename); - free(filename); + fprintf(stderr, "failed to open %s/%s_index\n", + scan_el_dir, current->name); goto error_cleanup_array; } @@ -470,17 +462,14 @@ int build_channel_array(const char *device_dir, int buffer_idx, if (fclose(sysfsfp)) perror("build_channel_array(): Failed to close file"); - free(filename); goto error_cleanup_array; } if (fclose(sysfsfp)) { ret = -errno; - free(filename); goto error_cleanup_array; } - free(filename); /* Find the scale */ ret = iioutils_get_param_float(¤t->scale, "scale", -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] tools/iio/iio_utils:fix memory leak 2023-01-17 2:51 ` [PATCH v3] " Yulong Zhang @ 2023-01-21 17:53 ` Jonathan Cameron 0 siblings, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2023-01-21 17:53 UTC (permalink / raw) To: Yulong Zhang; +Cc: jonathan.cameron, lars, linux-iio, linux-kernel On Tue, 17 Jan 2023 10:51:47 +0800 Yulong Zhang <yulong.zhang@metoak.net> wrote: > 1. fopen sysfs without fclose. > 2. asprintf filename without free. > 3. if asprintf return error,do not need to free the buffer. > > Signed-off-by: Yulong Zhang <yulong.zhang@metoak.net> I'm not that worried about rushing in a memory leak fix for the example code, so I've queued this up for the next merge window. Thanks, Jonathan > --- > tools/iio/iio_utils.c | 23 ++++++----------------- > 1 file changed, 6 insertions(+), 17 deletions(-) > > diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c > index 8d35893b2fa8..6a00a6eecaef 100644 > --- a/tools/iio/iio_utils.c > +++ b/tools/iio/iio_utils.c > @@ -264,6 +264,7 @@ 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: > @@ -345,9 +346,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, > } > > sysfsfp = fopen(filename, "r"); > + free(filename); > if (!sysfsfp) { > ret = -errno; > - free(filename); > goto error_close_dir; > } > > @@ -357,7 +358,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, > if (fclose(sysfsfp)) > perror("build_channel_array(): Failed to close file"); > > - free(filename); > goto error_close_dir; > } > if (ret == 1) > @@ -365,11 +365,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, > > if (fclose(sysfsfp)) { > ret = -errno; > - free(filename); > goto error_close_dir; > } > > - free(filename); > } > > *ci_array = malloc(sizeof(**ci_array) * (*counter)); > @@ -395,9 +393,9 @@ int build_channel_array(const char *device_dir, int buffer_idx, > } > > sysfsfp = fopen(filename, "r"); > + free(filename); > if (!sysfsfp) { > ret = -errno; > - free(filename); > count--; > goto error_cleanup_array; > } > @@ -405,20 +403,17 @@ int build_channel_array(const char *device_dir, int buffer_idx, > errno = 0; > if (fscanf(sysfsfp, "%i", ¤t_enabled) != 1) { > ret = errno ? -errno : -ENODATA; > - free(filename); > count--; > goto error_cleanup_array; > } > > if (fclose(sysfsfp)) { > ret = -errno; > - free(filename); > count--; > goto error_cleanup_array; > } > > if (!current_enabled) { > - free(filename); > count--; > continue; > } > @@ -429,7 +424,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, > strlen(ent->d_name) - > strlen("_en")); > if (!current->name) { > - free(filename); > ret = -ENOMEM; > count--; > goto error_cleanup_array; > @@ -439,7 +433,6 @@ int build_channel_array(const char *device_dir, int buffer_idx, > ret = iioutils_break_up_name(current->name, > ¤t->generic_name); > if (ret) { > - free(filename); > free(current->name); > count--; > goto error_cleanup_array; > @@ -450,17 +443,16 @@ int build_channel_array(const char *device_dir, int buffer_idx, > scan_el_dir, > current->name); > if (ret < 0) { > - free(filename); > ret = -ENOMEM; > goto error_cleanup_array; > } > > sysfsfp = fopen(filename, "r"); > + free(filename); > if (!sysfsfp) { > ret = -errno; > - fprintf(stderr, "failed to open %s\n", > - filename); > - free(filename); > + fprintf(stderr, "failed to open %s/%s_index\n", > + scan_el_dir, current->name); > goto error_cleanup_array; > } > > @@ -470,17 +462,14 @@ int build_channel_array(const char *device_dir, int buffer_idx, > if (fclose(sysfsfp)) > perror("build_channel_array(): Failed to close file"); > > - free(filename); > goto error_cleanup_array; > } > > if (fclose(sysfsfp)) { > ret = -errno; > - free(filename); > goto error_cleanup_array; > } > > - free(filename); > /* Find the scale */ > ret = iioutils_get_param_float(¤t->scale, > "scale", ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-21 17:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-12 11:01 [PATCH] tools/iio/iio_utils:fix memory leak Yulong Zhang 2023-01-14 16:41 ` Jonathan Cameron 2023-01-16 3:30 ` [PATCH v2] " Yulong Zhang 2023-01-16 15:16 ` Jonathan Cameron 2023-01-17 2:51 ` [PATCH v3] " Yulong Zhang 2023-01-21 17:53 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox