Received-SPF: pass (google.com: domain of linux-kernel+bounces-164606-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 30 Apr 2024 11:41:56 -0700
Message-Id: <20240430184156.1824083-1-irogers@google.com>
Precedence: bulk
Mime-Version: 1.0
Subject: [PATCH v1] perf lock: More strdup argument freeing
From: Ian Rogers <irogers@google.com>
To: zhaimingbing <zhaimingbing@cmss.chinamobile.com>, 
	Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Namhyung Kim <namhyung@kernel.org>, 
	Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Ian Rogers <irogers@google.com>, Adrian Hunter <adrian.hunter@intel.com>, 
	Kan Liang <kan.liang@linux.intel.com>, linux-perf-users@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Leak sanitizer complains about the strdup-ed arguments not being
freed. rec_argv is reordered and duplicates inserted, meaning making
all its contents strdup-ed and freeing them all leads to double frees
or leaks. Add an extra array to track strup-ed arguments and free
them. This makes address sanitier running `perf test` "kernel lock
contention analysis test" memory leak free.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-lock.c | 44 +++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 230461280e45..26c059397cdf 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -2230,10 +2230,11 @@ static int __cmd_record(int argc, const char **argv)
 	const char *callgraph_args[] = {
 		"--call-graph", "fp," __stringify(CONTENTION_STACK_DEPTH),
 	};
-	unsigned int rec_argc, i, j, ret;
+	unsigned int rec_argc, i, j, dups = 0, ret;
 	unsigned int nr_tracepoints;
 	unsigned int nr_callgraph_args = 0;
 	const char **rec_argv;
+	char **to_free;
 	bool has_lock_stat = true;
 
 	for (i = 0; i < ARRAY_SIZE(lock_tracepoints); i++) {
@@ -2270,28 +2271,25 @@ static int __cmd_record(int argc, const char **argv)
 	/* factor of 2 is for -e in front of each tracepoint */
 	rec_argc += 2 * nr_tracepoints;
 
-	rec_argv = calloc(rec_argc + 1, sizeof(char *));
+	rec_argv = calloc(rec_argc + 1, sizeof(*rec_argv));
 	if (!rec_argv)
 		return -ENOMEM;
 
-	for (i = 0; i < ARRAY_SIZE(record_args); i++)
-		rec_argv[i] = strdup(record_args[i]);
-
-	for (j = 0; j < nr_tracepoints; j++) {
-		const char *ev_name;
+	to_free = calloc(rec_argc, sizeof(*to_free));
+	if (!to_free)
+		return -ENOMEM;
 
-		if (has_lock_stat)
-			ev_name = strdup(lock_tracepoints[j].name);
-		else
-			ev_name = strdup(contention_tracepoints[j].name);
-
-		if (!ev_name) {
-			free(rec_argv);
-			return -ENOMEM;
-		}
 
+	for (i = 0; i < ARRAY_SIZE(record_args);) {
+		to_free[dups] = strdup(record_args[i]);
+		rec_argv[i++] = to_free[dups++];
+	}
+	for (j = 0; j < nr_tracepoints; j++) {
+		to_free[dups] = strdup(has_lock_stat
+				       ? lock_tracepoints[j].name
+				       : contention_tracepoints[j].name);
 		rec_argv[i++] = "-e";
-		rec_argv[i++] = ev_name;
+		rec_argv[i++] = to_free[dups++];
 	}
 
 	for (j = 0; j < nr_callgraph_args; j++, i++)
@@ -2302,7 +2300,17 @@ static int __cmd_record(int argc, const char **argv)
 
 	BUG_ON(i != rec_argc);
 
-	ret = cmd_record(i, rec_argv);
+	for (i = 0; i < dups; i++) {
+		if (to_free[i] == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	}
+	ret = cmd_record(rec_argc, rec_argv);
+out:
+	for (i = 0; i < dups; i++)
+		zfree(&to_free[i]);
+	free(to_free);
 	free(rec_argv);
 	return ret;
 }
-- 
2.45.0.rc0.197.gbae5840b3b-goog