Skip to contentSkip to conversation

some memory leak fixes

PATCH Superseded Andrea Claudi 2 participants 7 messages Feb 1, 2022 – Feb 2, 2022

Subsystems (3)
Networking Network drivers Library code

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Andrea Claudi

author
#0 lore raw

cover some memory leak fixes

This series fixes some memory leaks related to the usage of the get_task_name() function from lib/fs.c.

Patch 3/3 addresses a coverity warning related to this memory leak, making the code a bit more readable by humans and coverity.

Andrea Claudi (3):
lib/fs: fix memory leak in get_task_name()
rdma: stat: fix memory leak in res_counter_line()
rdma: RES_PID and RES_KERN_NAME are alternatives to each other

 lib/fs.c        | 10 +++++--
 rdma/res-cmid.c | 10 +++----
 rdma/res-cq.c   |  9 +++---
 rdma/res-ctx.c  |  9 +++---
 rdma/res-mr.c   |  8 ++---
 rdma/res-pd.c   |  9 +++---
 rdma/res-qp.c   |  9 +++---
 rdma/res-srq.c  | 10 +++----
 rdma/stat.c     | 79 ++++++++++++++++++++++++++++++++-----------------
 9 files changed, 88 insertions(+), 65 deletions(-)
Signature · 1 line
2.34.1
author
#6 lore raw

PATCH 3/3 rdma: RES_PID and RES_KERN_NAME are alternatives to each other

RDMA_NLDEV_ATTR_RES_PID and RDMA_NLDEV_ATTR_RES_KERN_NAME cannot be set together, as evident from the fill_res_name_pid() function in the kernel infiniband driver. This commit makes this clear at first glance, using an else branch for RDMA_NLDEV_ATTR_RES_KERN_NAME.

This also helps coverity to better understand this code and avoid producing a bogus warning complaining about mnl_attr_get_str overwriting comm, and thus leaking the storage that comm points to.

Signed-off-by Andrea Claudi <[email protected]>
---
 rdma/res-cmid.c | 10 ++++------
 rdma/res-cq.c   |  9 ++++-----
 rdma/res-ctx.c  |  9 ++++-----
 rdma/res-mr.c   |  8 ++++----
 rdma/res-pd.c   |  9 ++++-----
 rdma/res-qp.c   |  9 ++++-----
 rdma/res-srq.c  | 10 +++++-----
 rdma/stat.c     | 11 +++++------
 8 files changed, 34 insertions(+), 41 deletions(-)
rdma/res-cmid.c +4 -6
index bfaa47b5..21bdabca 100644
@@ -161,6 +161,10 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx,
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		comm = get_task_name(pid);
+	} else if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+		/* discard const from mnl_attr_get_str */
+		comm = (char *)mnl_attr_get_str(
+			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
 	}
 
 	if (rd_is_filtered_attr(rd, "pid", pid,

@@ -173,12 +177,6 @@ static int res_cm_id_line(struct rd *rd, const char *name, int idx,
 				nla_line[RDMA_NLDEV_ATTR_RES_CM_IDN]))
 		goto out;
 
-	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
-		/* discard const from mnl_attr_get_str */
-		comm = (char *)mnl_attr_get_str(
-			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
-	}
-
 	open_json_object(NULL);
 	print_link(rd, idx, name, port, nla_line);
 	res_print_uint(rd, "cm-idn", cm_idn,
rdma/res-cq.c +4 -5
index 9e7c4f51..0ba19447 100644
@@ -86,6 +86,10 @@ static int res_cq_line(struct rd *rd, const char *name, int idx,
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		comm = get_task_name(pid);
+	} else if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+		/* discard const from mnl_attr_get_str */
+		comm = (char *)mnl_attr_get_str(
+			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
 	}
 
 	if (rd_is_filtered_attr(rd, "pid", pid,

@@ -103,11 +107,6 @@ static int res_cq_line(struct rd *rd, const char *name, int idx,
 				nla_line[RDMA_NLDEV_ATTR_RES_CTXN]))
 		goto out;
 
-	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME])
-		/* discard const from mnl_attr_get_str */
-		comm = (char *)mnl_attr_get_str(
-			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
-
 	open_json_object(NULL);
 	print_dev(rd, idx, name);
 	res_print_uint(rd, "cqn", cqn, nla_line[RDMA_NLDEV_ATTR_RES_CQN]);
rdma/res-ctx.c +4 -5
index 30afe97a..895598d8 100644
@@ -20,6 +20,10 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx,
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		comm = get_task_name(pid);
+	} else if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+		/* discard const from mnl_attr_get_str */
+		comm = (char *)mnl_attr_get_str(
+			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
 	}
 
 	if (rd_is_filtered_attr(rd, "pid", pid,

@@ -33,11 +37,6 @@ static int res_ctx_line(struct rd *rd, const char *name, int idx,
 				nla_line[RDMA_NLDEV_ATTR_RES_CTXN]))
 		goto out;
 
-	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME])
-		/* discard const from mnl_attr_get_str */
-		comm = (char *)mnl_attr_get_str(
-			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
-
 	open_json_object(NULL);
 	print_dev(rd, idx, name);
 	res_print_uint(rd, "ctxn", ctxn, nla_line[RDMA_NLDEV_ATTR_RES_CTXN]);
rdma/res-mr.c +4 -4
index 1bf73f3a..9ae6c777 100644
@@ -49,6 +49,10 @@ static int res_mr_line(struct rd *rd, const char *name, int idx,
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		comm = get_task_name(pid);
+	} else if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+		/* discard const from mnl_attr_get_str */
+		comm = (char *)mnl_attr_get_str(
+			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
 	}
 
 	if (rd_is_filtered_attr(rd, "pid", pid,

@@ -67,10 +71,6 @@ static int res_mr_line(struct rd *rd, const char *name, int idx,
 				nla_line[RDMA_NLDEV_ATTR_RES_PDN]))
 		goto out;
 
-	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME])
-		/* discard const from mnl_attr_get_str */
-		comm = (char *)mnl_attr_get_str(
-			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
 	open_json_object(NULL);
 	print_dev(rd, idx, name);
 	res_print_uint(rd, "mrn", mrn, nla_line[RDMA_NLDEV_ATTR_RES_MRN]);
rdma/res-pd.c +4 -5
index df538010..aca2b0cc 100644
@@ -36,6 +36,10 @@ static int res_pd_line(struct rd *rd, const char *name, int idx,
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		comm = get_task_name(pid);
+	} else if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+		/* discard const from mnl_attr_get_str */
+		comm = (char *)mnl_attr_get_str(
+			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
 	}
 
 	if (rd_is_filtered_attr(rd, "pid", pid,

@@ -55,11 +59,6 @@ static int res_pd_line(struct rd *rd, const char *name, int idx,
 				nla_line[RDMA_NLDEV_ATTR_RES_PDN]))
 		goto out;
 
-	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME])
-		/* discard const from mnl_attr_get_str */
-		comm = (char *)mnl_attr_get_str(
-			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
-
 	open_json_object(NULL);
 	print_dev(rd, idx, name);
 	res_print_uint(rd, "pdn", pdn, nla_line[RDMA_NLDEV_ATTR_RES_PDN]);
rdma/res-qp.c +4 -5
index a38be399..76a5334f 100644
@@ -148,17 +148,16 @@ static int res_qp_line(struct rd *rd, const char *name, int idx,
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		comm = get_task_name(pid);
+	} else if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+		/* discard const from mnl_attr_get_str */
+		comm = (char *)mnl_attr_get_str(
+			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
 	}
 
 	if (rd_is_filtered_attr(rd, "pid", pid,
 				nla_line[RDMA_NLDEV_ATTR_RES_PID]))
 		goto out;
 
-	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME])
-		/* discard const from mnl_attr_get_str */
-		comm = (char *)mnl_attr_get_str(
-			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
-
 	open_json_object(NULL);
 	print_link(rd, idx, name, port, nla_line);
 	res_print_uint(rd, "lqpn", lqpn, nla_line[RDMA_NLDEV_ATTR_RES_LQPN]);
rdma/res-srq.c +5 -5
index 3038c352..982e3fe9 100644
@@ -176,7 +176,12 @@ static int res_srq_line(struct rd *rd, const char *name, int idx,
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		comm = get_task_name(pid);
+	} else if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+		/* discard const from mnl_attr_get_str */
+		comm = (char *)mnl_attr_get_str(
+			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
 	}
+
 	if (rd_is_filtered_attr(rd, "pid", pid,
 				nla_line[RDMA_NLDEV_ATTR_RES_PID]))
 		goto out;

@@ -209,11 +214,6 @@ static int res_srq_line(struct rd *rd, const char *name, int idx,
 			MNL_CB_OK)
 		goto out;
 
-	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME])
-		/* discard const from mnl_attr_get_str */
-		comm = (char *)mnl_attr_get_str(
-			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
-
 	open_json_object(NULL);
 	print_dev(rd, idx, name);
 	res_print_uint(rd, "srqn", srqn, nla_line[RDMA_NLDEV_ATTR_RES_SRQN]);
rdma/stat.c +5 -6
index 66121b12..cf2c705b 100644
@@ -256,19 +256,18 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		comm = get_task_name(pid);
+	} else if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+		/* discard const from mnl_attr_get_str */
+		comm = (char *)mnl_attr_get_str(
+			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
 	}
+
 	if (rd_is_filtered_attr(rd, "pid", pid,
 				nla_line[RDMA_NLDEV_ATTR_RES_PID])) {
 		ret = MNL_CB_OK;
 		goto out;
 	}
 
-	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
-		/* discard const from mnl_attr_get_str */
-		comm = (char *)mnl_attr_get_str(
-			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
-	}
-
 	mnl_attr_for_each_nested(nla_entry, qp_table) {
 		struct nlattr *qp_line[RDMA_NLDEV_ATTR_MAX] = {};
Signature · 1 line
2.34.1
author
#5 lore raw

PATCH 2/3 rdma: stat: fix memory leak in res_counter_line()

In reply to Andrea Claudi

get_task_name() uses asprintf() to reserve storage for the task name string. As asprintf() manpage states, a free() should be used to release the allocated storage when it is no longer needed.

res_counter_line() uses get_task_name() but does not free the allocated storage on the error paths and after the usage.

This uses a single return point to free the allocated storage, adopting the same approach of other rdma code using get_task_name().

Fixes 5937552b42e4 ("rdma: Add "stat qp show" support")
Signed-off-by Andrea Claudi <[email protected]>
---
 rdma/stat.c | 74 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 24 deletions(-)
rdma/stat.c +50 -24
index adfcd34a..66121b12 100644
@@ -222,9 +222,9 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
 	uint32_t cntn, port = 0, pid = 0, qpn, qp_type = 0;
 	struct nlattr *hwc_table, *qp_table;
 	struct nlattr *nla_entry;
-	const char *comm = NULL;
+	char *comm = NULL;
 	bool isfirst;
-	int err;
+	int err, ret;
 
 	if (nla_line[RDMA_NLDEV_ATTR_PORT_INDEX])
 		port = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_PORT_INDEX]);

@@ -232,52 +232,70 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
 	hwc_table = nla_line[RDMA_NLDEV_ATTR_STAT_HWCOUNTERS];
 	qp_table = nla_line[RDMA_NLDEV_ATTR_RES_QP];
 	if (!hwc_table || !qp_table ||
-	    !nla_line[RDMA_NLDEV_ATTR_STAT_COUNTER_ID])
-		return MNL_CB_ERROR;
+	    !nla_line[RDMA_NLDEV_ATTR_STAT_COUNTER_ID]) {
+		ret = MNL_CB_ERROR;
+		goto out;
+	}
 
 	cntn = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_STAT_COUNTER_ID]);
 	if (rd_is_filtered_attr(rd, "cntn", cntn,
-				nla_line[RDMA_NLDEV_ATTR_STAT_COUNTER_ID]))
-		return MNL_CB_OK;
+				nla_line[RDMA_NLDEV_ATTR_STAT_COUNTER_ID])) {
+		ret = MNL_CB_OK;
+		goto out;
+	}
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_TYPE])
 		qp_type = mnl_attr_get_u8(nla_line[RDMA_NLDEV_ATTR_RES_TYPE]);
 
 	if (rd_is_string_filtered_attr(rd, "qp-type", qp_types_to_str(qp_type),
-				       nla_line[RDMA_NLDEV_ATTR_RES_TYPE]))
-		return MNL_CB_OK;
+				       nla_line[RDMA_NLDEV_ATTR_RES_TYPE])) {
+		ret = MNL_CB_OK;
+		goto out;
+	}
 
 	if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
 		pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
 		comm = get_task_name(pid);
 	}
 	if (rd_is_filtered_attr(rd, "pid", pid,
-				nla_line[RDMA_NLDEV_ATTR_RES_PID]))
-		return MNL_CB_OK;
+				nla_line[RDMA_NLDEV_ATTR_RES_PID])) {
+		ret = MNL_CB_OK;
+		goto out;
+	}
 
-	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME])
+	if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+		/* discard const from mnl_attr_get_str */
 		comm = (char *)mnl_attr_get_str(
 			nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
+	}
 
 	mnl_attr_for_each_nested(nla_entry, qp_table) {
 		struct nlattr *qp_line[RDMA_NLDEV_ATTR_MAX] = {};
 
 		err = mnl_attr_parse_nested(nla_entry, rd_attr_cb, qp_line);
-		if (err != MNL_CB_OK)
-			return -EINVAL;
+		if (err != MNL_CB_OK) {
+			ret = -EINVAL;
+			goto out;
+		}
 
-		if (!qp_line[RDMA_NLDEV_ATTR_RES_LQPN])
-			return -EINVAL;
+		if (!qp_line[RDMA_NLDEV_ATTR_RES_LQPN]) {
+			ret = -EINVAL;
+			goto out;
+		}
 
 		qpn = mnl_attr_get_u32(qp_line[RDMA_NLDEV_ATTR_RES_LQPN]);
 		if (rd_is_filtered_attr(rd, "lqpn", qpn,
-					qp_line[RDMA_NLDEV_ATTR_RES_LQPN]))
-			return MNL_CB_OK;
+					qp_line[RDMA_NLDEV_ATTR_RES_LQPN])) {
+			ret = MNL_CB_OK;
+			goto out;
+		}
 	}
 
 	err = res_get_hwcounters(rd, hwc_table, false);
-	if (err != MNL_CB_OK)
-		return err;
+	if (err != MNL_CB_OK) {
+		ret = err;
+		goto out;
+	}
 	open_json_object(NULL);
 	print_link(rd, index, name, port, nla_line);
 	print_color_uint(PRINT_ANY, COLOR_NONE, "cntn", "cntn %u ", cntn);

@@ -292,11 +310,15 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
 	mnl_attr_for_each_nested(nla_entry, qp_table) {
 		struct nlattr *qp_line[RDMA_NLDEV_ATTR_MAX] = {};
 		err = mnl_attr_parse_nested(nla_entry, rd_attr_cb, qp_line);
-		if (err != MNL_CB_OK)
-			return -EINVAL;
+		if (err != MNL_CB_OK) {
+			ret = -EINVAL;
+			goto out;
+		}
 
-		if (!qp_line[RDMA_NLDEV_ATTR_RES_LQPN])
-			return -EINVAL;
+		if (!qp_line[RDMA_NLDEV_ATTR_RES_LQPN]) {
+			ret = -EINVAL;
+			goto out;
+		}
 
 		qpn = mnl_attr_get_u32(qp_line[RDMA_NLDEV_ATTR_RES_LQPN]);
 		if (!isfirst)

@@ -307,7 +329,11 @@ static int res_counter_line(struct rd *rd, const char *name, int index,
 	}
 	close_json_array(PRINT_ANY, ">");
 	newline(rd);
-	return MNL_CB_OK;
+	ret = MNL_CB_OK;
+out:
+	if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
+		free(comm);
+	return ret;
 }
 
 static int stat_qp_show_parse_cb(const struct nlmsghdr *nlh, void *data)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH 1/3 lib/fs: fix memory leak in get_task_name()

In reply to Andrea Claudi

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 <[email protected]>
---
 lib/fs.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
lib/fs.c +7 -3
index f6f5f8a0..5692e2d3 100644
@@ -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);
Signature · 1 line
2.34.1
#2 lore raw

Re: [PATCH iproute2 1/3] lib/fs: fix memory leak in get_task_name()

lib/fs.c char *get_task_name(pid_t pid) · line 364

Quoted 3 lines · On Tue, 1 Feb 2022 18:39:24 +0100 Andrea Claudi <[email protected]> wrote:
lib/fs.c +3 -0
+	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.

author
#3 lore raw

Re: [PATCH iproute2 1/3] lib/fs: fix memory leak in get_task_name()

Quoted 9 lines · Stephen Hemminger

lib/fs.c char *get_task_name(pid_t pid) · line 364

Quoted 3 lines · On Tue, 1 Feb 2022 18:39:24 +0100 Andrea Claudi <[email protected]> wrote:
lib/fs.c +3 -0
+	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 :(

Quoted 4 lines

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().

#4 lore raw

Re: [PATCH iproute2 1/3] lib/fs: fix memory leak in get_task_name()

Quoted 18 lines · On Wed, 2 Feb 2022 15:38:16 +0100 Andrea Claudi <[email protected]> wrote:
Quoted 9 lines · Stephen Hemminger

lib/fs.c char *get_task_name(pid_t pid) · line 364

Quoted 3 lines · On Tue, 1 Feb 2022 18:39:24 +0100 Andrea Claudi <[email protected]> wrote:
lib/fs.c +3 -0
+	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 :(

Quoted 4 lines

Why not change it to use a local variable for the path (avoid asprintf) and not reuse comm for both pathname and return value.

Quoted 5 lines

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.