| Message ID | 0731f9e5b5ce95ab2da44ac74aa1f79ead9413bf.1646223467.git.aclaudi@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | fix memory leak in get_task_name() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Mar 02, 2022 at 01:28:47PM +0100, Andrea Claudi wrote: > asprintf() allocates memory which is not freed on the error path of > get_task_name(), thus potentially leading to memory leaks. Not really, memory is released when the application exits, which is the case here. > > Rework get_task_name() and avoid asprintf() usage and memory allocation, > returning the task string to the caller using an additional char* > parameter. > > Fixes: 81bfd01a4c9e ("lib: move get_task_name() from rdma") As I was told before, in netdev Fixes means that it is a bug that affects users. This is not the case here. > Signed-off-by: Andrea Claudi <aclaudi@redhat.com> > --- > include/utils.h | 2 +- > ip/iptuntap.c | 17 ++++++++++------- > lib/fs.c | 20 ++++++++++---------- > rdma/res-cmid.c | 8 +++++--- > rdma/res-cq.c | 8 +++++--- > rdma/res-ctx.c | 7 ++++--- > rdma/res-mr.c | 7 ++++--- > rdma/res-pd.c | 8 +++++--- > rdma/res-qp.c | 7 ++++--- > rdma/res-srq.c | 7 ++++--- > rdma/stat.c | 5 ++++- > 11 files changed, 56 insertions(+), 40 deletions(-) Honestly, I don't see any need in both patches. Thanks > > diff --git a/include/utils.h b/include/utils.h > index b6c468e9..81294488 100644 > --- a/include/utils.h > +++ b/include/utils.h > @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount); > __u64 get_cgroup2_id(const char *path); > char *get_cgroup2_path(__u64 id, bool full); > int get_command_name(const char *pid, char *comm, size_t len); > -char *get_task_name(pid_t pid); > +int get_task_name(pid_t pid, char *name); > > int get_rtnl_link_stats_rta(struct rtnl_link_stats64 *stats64, > struct rtattr *tb[]); > diff --git a/ip/iptuntap.c b/ip/iptuntap.c > index 385d2bd8..2ae6b1a1 100644 > --- a/ip/iptuntap.c > +++ b/ip/iptuntap.c > @@ -321,14 +321,17 @@ static void show_processes(const char *name) > } else if (err == 2 && > !strcmp("iff", key) && > !strcmp(name, value)) { > - char *pname = get_task_name(pid); > - > - print_string(PRINT_ANY, "name", > - "%s", pname ? : "<NULL>"); > + SPRINT_BUF(pname); > + > + if (get_task_name(pid, pname)) { > + print_string(PRINT_ANY, "name", > + "%s", "<NULL>"); > + } else { > + print_string(PRINT_ANY, "name", > + "%s", pname); > + } > > - print_uint(PRINT_ANY, "pid", > - "(%d)", pid); > - free(pname); > + print_uint(PRINT_ANY, "pid", "(%d)", pid); > } > > free(key); > diff --git a/lib/fs.c b/lib/fs.c > index f6f5f8a0..03df0f6a 100644 > --- a/lib/fs.c > +++ b/lib/fs.c > @@ -342,25 +342,25 @@ int get_command_name(const char *pid, char *comm, size_t len) > return 0; > } > > -char *get_task_name(pid_t pid) > +int get_task_name(pid_t pid, char *name) > { > - char *comm; > + char path[PATH_MAX]; > FILE *f; > > if (!pid) > - return NULL; > + return -1; > > - if (asprintf(&comm, "/proc/%d/comm", pid) < 0) > - return NULL; > + if (snprintf(path, sizeof(path), "/proc/%d/comm", pid) >= sizeof(path)) > + return -1; > > - f = fopen(comm, "r"); > + f = fopen(path, "r"); > if (!f) > - return NULL; > + return -1; > > - if (fscanf(f, "%ms\n", &comm) != 1) > - comm = NULL; > + if (fscanf(f, "%s\n", name) != 1) > + return -1; > > fclose(f); > > - return comm; > + return 0; > } > diff --git a/rdma/res-cmid.c b/rdma/res-cmid.c > index bfaa47b5..3475349d 100644 > --- a/rdma/res-cmid.c > +++ b/rdma/res-cmid.c > @@ -159,8 +159,11 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx, > goto out; > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > + SPRINT_BUF(b); > + > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > - comm = get_task_name(pid); > + if (!get_task_name(pid, b)) > + comm = b; > } > > if (rd_is_filtered_attr(rd, "pid", pid, > @@ -199,8 +202,7 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx, > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > newline(rd); > > -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > - free(comm); > +out: > return MNL_CB_OK; > } > > diff --git a/rdma/res-cq.c b/rdma/res-cq.c > index 9e7c4f51..5ed455ea 100644 > --- a/rdma/res-cq.c > +++ b/rdma/res-cq.c > @@ -84,8 +84,11 @@ static int res_cq_line(struct rd *rd, const char *name, int idx, > goto out; > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > + SPRINT_BUF(b); > + > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > - comm = get_task_name(pid); > + if (!get_task_name(pid, b)) > + comm = b; > } > > if (rd_is_filtered_attr(rd, "pid", pid, > @@ -123,8 +126,7 @@ static int res_cq_line(struct rd *rd, const char *name, int idx, > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > newline(rd); > > -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > - free(comm); > +out: > return MNL_CB_OK; > } > > diff --git a/rdma/res-ctx.c b/rdma/res-ctx.c > index 30afe97a..fbd52dd5 100644 > --- a/rdma/res-ctx.c > +++ b/rdma/res-ctx.c > @@ -18,8 +18,11 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx, > return MNL_CB_ERROR; > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > + SPRINT_BUF(b); > + > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > - comm = get_task_name(pid); > + if (!get_task_name(pid, b)) > + comm = b; > } > > if (rd_is_filtered_attr(rd, "pid", pid, > @@ -48,8 +51,6 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx, > newline(rd); > > out: > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > - free(comm); > return MNL_CB_OK; > } > > diff --git a/rdma/res-mr.c b/rdma/res-mr.c > index 1bf73f3a..6a59d9e4 100644 > --- a/rdma/res-mr.c > +++ b/rdma/res-mr.c > @@ -47,8 +47,11 @@ static int res_mr_line(struct rd *rd, const char *name, int idx, > goto out; > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > + SPRINT_BUF(b); > + > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > - comm = get_task_name(pid); > + if (!get_task_name(pid, b)) > + comm = b; > } > > if (rd_is_filtered_attr(rd, "pid", pid, > @@ -87,8 +90,6 @@ static int res_mr_line(struct rd *rd, const char *name, int idx, > newline(rd); > > out: > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > - free(comm); > return MNL_CB_OK; > } > > diff --git a/rdma/res-pd.c b/rdma/res-pd.c > index df538010..a51bb634 100644 > --- a/rdma/res-pd.c > +++ b/rdma/res-pd.c > @@ -34,8 +34,11 @@ static int res_pd_line(struct rd *rd, const char *name, int idx, > nla_line[RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY]); > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > + SPRINT_BUF(b); > + > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > - comm = get_task_name(pid); > + if (!get_task_name(pid, b)) > + comm = b; > } > > if (rd_is_filtered_attr(rd, "pid", pid, > @@ -76,8 +79,7 @@ static int res_pd_line(struct rd *rd, const char *name, int idx, > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > newline(rd); > > -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > - free(comm); > +out: > return MNL_CB_OK; > } > > diff --git a/rdma/res-qp.c b/rdma/res-qp.c > index a38be399..575e0529 100644 > --- a/rdma/res-qp.c > +++ b/rdma/res-qp.c > @@ -146,8 +146,11 @@ static int res_qp_line(struct rd *rd, const char *name, int idx, > goto out; > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > + SPRINT_BUF(b); > + > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > - comm = get_task_name(pid); > + if (!get_task_name(pid, b)) > + comm = b; > } > > if (rd_is_filtered_attr(rd, "pid", pid, > @@ -179,8 +182,6 @@ static int res_qp_line(struct rd *rd, const char *name, int idx, > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > newline(rd); > out: > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > - free(comm); > return MNL_CB_OK; > } > > diff --git a/rdma/res-srq.c b/rdma/res-srq.c > index 3038c352..945109fc 100644 > --- a/rdma/res-srq.c > +++ b/rdma/res-srq.c > @@ -174,8 +174,11 @@ static int res_srq_line(struct rd *rd, const char *name, int idx, > return MNL_CB_ERROR; > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > + SPRINT_BUF(b); > + > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > - comm = get_task_name(pid); > + if (!get_task_name(pid, b)) > + comm = b; > } > if (rd_is_filtered_attr(rd, "pid", pid, > nla_line[RDMA_NLDEV_ATTR_RES_PID])) > @@ -228,8 +231,6 @@ static int res_srq_line(struct rd *rd, const char *name, int idx, > newline(rd); > > out: > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > - free(comm); > return MNL_CB_OK; > } > > diff --git a/rdma/stat.c b/rdma/stat.c > index adfcd34a..a63b70a4 100644 > --- a/rdma/stat.c > +++ b/rdma/stat.c > @@ -248,8 +248,11 @@ static int res_counter_line(struct rd *rd, const char *name, int index, > return MNL_CB_OK; > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > + SPRINT_BUF(b); > + > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > - comm = get_task_name(pid); > + if (!get_task_name(pid, b)) > + comm = b; > } > if (rd_is_filtered_attr(rd, "pid", pid, > nla_line[RDMA_NLDEV_ATTR_RES_PID])) > -- > 2.35.1 >
On 3/2/22 5:28 AM, Andrea Claudi wrote: > diff --git a/include/utils.h b/include/utils.h > index b6c468e9..81294488 100644 > --- a/include/utils.h > +++ b/include/utils.h > @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount); > __u64 get_cgroup2_id(const char *path); > char *get_cgroup2_path(__u64 id, bool full); > int get_command_name(const char *pid, char *comm, size_t len); > -char *get_task_name(pid_t pid); > +int get_task_name(pid_t pid, char *name); > changing to an API with an assumed length is not better than the current situation. Why not just fixup the callers as needed to free the allocation?
Hi Leon, Thanks for your review. On Thu, Mar 03, 2022 at 08:56:57PM +0200, Leon Romanovsky wrote: > On Wed, Mar 02, 2022 at 01:28:47PM +0100, Andrea Claudi wrote: > > asprintf() allocates memory which is not freed on the error path of > > get_task_name(), thus potentially leading to memory leaks. > > Not really, memory is released when the application exits, which is the > case here. > That's certainly true. However this is still a leak in the time frame between get_task_name function call and the application exit. For example: $ ip -d -b - << EOF tuntap show monitor EOF calls get_task_name one or more time (once for each tun interface), and leaks memory indefinitely, if ip is not interrupted in some way. Of course this is a corner case, and the leaks should anyway be small. However I cannot see this as a good reason not to fix it. > > > > Rework get_task_name() and avoid asprintf() usage and memory allocation, > > returning the task string to the caller using an additional char* > > parameter. > > > > Fixes: 81bfd01a4c9e ("lib: move get_task_name() from rdma") > > As I was told before, in netdev Fixes means that it is a bug that > affects users. This is not the case here. Thanks for letting me know. I usually rely a lot on Fixes: as iproute2 package maintainer, but I'll change my habits if this is the common understanding. Stephen, David, WDYT? > > > Signed-off-by: Andrea Claudi <aclaudi@redhat.com> > > --- > > include/utils.h | 2 +- > > ip/iptuntap.c | 17 ++++++++++------- > > lib/fs.c | 20 ++++++++++---------- > > rdma/res-cmid.c | 8 +++++--- > > rdma/res-cq.c | 8 +++++--- > > rdma/res-ctx.c | 7 ++++--- > > rdma/res-mr.c | 7 ++++--- > > rdma/res-pd.c | 8 +++++--- > > rdma/res-qp.c | 7 ++++--- > > rdma/res-srq.c | 7 ++++--- > > rdma/stat.c | 5 ++++- > > 11 files changed, 56 insertions(+), 40 deletions(-) > > Honestly, I don't see any need in both patches. > > Thanks > > > > > diff --git a/include/utils.h b/include/utils.h > > index b6c468e9..81294488 100644 > > --- a/include/utils.h > > +++ b/include/utils.h > > @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount); > > __u64 get_cgroup2_id(const char *path); > > char *get_cgroup2_path(__u64 id, bool full); > > int get_command_name(const char *pid, char *comm, size_t len); > > -char *get_task_name(pid_t pid); > > +int get_task_name(pid_t pid, char *name); > > > > int get_rtnl_link_stats_rta(struct rtnl_link_stats64 *stats64, > > struct rtattr *tb[]); > > diff --git a/ip/iptuntap.c b/ip/iptuntap.c > > index 385d2bd8..2ae6b1a1 100644 > > --- a/ip/iptuntap.c > > +++ b/ip/iptuntap.c > > @@ -321,14 +321,17 @@ static void show_processes(const char *name) > > } else if (err == 2 && > > !strcmp("iff", key) && > > !strcmp(name, value)) { > > - char *pname = get_task_name(pid); > > - > > - print_string(PRINT_ANY, "name", > > - "%s", pname ? : "<NULL>"); > > + SPRINT_BUF(pname); > > + > > + if (get_task_name(pid, pname)) { > > + print_string(PRINT_ANY, "name", > > + "%s", "<NULL>"); > > + } else { > > + print_string(PRINT_ANY, "name", > > + "%s", pname); > > + } > > > > - print_uint(PRINT_ANY, "pid", > > - "(%d)", pid); > > - free(pname); > > + print_uint(PRINT_ANY, "pid", "(%d)", pid); > > } > > > > free(key); > > diff --git a/lib/fs.c b/lib/fs.c > > index f6f5f8a0..03df0f6a 100644 > > --- a/lib/fs.c > > +++ b/lib/fs.c > > @@ -342,25 +342,25 @@ int get_command_name(const char *pid, char *comm, size_t len) > > return 0; > > } > > > > -char *get_task_name(pid_t pid) > > +int get_task_name(pid_t pid, char *name) > > { > > - char *comm; > > + char path[PATH_MAX]; > > FILE *f; > > > > if (!pid) > > - return NULL; > > + return -1; > > > > - if (asprintf(&comm, "/proc/%d/comm", pid) < 0) > > - return NULL; > > + if (snprintf(path, sizeof(path), "/proc/%d/comm", pid) >= sizeof(path)) > > + return -1; > > > > - f = fopen(comm, "r"); > > + f = fopen(path, "r"); > > if (!f) > > - return NULL; > > + return -1; > > > > - if (fscanf(f, "%ms\n", &comm) != 1) > > - comm = NULL; > > + if (fscanf(f, "%s\n", name) != 1) > > + return -1; > > > > fclose(f); > > > > - return comm; > > + return 0; > > } > > diff --git a/rdma/res-cmid.c b/rdma/res-cmid.c > > index bfaa47b5..3475349d 100644 > > --- a/rdma/res-cmid.c > > +++ b/rdma/res-cmid.c > > @@ -159,8 +159,11 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx, > > goto out; > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > + SPRINT_BUF(b); > > + > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > - comm = get_task_name(pid); > > + if (!get_task_name(pid, b)) > > + comm = b; > > } > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > @@ -199,8 +202,7 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx, > > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > > newline(rd); > > > > -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > - free(comm); > > +out: > > return MNL_CB_OK; > > } > > > > diff --git a/rdma/res-cq.c b/rdma/res-cq.c > > index 9e7c4f51..5ed455ea 100644 > > --- a/rdma/res-cq.c > > +++ b/rdma/res-cq.c > > @@ -84,8 +84,11 @@ static int res_cq_line(struct rd *rd, const char *name, int idx, > > goto out; > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > + SPRINT_BUF(b); > > + > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > - comm = get_task_name(pid); > > + if (!get_task_name(pid, b)) > > + comm = b; > > } > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > @@ -123,8 +126,7 @@ static int res_cq_line(struct rd *rd, const char *name, int idx, > > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > > newline(rd); > > > > -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > - free(comm); > > +out: > > return MNL_CB_OK; > > } > > > > diff --git a/rdma/res-ctx.c b/rdma/res-ctx.c > > index 30afe97a..fbd52dd5 100644 > > --- a/rdma/res-ctx.c > > +++ b/rdma/res-ctx.c > > @@ -18,8 +18,11 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx, > > return MNL_CB_ERROR; > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > + SPRINT_BUF(b); > > + > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > - comm = get_task_name(pid); > > + if (!get_task_name(pid, b)) > > + comm = b; > > } > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > @@ -48,8 +51,6 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx, > > newline(rd); > > > > out: > > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > - free(comm); > > return MNL_CB_OK; > > } > > > > diff --git a/rdma/res-mr.c b/rdma/res-mr.c > > index 1bf73f3a..6a59d9e4 100644 > > --- a/rdma/res-mr.c > > +++ b/rdma/res-mr.c > > @@ -47,8 +47,11 @@ static int res_mr_line(struct rd *rd, const char *name, int idx, > > goto out; > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > + SPRINT_BUF(b); > > + > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > - comm = get_task_name(pid); > > + if (!get_task_name(pid, b)) > > + comm = b; > > } > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > @@ -87,8 +90,6 @@ static int res_mr_line(struct rd *rd, const char *name, int idx, > > newline(rd); > > > > out: > > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > - free(comm); > > return MNL_CB_OK; > > } > > > > diff --git a/rdma/res-pd.c b/rdma/res-pd.c > > index df538010..a51bb634 100644 > > --- a/rdma/res-pd.c > > +++ b/rdma/res-pd.c > > @@ -34,8 +34,11 @@ static int res_pd_line(struct rd *rd, const char *name, int idx, > > nla_line[RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY]); > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > + SPRINT_BUF(b); > > + > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > - comm = get_task_name(pid); > > + if (!get_task_name(pid, b)) > > + comm = b; > > } > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > @@ -76,8 +79,7 @@ static int res_pd_line(struct rd *rd, const char *name, int idx, > > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > > newline(rd); > > > > -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > - free(comm); > > +out: > > return MNL_CB_OK; > > } > > > > diff --git a/rdma/res-qp.c b/rdma/res-qp.c > > index a38be399..575e0529 100644 > > --- a/rdma/res-qp.c > > +++ b/rdma/res-qp.c > > @@ -146,8 +146,11 @@ static int res_qp_line(struct rd *rd, const char *name, int idx, > > goto out; > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > + SPRINT_BUF(b); > > + > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > - comm = get_task_name(pid); > > + if (!get_task_name(pid, b)) > > + comm = b; > > } > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > @@ -179,8 +182,6 @@ static int res_qp_line(struct rd *rd, const char *name, int idx, > > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > > newline(rd); > > out: > > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > - free(comm); > > return MNL_CB_OK; > > } > > > > diff --git a/rdma/res-srq.c b/rdma/res-srq.c > > index 3038c352..945109fc 100644 > > --- a/rdma/res-srq.c > > +++ b/rdma/res-srq.c > > @@ -174,8 +174,11 @@ static int res_srq_line(struct rd *rd, const char *name, int idx, > > return MNL_CB_ERROR; > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > + SPRINT_BUF(b); > > + > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > - comm = get_task_name(pid); > > + if (!get_task_name(pid, b)) > > + comm = b; > > } > > if (rd_is_filtered_attr(rd, "pid", pid, > > nla_line[RDMA_NLDEV_ATTR_RES_PID])) > > @@ -228,8 +231,6 @@ static int res_srq_line(struct rd *rd, const char *name, int idx, > > newline(rd); > > > > out: > > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > - free(comm); > > return MNL_CB_OK; > > } > > > > diff --git a/rdma/stat.c b/rdma/stat.c > > index adfcd34a..a63b70a4 100644 > > --- a/rdma/stat.c > > +++ b/rdma/stat.c > > @@ -248,8 +248,11 @@ static int res_counter_line(struct rd *rd, const char *name, int index, > > return MNL_CB_OK; > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > + SPRINT_BUF(b); > > + > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > - comm = get_task_name(pid); > > + if (!get_task_name(pid, b)) > > + comm = b; > > } > > if (rd_is_filtered_attr(rd, "pid", pid, > > nla_line[RDMA_NLDEV_ATTR_RES_PID])) > > -- > > 2.35.1 > > >
On 3/7/22 11:07 AM, Andrea Claudi wrote: > Thanks for letting me know. I usually rely a lot on Fixes: as iproute2 > package maintainer, but I'll change my habits if this is the common > understanding. Stephen, David, WDYT? I think a Fixes tag is relevant here.
On Mon, 7 Mar 2022 10:58:37 -0700 David Ahern <dsahern@kernel.org> wrote: > On 3/2/22 5:28 AM, Andrea Claudi wrote: > > diff --git a/include/utils.h b/include/utils.h > > index b6c468e9..81294488 100644 > > --- a/include/utils.h > > +++ b/include/utils.h > > @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount); > > __u64 get_cgroup2_id(const char *path); > > char *get_cgroup2_path(__u64 id, bool full); > > int get_command_name(const char *pid, char *comm, size_t len); > > -char *get_task_name(pid_t pid); > > +int get_task_name(pid_t pid, char *name); > > > > changing to an API with an assumed length is not better than the current > situation. Why not just fixup the callers as needed to free the allocation? I wonder if iproute2 should start using GCC attribute cleanup function (like systemd) or would this be too much of using a new thing? Not sure if using the attribute (wrapped in macro) just hides the problem
On Mon, Mar 07, 2022 at 10:58:37AM -0700, David Ahern wrote: > On 3/2/22 5:28 AM, Andrea Claudi wrote: > > diff --git a/include/utils.h b/include/utils.h > > index b6c468e9..81294488 100644 > > --- a/include/utils.h > > +++ b/include/utils.h > > @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount); > > __u64 get_cgroup2_id(const char *path); > > char *get_cgroup2_path(__u64 id, bool full); > > int get_command_name(const char *pid, char *comm, size_t len); > > -char *get_task_name(pid_t pid); > > +int get_task_name(pid_t pid, char *name); > > > > changing to an API with an assumed length is not better than the current > situation. Why not just fixup the callers as needed to free the allocation? > I actually did that on v1. After Stephen's comment about asprintf(), I got the idea to make get_task_name() similar to get_command_name() and a bit more "user-friendly", so that callers do not need a free(). If you think this is not ideal, I can post a v3 with the necessary fixes to the callers. Thanks, Andrea
On Mon, 7 Mar 2022 19:21:41 +0100 Andrea Claudi <aclaudi@redhat.com> wrote: > On Mon, Mar 07, 2022 at 10:58:37AM -0700, David Ahern wrote: > > On 3/2/22 5:28 AM, Andrea Claudi wrote: > > > diff --git a/include/utils.h b/include/utils.h > > > index b6c468e9..81294488 100644 > > > --- a/include/utils.h > > > +++ b/include/utils.h > > > @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount); > > > __u64 get_cgroup2_id(const char *path); > > > char *get_cgroup2_path(__u64 id, bool full); > > > int get_command_name(const char *pid, char *comm, size_t len); > > > -char *get_task_name(pid_t pid); > > > +int get_task_name(pid_t pid, char *name); > > > > > > > changing to an API with an assumed length is not better than the current > > situation. Why not just fixup the callers as needed to free the allocation? > > > > I actually did that on v1. After Stephen's comment about asprintf(), I > got the idea to make get_task_name() similar to get_command_name() and > a bit more "user-friendly", so that callers do not need a free(). > > If you think this is not ideal, I can post a v3 with the necessary fixes > to the callers. > > Thanks, > Andrea > My comment was purely a suggestion not a requirement. I have just had issues with complaints from compiler about code not checking return value from asprintf, so tend to avoid it.
On 3/7/22 11:21 AM, Andrea Claudi wrote: > On Mon, Mar 07, 2022 at 10:58:37AM -0700, David Ahern wrote: >> On 3/2/22 5:28 AM, Andrea Claudi wrote: >>> diff --git a/include/utils.h b/include/utils.h >>> index b6c468e9..81294488 100644 >>> --- a/include/utils.h >>> +++ b/include/utils.h >>> @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount); >>> __u64 get_cgroup2_id(const char *path); >>> char *get_cgroup2_path(__u64 id, bool full); >>> int get_command_name(const char *pid, char *comm, size_t len); >>> -char *get_task_name(pid_t pid); >>> +int get_task_name(pid_t pid, char *name); >>> >> >> changing to an API with an assumed length is not better than the current >> situation. Why not just fixup the callers as needed to free the allocation? >> > > I actually did that on v1. After Stephen's comment about asprintf(), I > got the idea to make get_task_name() similar to get_command_name() and > a bit more "user-friendly", so that callers do not need a free(). > get_command_name passes a buffer and length. That's a better API - no assumptions.
On Mon, Mar 07, 2022 at 07:07:10PM +0100, Andrea Claudi wrote: > Hi Leon, > Thanks for your review. > > On Thu, Mar 03, 2022 at 08:56:57PM +0200, Leon Romanovsky wrote: > > On Wed, Mar 02, 2022 at 01:28:47PM +0100, Andrea Claudi wrote: > > > asprintf() allocates memory which is not freed on the error path of > > > get_task_name(), thus potentially leading to memory leaks. > > > > Not really, memory is released when the application exits, which is the > > case here. > > > > That's certainly true. However this is still a leak in the time frame > between get_task_name function call and the application exit. For > example: > > $ ip -d -b - << EOF > tuntap show > monitor > EOF > > calls get_task_name one or more time (once for each tun interface), and > leaks memory indefinitely, if ip is not interrupted in some way. > > Of course this is a corner case, and the leaks should anyway be small. > However I cannot see this as a good reason not to fix it. > > > > > > > Rework get_task_name() and avoid asprintf() usage and memory allocation, > > > returning the task string to the caller using an additional char* > > > parameter. > > > > > > Fixes: 81bfd01a4c9e ("lib: move get_task_name() from rdma") > > > > As I was told before, in netdev Fixes means that it is a bug that > > affects users. This is not the case here. > > Thanks for letting me know. I usually rely a lot on Fixes: as iproute2 > package maintainer, but I'll change my habits if this is the common > understanding. Stephen, David, WDYT? > > > > > > Signed-off-by: Andrea Claudi <aclaudi@redhat.com> > > > --- > > > include/utils.h | 2 +- > > > ip/iptuntap.c | 17 ++++++++++------- Ahh, sorry, I missed this user. So yes, Fixes is needed here. Thanks > > > lib/fs.c | 20 ++++++++++---------- > > > rdma/res-cmid.c | 8 +++++--- > > > rdma/res-cq.c | 8 +++++--- > > > rdma/res-ctx.c | 7 ++++--- > > > rdma/res-mr.c | 7 ++++--- > > > rdma/res-pd.c | 8 +++++--- > > > rdma/res-qp.c | 7 ++++--- > > > rdma/res-srq.c | 7 ++++--- > > > rdma/stat.c | 5 ++++- > > > 11 files changed, 56 insertions(+), 40 deletions(-) > > > > Honestly, I don't see any need in both patches. > > > > Thanks > > > > > > > > diff --git a/include/utils.h b/include/utils.h > > > index b6c468e9..81294488 100644 > > > --- a/include/utils.h > > > +++ b/include/utils.h > > > @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount); > > > __u64 get_cgroup2_id(const char *path); > > > char *get_cgroup2_path(__u64 id, bool full); > > > int get_command_name(const char *pid, char *comm, size_t len); > > > -char *get_task_name(pid_t pid); > > > +int get_task_name(pid_t pid, char *name); > > > > > > int get_rtnl_link_stats_rta(struct rtnl_link_stats64 *stats64, > > > struct rtattr *tb[]); > > > diff --git a/ip/iptuntap.c b/ip/iptuntap.c > > > index 385d2bd8..2ae6b1a1 100644 > > > --- a/ip/iptuntap.c > > > +++ b/ip/iptuntap.c > > > @@ -321,14 +321,17 @@ static void show_processes(const char *name) > > > } else if (err == 2 && > > > !strcmp("iff", key) && > > > !strcmp(name, value)) { > > > - char *pname = get_task_name(pid); > > > - > > > - print_string(PRINT_ANY, "name", > > > - "%s", pname ? : "<NULL>"); > > > + SPRINT_BUF(pname); > > > + > > > + if (get_task_name(pid, pname)) { > > > + print_string(PRINT_ANY, "name", > > > + "%s", "<NULL>"); > > > + } else { > > > + print_string(PRINT_ANY, "name", > > > + "%s", pname); > > > + } > > > > > > - print_uint(PRINT_ANY, "pid", > > > - "(%d)", pid); > > > - free(pname); > > > + print_uint(PRINT_ANY, "pid", "(%d)", pid); > > > } > > > > > > free(key); > > > diff --git a/lib/fs.c b/lib/fs.c > > > index f6f5f8a0..03df0f6a 100644 > > > --- a/lib/fs.c > > > +++ b/lib/fs.c > > > @@ -342,25 +342,25 @@ int get_command_name(const char *pid, char *comm, size_t len) > > > return 0; > > > } > > > > > > -char *get_task_name(pid_t pid) > > > +int get_task_name(pid_t pid, char *name) > > > { > > > - char *comm; > > > + char path[PATH_MAX]; > > > FILE *f; > > > > > > if (!pid) > > > - return NULL; > > > + return -1; > > > > > > - if (asprintf(&comm, "/proc/%d/comm", pid) < 0) > > > - return NULL; > > > + if (snprintf(path, sizeof(path), "/proc/%d/comm", pid) >= sizeof(path)) > > > + return -1; > > > > > > - f = fopen(comm, "r"); > > > + f = fopen(path, "r"); > > > if (!f) > > > - return NULL; > > > + return -1; > > > > > > - if (fscanf(f, "%ms\n", &comm) != 1) > > > - comm = NULL; > > > + if (fscanf(f, "%s\n", name) != 1) > > > + return -1; > > > > > > fclose(f); > > > > > > - return comm; > > > + return 0; > > > } > > > diff --git a/rdma/res-cmid.c b/rdma/res-cmid.c > > > index bfaa47b5..3475349d 100644 > > > --- a/rdma/res-cmid.c > > > +++ b/rdma/res-cmid.c > > > @@ -159,8 +159,11 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx, > > > goto out; > > > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > > + SPRINT_BUF(b); > > > + > > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > > - comm = get_task_name(pid); > > > + if (!get_task_name(pid, b)) > > > + comm = b; > > > } > > > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > > @@ -199,8 +202,7 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx, > > > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > > > newline(rd); > > > > > > -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > > - free(comm); > > > +out: > > > return MNL_CB_OK; > > > } > > > > > > diff --git a/rdma/res-cq.c b/rdma/res-cq.c > > > index 9e7c4f51..5ed455ea 100644 > > > --- a/rdma/res-cq.c > > > +++ b/rdma/res-cq.c > > > @@ -84,8 +84,11 @@ static int res_cq_line(struct rd *rd, const char *name, int idx, > > > goto out; > > > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > > + SPRINT_BUF(b); > > > + > > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > > - comm = get_task_name(pid); > > > + if (!get_task_name(pid, b)) > > > + comm = b; > > > } > > > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > > @@ -123,8 +126,7 @@ static int res_cq_line(struct rd *rd, const char *name, int idx, > > > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > > > newline(rd); > > > > > > -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > > - free(comm); > > > +out: > > > return MNL_CB_OK; > > > } > > > > > > diff --git a/rdma/res-ctx.c b/rdma/res-ctx.c > > > index 30afe97a..fbd52dd5 100644 > > > --- a/rdma/res-ctx.c > > > +++ b/rdma/res-ctx.c > > > @@ -18,8 +18,11 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx, > > > return MNL_CB_ERROR; > > > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > > + SPRINT_BUF(b); > > > + > > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > > - comm = get_task_name(pid); > > > + if (!get_task_name(pid, b)) > > > + comm = b; > > > } > > > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > > @@ -48,8 +51,6 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx, > > > newline(rd); > > > > > > out: > > > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > > - free(comm); > > > return MNL_CB_OK; > > > } > > > > > > diff --git a/rdma/res-mr.c b/rdma/res-mr.c > > > index 1bf73f3a..6a59d9e4 100644 > > > --- a/rdma/res-mr.c > > > +++ b/rdma/res-mr.c > > > @@ -47,8 +47,11 @@ static int res_mr_line(struct rd *rd, const char *name, int idx, > > > goto out; > > > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > > + SPRINT_BUF(b); > > > + > > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > > - comm = get_task_name(pid); > > > + if (!get_task_name(pid, b)) > > > + comm = b; > > > } > > > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > > @@ -87,8 +90,6 @@ static int res_mr_line(struct rd *rd, const char *name, int idx, > > > newline(rd); > > > > > > out: > > > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > > - free(comm); > > > return MNL_CB_OK; > > > } > > > > > > diff --git a/rdma/res-pd.c b/rdma/res-pd.c > > > index df538010..a51bb634 100644 > > > --- a/rdma/res-pd.c > > > +++ b/rdma/res-pd.c > > > @@ -34,8 +34,11 @@ static int res_pd_line(struct rd *rd, const char *name, int idx, > > > nla_line[RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY]); > > > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > > + SPRINT_BUF(b); > > > + > > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > > - comm = get_task_name(pid); > > > + if (!get_task_name(pid, b)) > > > + comm = b; > > > } > > > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > > @@ -76,8 +79,7 @@ static int res_pd_line(struct rd *rd, const char *name, int idx, > > > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > > > newline(rd); > > > > > > -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > > - free(comm); > > > +out: > > > return MNL_CB_OK; > > > } > > > > > > diff --git a/rdma/res-qp.c b/rdma/res-qp.c > > > index a38be399..575e0529 100644 > > > --- a/rdma/res-qp.c > > > +++ b/rdma/res-qp.c > > > @@ -146,8 +146,11 @@ static int res_qp_line(struct rd *rd, const char *name, int idx, > > > goto out; > > > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > > + SPRINT_BUF(b); > > > + > > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > > - comm = get_task_name(pid); > > > + if (!get_task_name(pid, b)) > > > + comm = b; > > > } > > > > > > if (rd_is_filtered_attr(rd, "pid", pid, > > > @@ -179,8 +182,6 @@ static int res_qp_line(struct rd *rd, const char *name, int idx, > > > print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > > > newline(rd); > > > out: > > > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > > - free(comm); > > > return MNL_CB_OK; > > > } > > > > > > diff --git a/rdma/res-srq.c b/rdma/res-srq.c > > > index 3038c352..945109fc 100644 > > > --- a/rdma/res-srq.c > > > +++ b/rdma/res-srq.c > > > @@ -174,8 +174,11 @@ static int res_srq_line(struct rd *rd, const char *name, int idx, > > > return MNL_CB_ERROR; > > > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > > + SPRINT_BUF(b); > > > + > > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > > - comm = get_task_name(pid); > > > + if (!get_task_name(pid, b)) > > > + comm = b; > > > } > > > if (rd_is_filtered_attr(rd, "pid", pid, > > > nla_line[RDMA_NLDEV_ATTR_RES_PID])) > > > @@ -228,8 +231,6 @@ static int res_srq_line(struct rd *rd, const char *name, int idx, > > > newline(rd); > > > > > > out: > > > - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > > > - free(comm); > > > return MNL_CB_OK; > > > } > > > > > > diff --git a/rdma/stat.c b/rdma/stat.c > > > index adfcd34a..a63b70a4 100644 > > > --- a/rdma/stat.c > > > +++ b/rdma/stat.c > > > @@ -248,8 +248,11 @@ static int res_counter_line(struct rd *rd, const char *name, int index, > > > return MNL_CB_OK; > > > > > > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { > > > + SPRINT_BUF(b); > > > + > > > pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); > > > - comm = get_task_name(pid); > > > + if (!get_task_name(pid, b)) > > > + comm = b; > > > } > > > if (rd_is_filtered_attr(rd, "pid", pid, > > > nla_line[RDMA_NLDEV_ATTR_RES_PID])) > > > -- > > > 2.35.1 > > > > > >
diff --git a/include/utils.h b/include/utils.h index b6c468e9..81294488 100644 --- a/include/utils.h +++ b/include/utils.h @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount); __u64 get_cgroup2_id(const char *path); char *get_cgroup2_path(__u64 id, bool full); int get_command_name(const char *pid, char *comm, size_t len); -char *get_task_name(pid_t pid); +int get_task_name(pid_t pid, char *name); int get_rtnl_link_stats_rta(struct rtnl_link_stats64 *stats64, struct rtattr *tb[]); diff --git a/ip/iptuntap.c b/ip/iptuntap.c index 385d2bd8..2ae6b1a1 100644 --- a/ip/iptuntap.c +++ b/ip/iptuntap.c @@ -321,14 +321,17 @@ static void show_processes(const char *name) } else if (err == 2 && !strcmp("iff", key) && !strcmp(name, value)) { - char *pname = get_task_name(pid); - - print_string(PRINT_ANY, "name", - "%s", pname ? : "<NULL>"); + SPRINT_BUF(pname); + + if (get_task_name(pid, pname)) { + print_string(PRINT_ANY, "name", + "%s", "<NULL>"); + } else { + print_string(PRINT_ANY, "name", + "%s", pname); + } - print_uint(PRINT_ANY, "pid", - "(%d)", pid); - free(pname); + print_uint(PRINT_ANY, "pid", "(%d)", pid); } free(key); diff --git a/lib/fs.c b/lib/fs.c index f6f5f8a0..03df0f6a 100644 --- a/lib/fs.c +++ b/lib/fs.c @@ -342,25 +342,25 @@ int get_command_name(const char *pid, char *comm, size_t len) return 0; } -char *get_task_name(pid_t pid) +int get_task_name(pid_t pid, char *name) { - char *comm; + char path[PATH_MAX]; FILE *f; if (!pid) - return NULL; + return -1; - if (asprintf(&comm, "/proc/%d/comm", pid) < 0) - return NULL; + if (snprintf(path, sizeof(path), "/proc/%d/comm", pid) >= sizeof(path)) + return -1; - f = fopen(comm, "r"); + f = fopen(path, "r"); if (!f) - return NULL; + return -1; - if (fscanf(f, "%ms\n", &comm) != 1) - comm = NULL; + if (fscanf(f, "%s\n", name) != 1) + return -1; fclose(f); - return comm; + return 0; } diff --git a/rdma/res-cmid.c b/rdma/res-cmid.c index bfaa47b5..3475349d 100644 --- a/rdma/res-cmid.c +++ b/rdma/res-cmid.c @@ -159,8 +159,11 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx, goto out; if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { + SPRINT_BUF(b); + pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); - comm = get_task_name(pid); + if (!get_task_name(pid, b)) + comm = b; } if (rd_is_filtered_attr(rd, "pid", pid, @@ -199,8 +202,7 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx, print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); newline(rd); -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) - free(comm); +out: return MNL_CB_OK; } diff --git a/rdma/res-cq.c b/rdma/res-cq.c index 9e7c4f51..5ed455ea 100644 --- a/rdma/res-cq.c +++ b/rdma/res-cq.c @@ -84,8 +84,11 @@ static int res_cq_line(struct rd *rd, const char *name, int idx, goto out; if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { + SPRINT_BUF(b); + pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); - comm = get_task_name(pid); + if (!get_task_name(pid, b)) + comm = b; } if (rd_is_filtered_attr(rd, "pid", pid, @@ -123,8 +126,7 @@ static int res_cq_line(struct rd *rd, const char *name, int idx, print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); newline(rd); -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) - free(comm); +out: return MNL_CB_OK; } diff --git a/rdma/res-ctx.c b/rdma/res-ctx.c index 30afe97a..fbd52dd5 100644 --- a/rdma/res-ctx.c +++ b/rdma/res-ctx.c @@ -18,8 +18,11 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx, return MNL_CB_ERROR; if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { + SPRINT_BUF(b); + pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); - comm = get_task_name(pid); + if (!get_task_name(pid, b)) + comm = b; } if (rd_is_filtered_attr(rd, "pid", pid, @@ -48,8 +51,6 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx, newline(rd); out: - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) - free(comm); return MNL_CB_OK; } diff --git a/rdma/res-mr.c b/rdma/res-mr.c index 1bf73f3a..6a59d9e4 100644 --- a/rdma/res-mr.c +++ b/rdma/res-mr.c @@ -47,8 +47,11 @@ static int res_mr_line(struct rd *rd, const char *name, int idx, goto out; if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { + SPRINT_BUF(b); + pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); - comm = get_task_name(pid); + if (!get_task_name(pid, b)) + comm = b; } if (rd_is_filtered_attr(rd, "pid", pid, @@ -87,8 +90,6 @@ static int res_mr_line(struct rd *rd, const char *name, int idx, newline(rd); out: - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) - free(comm); return MNL_CB_OK; } diff --git a/rdma/res-pd.c b/rdma/res-pd.c index df538010..a51bb634 100644 --- a/rdma/res-pd.c +++ b/rdma/res-pd.c @@ -34,8 +34,11 @@ static int res_pd_line(struct rd *rd, const char *name, int idx, nla_line[RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY]); if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { + SPRINT_BUF(b); + pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); - comm = get_task_name(pid); + if (!get_task_name(pid, b)) + comm = b; } if (rd_is_filtered_attr(rd, "pid", pid, @@ -76,8 +79,7 @@ static int res_pd_line(struct rd *rd, const char *name, int idx, print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); newline(rd); -out: if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) - free(comm); +out: return MNL_CB_OK; } diff --git a/rdma/res-qp.c b/rdma/res-qp.c index a38be399..575e0529 100644 --- a/rdma/res-qp.c +++ b/rdma/res-qp.c @@ -146,8 +146,11 @@ static int res_qp_line(struct rd *rd, const char *name, int idx, goto out; if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { + SPRINT_BUF(b); + pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); - comm = get_task_name(pid); + if (!get_task_name(pid, b)) + comm = b; } if (rd_is_filtered_attr(rd, "pid", pid, @@ -179,8 +182,6 @@ static int res_qp_line(struct rd *rd, const char *name, int idx, print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); newline(rd); out: - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) - free(comm); return MNL_CB_OK; } diff --git a/rdma/res-srq.c b/rdma/res-srq.c index 3038c352..945109fc 100644 --- a/rdma/res-srq.c +++ b/rdma/res-srq.c @@ -174,8 +174,11 @@ static int res_srq_line(struct rd *rd, const char *name, int idx, return MNL_CB_ERROR; if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { + SPRINT_BUF(b); + pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); - comm = get_task_name(pid); + if (!get_task_name(pid, b)) + comm = b; } if (rd_is_filtered_attr(rd, "pid", pid, nla_line[RDMA_NLDEV_ATTR_RES_PID])) @@ -228,8 +231,6 @@ static int res_srq_line(struct rd *rd, const char *name, int idx, newline(rd); out: - if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) - free(comm); return MNL_CB_OK; } diff --git a/rdma/stat.c b/rdma/stat.c index adfcd34a..a63b70a4 100644 --- a/rdma/stat.c +++ b/rdma/stat.c @@ -248,8 +248,11 @@ static int res_counter_line(struct rd *rd, const char *name, int index, return MNL_CB_OK; if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) { + SPRINT_BUF(b); + pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]); - comm = get_task_name(pid); + if (!get_task_name(pid, b)) + comm = b; } if (rd_is_filtered_attr(rd, "pid", pid, nla_line[RDMA_NLDEV_ATTR_RES_PID]))
asprintf() allocates memory which is not freed on the error path of get_task_name(), thus potentially leading to memory leaks. Rework get_task_name() and avoid asprintf() usage and memory allocation, returning the task string to the caller using an additional char* parameter. Fixes: 81bfd01a4c9e ("lib: move get_task_name() from rdma") Signed-off-by: Andrea Claudi <aclaudi@redhat.com> --- include/utils.h | 2 +- ip/iptuntap.c | 17 ++++++++++------- lib/fs.c | 20 ++++++++++---------- rdma/res-cmid.c | 8 +++++--- rdma/res-cq.c | 8 +++++--- rdma/res-ctx.c | 7 ++++--- rdma/res-mr.c | 7 ++++--- rdma/res-pd.c | 8 +++++--- rdma/res-qp.c | 7 ++++--- rdma/res-srq.c | 7 ++++--- rdma/stat.c | 5 ++++- 11 files changed, 56 insertions(+), 40 deletions(-)