[PATCH v3] tools/mm/page_owner_sort: free per-record allocations

[PATCH v3] tools/mm/page_owner_sort: free per-record allocations

[Yichong Chen]

add_list() allocates comm and txt for each page owner record, but the
cleanup path only frees the outer list array. This leaks both buffers for
every retained record.

Free partial allocations in add_list(), discarded records during
culling, and retained records on exit. Return explicit error, skip, and
match results from filter_record() to handle get_comm() failures.

Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
Changes in v3:
- Keep the free in add_list(), since incomplete records are not included in
  list_size and cannot be reached by the common cleanup path.
- Return explicit error, skip, and match results from filter_record() to
  handle a NULL return from get_comm().

Changes in v2:
- Wrap commit message lines to approximately 75 columns.
- Use "Yichong Chen" as the author name.

 tools/mm/page_owner_sort.c | 61 +++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 11 deletions(-)

diff --git a/tools/mm/page_owner_sort.c b/tools/mm/page_owner_sort.c
index e6954909401c..cdf31985287c 100644
--- a/tools/mm/page_owner_sort.c
+++ b/tools/mm/page_owner_sort.c
@@ -43,6 +43,13 @@ enum FILTER_BIT {
 	FILTER_TGID = 1<<2,
 	FILTER_COMM = 1<<3
 };
+
+enum FILTER_RESULT {
+	FILTER_ERROR,
+	FILTER_SKIP,
+	FILTER_MATCH
+};
+
 enum CULL_BIT {
 	CULL_PID = 1<<1,
 	CULL_TGID = 1<<2,
@@ -372,6 +379,9 @@ static char *get_comm(char *buf)
 {
 	char *comm_str = malloc(TASK_COMM_LEN);
 
+	if (!comm_str)
+		return NULL;
+
 	memset(comm_str, 0, TASK_COMM_LEN);
 
 	search_pattern(&comm_pattern, comm_str, buf);
@@ -386,6 +396,12 @@ static char *get_comm(char *buf)
 	return comm_str;
 }
 
+static void free_block_list(struct block_list *block)
+{
+	free(block->comm);
+	free(block->txt);
+}
+
 static int get_arg_type(const char *arg)
 {
 	if (!strcmp(arg, "pid") || !strcmp(arg, "p"))
@@ -450,39 +466,57 @@ static bool match_str_list(const char *str, char **list, int list_size)
 	return false;
 }
 
-static bool is_need(char *buf)
+static enum FILTER_RESULT filter_record(char *buf)
 {
+	char *comm;
+
 	if ((filter & FILTER_PID) && !match_num_list(get_pid(buf), fc.pids, fc.pids_size))
-		return false;
+		return FILTER_SKIP;
 	if ((filter & FILTER_TGID) &&
 		!match_num_list(get_tgid(buf), fc.tgids, fc.tgids_size))
-		return false;
+		return FILTER_SKIP;
+	if (!(filter & FILTER_COMM))
+		return FILTER_MATCH;
 
-	char *comm = get_comm(buf);
+	comm = get_comm(buf);
+	if (!comm)
+		return FILTER_ERROR;
 
-	if ((filter & FILTER_COMM) &&
-	!match_str_list(comm, fc.comms, fc.comms_size)) {
+	if (!match_str_list(comm, fc.comms, fc.comms_size)) {
 		free(comm);
-		return false;
+		return FILTER_SKIP;
 	}
 	free(comm);
-	return true;
+	return FILTER_MATCH;
 }
 
 static bool add_list(char *buf, int len, char *ext_buf)
 {
+	enum FILTER_RESULT filter_result;
+
 	if (list_size == max_size) {
 		fprintf(stderr, "max_size too small??\n");
 		return false;
 	}
-	if (!is_need(buf))
+	filter_result = filter_record(buf);
+	if (filter_result == FILTER_ERROR) {
+		fprintf(stderr, "Out of memory\n");
+		return false;
+	}
+	if (filter_result == FILTER_SKIP)
 		return true;
+
 	list[list_size].pid = get_pid(buf);
 	list[list_size].tgid = get_tgid(buf);
 	list[list_size].comm = get_comm(buf);
-	list[list_size].txt = malloc(len+1);
+	if (!list[list_size].comm) {
+		fprintf(stderr, "Out of memory\n");
+		return false;
+	}
+	list[list_size].txt = malloc(len + 1);
 	if (!list[list_size].txt) {
 		fprintf(stderr, "Out of memory\n");
+		free(list[list_size].comm);
 		return false;
 	}
 	memcpy(list[list_size].txt, buf, len);
@@ -841,8 +875,10 @@ int main(int argc, char **argv)
 		} else {
 			list[count-1].num += list[i].num;
 			list[count-1].page_num += list[i].page_num;
+			free_block_list(&list[i]);
 		}
 	}
+	list_size = count;
 
 	qsort(list, count, sizeof(list[0]), compare_sort_condition);
 
@@ -876,8 +912,11 @@ int main(int argc, char **argv)
 		free(ext_buf);
 	if (buf)
 		free(buf);
-	if (list)
+	if (list) {
+		for (i = 0; i < list_size; i++)
+			free_block_list(&list[i]);
 		free(list);
+	}
 out_ts:
 	regfree(&ts_nsec_pattern);
 out_comm:
-- 
2.51.0

Re: [PATCH v3] tools/mm/page_owner_sort: free per-record allocations

[Vishal Moola]

On Fri, Jun 12, 2026 at 01:50:17PM +0800, Yichong Chen wrote:
> add_list() allocates comm and txt for each page owner record, but the
> cleanup path only frees the outer list array. This leaks both buffers for
> every retained record.
> 
> Free partial allocations in add_list(), discarded records during
> culling, and retained records on exit. Return explicit error, skip, and
> match results from filter_record() to handle get_comm() failures.

Nit: IMO its bad practice to rename function names without stating it in
the commit log. Especially if you reference its behavior in that commit
log.

> 
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
> ---

I like the enum approach. Splitting it out and having this as a 2-patch
series would have made sense too (since doing it this way has
2 distinct logical changes).

Whether combined or split, you can add my tag:

Reviewed-by: Vishal Moola <vishal.moola@gmail.com>