| Message ID | c7d57346ddc4d9eaaabc0f004911d038c95238af.1643736038.git.aclaudi@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Stephen Hemminger |
| Headers | show |
| Series | some memory leak fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, 1 Feb 2022 18:39:24 +0100 Andrea Claudi <aclaudi@redhat.com> wrote: > + if (fscanf(f, "%ms\n", &comm) != 1) { > + free(comm); > + return NULL; This is still leaking the original comm. Why not change it to use a local variable for the path (avoid asprintf) and not reuse comm for both pathname and return value.
On Tue, Feb 01, 2022 at 10:27:12AM -0800, Stephen Hemminger wrote: > On Tue, 1 Feb 2022 18:39:24 +0100 > Andrea Claudi <aclaudi@redhat.com> wrote: > > > + if (fscanf(f, "%ms\n", &comm) != 1) { > > + free(comm); > > + return NULL; > > This is still leaking the original comm. > Thanks Stephen, I missed the %m over there :( > Why not change it to use a local variable for the path > (avoid asprintf) and not reuse comm for both pathname > and return value. > Thanks for the suggestion. What about taking an extra-step and get rid of the %m too? We can do something similar to the get_command_name() function so that we don't need to use free in places where we use get_task_name().
On Wed, 2 Feb 2022 15:38:16 +0100 Andrea Claudi <aclaudi@redhat.com> wrote: > On Tue, Feb 01, 2022 at 10:27:12AM -0800, Stephen Hemminger wrote: > > On Tue, 1 Feb 2022 18:39:24 +0100 > > Andrea Claudi <aclaudi@redhat.com> wrote: > > > > > + if (fscanf(f, "%ms\n", &comm) != 1) { > > > + free(comm); > > > + return NULL; > > > > This is still leaking the original comm. > > > > Thanks Stephen, I missed the %m over there :( > > > Why not change it to use a local variable for the path > > (avoid asprintf) and not reuse comm for both pathname > > and return value. > > > > Thanks for the suggestion. > > What about taking an extra-step and get rid of the %m too? > We can do something similar to the get_command_name() function so that > we don't need to use free in places where we use get_task_name(). What ever works best.
diff --git a/lib/fs.c b/lib/fs.c index f6f5f8a0..5692e2d3 100644 --- a/lib/fs.c +++ b/lib/fs.c @@ -354,11 +354,15 @@ char *get_task_name(pid_t pid) return NULL; f = fopen(comm, "r"); - if (!f) + if (!f) { + free(comm); return NULL; + } - if (fscanf(f, "%ms\n", &comm) != 1) - comm = NULL; + if (fscanf(f, "%ms\n", &comm) != 1) { + free(comm); + return NULL; + } fclose(f);
asprintf() allocates memory which is not freed on the error path of get_task_name(), thus potentially leading to memory leaks. This commit fixes this using free() on error paths. Fixes: 81bfd01a4c9e ("lib: move get_task_name() from rdma") Signed-off-by: Andrea Claudi <aclaudi@redhat.com> --- lib/fs.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)