* [PATCH v1] perf lock: More strdup argument freeing
@ 2024-04-30 18:41 Ian Rogers
2024-05-01 14:22 ` James Clark
2024-05-01 19:46 ` Namhyung Kim
0 siblings, 2 replies; 3+ messages in thread
From: Ian Rogers @ 2024-04-30 18:41 UTC (permalink / raw)
To: zhaimingbing, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Kan Liang, linux-perf-users, linux-kernel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v1] perf lock: More strdup argument freeing
2024-04-30 18:41 [PATCH v1] perf lock: More strdup argument freeing Ian Rogers
@ 2024-05-01 14:22 ` James Clark
2024-05-01 19:46 ` Namhyung Kim
1 sibling, 0 replies; 3+ messages in thread
From: James Clark @ 2024-05-01 14:22 UTC (permalink / raw)
To: Ian Rogers
Cc: zhaimingbing, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang,
linux-perf-users, linux-kernel
On 30/04/2024 19:41, Ian Rogers wrote:
> 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>
Reviewed-by: James Clark <james.clark@arm.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1] perf lock: More strdup argument freeing
2024-04-30 18:41 [PATCH v1] perf lock: More strdup argument freeing Ian Rogers
2024-05-01 14:22 ` James Clark
@ 2024-05-01 19:46 ` Namhyung Kim
1 sibling, 0 replies; 3+ messages in thread
From: Namhyung Kim @ 2024-05-01 19:46 UTC (permalink / raw)
To: Ian Rogers
Cc: zhaimingbing, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Adrian Hunter, Kan Liang, linux-perf-users,
linux-kernel
On Tue, Apr 30, 2024 at 11:42 AM Ian Rogers <irogers@google.com> wrote:
>
> 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;
Need to free rec_argv. 'goto out' would be fine.
>
> - 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++];
Now I'm curious why we copy the string in the first place.
Maybe not needed..?
Thanks,
Namhyung
> }
>
> 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-01 19:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-30 18:41 [PATCH v1] perf lock: More strdup argument freeing Ian Rogers
2024-05-01 14:22 ` James Clark
2024-05-01 19:46 ` Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).