* [PATCH] perf header: Fix memory leaks @ 2021-11-18 20:17 Ian Rogers 2021-11-28 15:46 ` Jiri Olsa 0 siblings, 1 reply; 5+ messages in thread From: Ian Rogers @ 2021-11-18 20:17 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-perf-users, linux-kernel Cc: Stephane Eranian, Ian Rogers These leaks were found with leak sanitizer running "perf pipe recording and injection test". In pipe mode feat_fd may hold onto an events struct that needs freeing. When string features are processed they may overwrite an already created string, so free this before the overwrite. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/header.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c index 79cce216727e..e3c1a532d059 100644 --- a/tools/perf/util/header.c +++ b/tools/perf/util/header.c @@ -2321,6 +2321,7 @@ static int perf_header__read_build_ids(struct perf_header *header, #define FEAT_PROCESS_STR_FUN(__feat, __feat_env) \ static int process_##__feat(struct feat_fd *ff, void *data __maybe_unused) \ {\ + free(ff->ph->env.__feat_env); \ ff->ph->env.__feat_env = do_read_string(ff); \ return ff->ph->env.__feat_env ? 0 : -ENOMEM; \ } @@ -4124,6 +4125,7 @@ int perf_event__process_feature(struct perf_session *session, struct perf_record_header_feature *fe = (struct perf_record_header_feature *)event; int type = fe->header.type; u64 feat = fe->feat_id; + int ret = 0; if (type < 0 || type >= PERF_RECORD_HEADER_MAX) { pr_warning("invalid record type %d in pipe-mode\n", type); @@ -4141,11 +4143,13 @@ int perf_event__process_feature(struct perf_session *session, ff.size = event->header.size - sizeof(*fe); ff.ph = &session->header; - if (feat_ops[feat].process(&ff, NULL)) - return -1; + if (feat_ops[feat].process(&ff, NULL)) { + ret = -1; + goto out; + } if (!feat_ops[feat].print || !tool->show_feat_hdr) - return 0; + goto out; if (!feat_ops[feat].full_only || tool->show_feat_hdr >= SHOW_FEAT_HEADER_FULL_INFO) { @@ -4154,8 +4158,9 @@ int perf_event__process_feature(struct perf_session *session, fprintf(stdout, "# %s info available, use -I to display\n", feat_ops[feat].name); } - - return 0; +out: + free_event_desc(ff.events); + return ret; } size_t perf_event__fprintf_event_update(union perf_event *event, FILE *fp) -- 2.34.0.rc2.393.gf8c9666880-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] perf header: Fix memory leaks 2021-11-18 20:17 [PATCH] perf header: Fix memory leaks Ian Rogers @ 2021-11-28 15:46 ` Jiri Olsa 2021-11-29 23:38 ` Ian Rogers 0 siblings, 1 reply; 5+ messages in thread From: Jiri Olsa @ 2021-11-28 15:46 UTC (permalink / raw) To: Ian Rogers Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, linux-perf-users, linux-kernel, Stephane Eranian On Thu, Nov 18, 2021 at 12:17:30PM -0800, Ian Rogers wrote: > These leaks were found with leak sanitizer running "perf pipe recording > and injection test". In pipe mode feat_fd may hold onto an events struct > that needs freeing. When string features are processed they may > overwrite an already created string, so free this before the overwrite. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/header.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > index 79cce216727e..e3c1a532d059 100644 > --- a/tools/perf/util/header.c > +++ b/tools/perf/util/header.c > @@ -2321,6 +2321,7 @@ static int perf_header__read_build_ids(struct perf_header *header, > #define FEAT_PROCESS_STR_FUN(__feat, __feat_env) \ > static int process_##__feat(struct feat_fd *ff, void *data __maybe_unused) \ > {\ > + free(ff->ph->env.__feat_env); \ hm, how is this set before this callback is triggered? jirka > ff->ph->env.__feat_env = do_read_string(ff); \ > return ff->ph->env.__feat_env ? 0 : -ENOMEM; \ > } > @@ -4124,6 +4125,7 @@ int perf_event__process_feature(struct perf_session *session, > struct perf_record_header_feature *fe = (struct perf_record_header_feature *)event; > int type = fe->header.type; > u64 feat = fe->feat_id; > + int ret = 0; > > if (type < 0 || type >= PERF_RECORD_HEADER_MAX) { > pr_warning("invalid record type %d in pipe-mode\n", type); > @@ -4141,11 +4143,13 @@ int perf_event__process_feature(struct perf_session *session, > ff.size = event->header.size - sizeof(*fe); > ff.ph = &session->header; > > - if (feat_ops[feat].process(&ff, NULL)) > - return -1; > + if (feat_ops[feat].process(&ff, NULL)) { > + ret = -1; > + goto out; > + } > > if (!feat_ops[feat].print || !tool->show_feat_hdr) > - return 0; > + goto out; > > if (!feat_ops[feat].full_only || > tool->show_feat_hdr >= SHOW_FEAT_HEADER_FULL_INFO) { > @@ -4154,8 +4158,9 @@ int perf_event__process_feature(struct perf_session *session, > fprintf(stdout, "# %s info available, use -I to display\n", > feat_ops[feat].name); > } > - > - return 0; > +out: > + free_event_desc(ff.events); > + return ret; > } > > size_t perf_event__fprintf_event_update(union perf_event *event, FILE *fp) > -- > 2.34.0.rc2.393.gf8c9666880-goog > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf header: Fix memory leaks 2021-11-28 15:46 ` Jiri Olsa @ 2021-11-29 23:38 ` Ian Rogers 2021-11-30 18:15 ` Jiri Olsa 0 siblings, 1 reply; 5+ messages in thread From: Ian Rogers @ 2021-11-29 23:38 UTC (permalink / raw) To: Jiri Olsa Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, linux-perf-users, linux-kernel, Stephane Eranian On Sun, Nov 28, 2021 at 7:47 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Nov 18, 2021 at 12:17:30PM -0800, Ian Rogers wrote: > > These leaks were found with leak sanitizer running "perf pipe recording > > and injection test". In pipe mode feat_fd may hold onto an events struct > > that needs freeing. When string features are processed they may > > overwrite an already created string, so free this before the overwrite. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/header.c | 15 ++++++++++----- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > > index 79cce216727e..e3c1a532d059 100644 > > --- a/tools/perf/util/header.c > > +++ b/tools/perf/util/header.c > > @@ -2321,6 +2321,7 @@ static int perf_header__read_build_ids(struct perf_header *header, > > #define FEAT_PROCESS_STR_FUN(__feat, __feat_env) \ > > static int process_##__feat(struct feat_fd *ff, void *data __maybe_unused) \ > > {\ > > + free(ff->ph->env.__feat_env); \ > > hm, how is this set before this callback is triggered? I see it for cpuid which is initially set in: #0 perf_env__read_cpuid (env=0x62b000007240) at util/env.c:363 #1 0x0000555556325153 in perf_env__cpuid (env=0x62b000007240) at util/env.c:456 #2 0x00005555564002ff in evlist__init_trace_event_sample_raw (evlist=0x61e000000080) at util/sample-raw.c:17 #3 0x00005555563f01f4 in __perf_session__new (data=0x7fffffffb8b0, repipe=false, repipe_fd=-1, tool=0x7fffffffbaa0) at util/session.c:228 #4 0x000055555615f26b in perf_session__new (data=0x7fffffffb8b0, tool=0x7fffffffbaa0) at util/session.h:70 #5 0x000055555616d991 in cmd_report (argc=0, argv=0x7fffffffe468) at builtin-report.c:1408 #6 0x00005555562f36b8 in run_builtin (p=0x5555586bacd0 <commands+240>, argc=5, argv=0x7fffffffe468) at perf.c:313 #7 0x00005555562f3c11 in handle_internal_command (argc=5, argv=0x7fffffffe468) at perf.c:365 #8 0x00005555562f3fce in run_argv (argcp=0x7fffffffe240, argv=0x7fffffffe250) at perf.c:409 #9 0x00005555562f47bd in main (argc=5, argv=0x7fffffffe468) at perf.c:539 And then overwritten causing the leak: #0 0x00005555563b965a in process_cpuid (ff=0x7fffffffad50, data=0x0) at util/header.c:2333 #1 0x00005555563c53d2 in perf_event__process_feature (session=0x62b000007200, event=0x621000006500) at util/header.c:4144 #2 0x000055555615fdef in process_feature_event (session=0x62b000007200, event=0x621000006500) at builtin-report.c:230 #3 0x00005555563fa033 in perf_session__process_user_event (session=0x62b000007200, event=0x621000006500, file_offset=868) at util/session.c:1668 #4 0x00005555563fae08 in perf_session__process_event (session=0x62b000007200, event=0x621000006500, file_offset=868) at util/session.c:1803 #5 0x00005555563fc4e6 in __perf_session__process_pipe_events (session=0x62b000007200) at util/session.c:2044 #6 0x00005555563ff005 in perf_session__process_events (session=0x62b000007200) at util/session.c:2418 #7 0x000055555616508a in __cmd_report (rep=0x7fffffffbaa0) at builtin-report.c:940 #8 0x000055555616f5c9 in cmd_report (argc=0, argv=0x7fffffffe468) at builtin-report.c:1629 #9 0x00005555562f36b8 in run_builtin (p=0x5555586bacd0 <commands+240>, argc=5, argv=0x7fffffffe468) at perf.c:313 #10 0x00005555562f3c11 in handle_internal_command (argc=5, argv=0x7fffffffe468) at perf.c:365 #11 0x00005555562f3fce in run_argv (argcp=0x7fffffffe240, argv=0x7fffffffe250) at perf.c:409 #12 0x00005555562f47bd in main (argc=5, argv=0x7fffffffe468) at perf.c:539 Thanks, Ian > jirka > > > ff->ph->env.__feat_env = do_read_string(ff); \ > > return ff->ph->env.__feat_env ? 0 : -ENOMEM; \ > > } > > @@ -4124,6 +4125,7 @@ int perf_event__process_feature(struct perf_session *session, > > struct perf_record_header_feature *fe = (struct perf_record_header_feature *)event; > > int type = fe->header.type; > > u64 feat = fe->feat_id; > > + int ret = 0; > > > > if (type < 0 || type >= PERF_RECORD_HEADER_MAX) { > > pr_warning("invalid record type %d in pipe-mode\n", type); > > @@ -4141,11 +4143,13 @@ int perf_event__process_feature(struct perf_session *session, > > ff.size = event->header.size - sizeof(*fe); > > ff.ph = &session->header; > > > > - if (feat_ops[feat].process(&ff, NULL)) > > - return -1; > > + if (feat_ops[feat].process(&ff, NULL)) { > > + ret = -1; > > + goto out; > > + } > > > > if (!feat_ops[feat].print || !tool->show_feat_hdr) > > - return 0; > > + goto out; > > > > if (!feat_ops[feat].full_only || > > tool->show_feat_hdr >= SHOW_FEAT_HEADER_FULL_INFO) { > > @@ -4154,8 +4158,9 @@ int perf_event__process_feature(struct perf_session *session, > > fprintf(stdout, "# %s info available, use -I to display\n", > > feat_ops[feat].name); > > } > > - > > - return 0; > > +out: > > + free_event_desc(ff.events); > > + return ret; > > } > > > > size_t perf_event__fprintf_event_update(union perf_event *event, FILE *fp) > > -- > > 2.34.0.rc2.393.gf8c9666880-goog > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf header: Fix memory leaks 2021-11-29 23:38 ` Ian Rogers @ 2021-11-30 18:15 ` Jiri Olsa 2021-11-30 19:22 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 5+ messages in thread From: Jiri Olsa @ 2021-11-30 18:15 UTC (permalink / raw) To: Ian Rogers Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, linux-perf-users, linux-kernel, Stephane Eranian On Mon, Nov 29, 2021 at 03:38:28PM -0800, Ian Rogers wrote: > On Sun, Nov 28, 2021 at 7:47 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Thu, Nov 18, 2021 at 12:17:30PM -0800, Ian Rogers wrote: > > > These leaks were found with leak sanitizer running "perf pipe recording > > > and injection test". In pipe mode feat_fd may hold onto an events struct > > > that needs freeing. When string features are processed they may > > > overwrite an already created string, so free this before the overwrite. > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > --- > > > tools/perf/util/header.c | 15 ++++++++++----- > > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > > > index 79cce216727e..e3c1a532d059 100644 > > > --- a/tools/perf/util/header.c > > > +++ b/tools/perf/util/header.c > > > @@ -2321,6 +2321,7 @@ static int perf_header__read_build_ids(struct perf_header *header, > > > #define FEAT_PROCESS_STR_FUN(__feat, __feat_env) \ > > > static int process_##__feat(struct feat_fd *ff, void *data __maybe_unused) \ > > > {\ > > > + free(ff->ph->env.__feat_env); \ > > > > hm, how is this set before this callback is triggered? > > I see it for cpuid which is initially set in: > #0 perf_env__read_cpuid (env=0x62b000007240) at util/env.c:363 > #1 0x0000555556325153 in perf_env__cpuid (env=0x62b000007240) at util/env.c:456 > #2 0x00005555564002ff in evlist__init_trace_event_sample_raw ok, I forgot we do this one, thanks for explanation Acked-by: Jiri Olsa <jolsa@redhat.com> jirka > (evlist=0x61e000000080) at util/sample-raw.c:17 > #3 0x00005555563f01f4 in __perf_session__new (data=0x7fffffffb8b0, > repipe=false, repipe_fd=-1, tool=0x7fffffffbaa0) > at util/session.c:228 > #4 0x000055555615f26b in perf_session__new (data=0x7fffffffb8b0, > tool=0x7fffffffbaa0) at util/session.h:70 > #5 0x000055555616d991 in cmd_report (argc=0, argv=0x7fffffffe468) at > builtin-report.c:1408 > #6 0x00005555562f36b8 in run_builtin (p=0x5555586bacd0 > <commands+240>, argc=5, argv=0x7fffffffe468) at perf.c:313 > #7 0x00005555562f3c11 in handle_internal_command (argc=5, > argv=0x7fffffffe468) at perf.c:365 > #8 0x00005555562f3fce in run_argv (argcp=0x7fffffffe240, > argv=0x7fffffffe250) at perf.c:409 > #9 0x00005555562f47bd in main (argc=5, argv=0x7fffffffe468) at perf.c:539 > > And then overwritten causing the leak: > #0 0x00005555563b965a in process_cpuid (ff=0x7fffffffad50, data=0x0) > at util/header.c:2333 > #1 0x00005555563c53d2 in perf_event__process_feature > (session=0x62b000007200, event=0x621000006500) at util/header.c:4144 > #2 0x000055555615fdef in process_feature_event > (session=0x62b000007200, event=0x621000006500) at builtin-report.c:230 > #3 0x00005555563fa033 in perf_session__process_user_event > (session=0x62b000007200, event=0x621000006500, file_offset=868) > at util/session.c:1668 > #4 0x00005555563fae08 in perf_session__process_event > (session=0x62b000007200, event=0x621000006500, file_offset=868) > at util/session.c:1803 > #5 0x00005555563fc4e6 in __perf_session__process_pipe_events > (session=0x62b000007200) at util/session.c:2044 > #6 0x00005555563ff005 in perf_session__process_events > (session=0x62b000007200) at util/session.c:2418 > #7 0x000055555616508a in __cmd_report (rep=0x7fffffffbaa0) at > builtin-report.c:940 > #8 0x000055555616f5c9 in cmd_report (argc=0, argv=0x7fffffffe468) at > builtin-report.c:1629 > #9 0x00005555562f36b8 in run_builtin (p=0x5555586bacd0 > <commands+240>, argc=5, argv=0x7fffffffe468) at perf.c:313 > #10 0x00005555562f3c11 in handle_internal_command (argc=5, > argv=0x7fffffffe468) at perf.c:365 > #11 0x00005555562f3fce in run_argv (argcp=0x7fffffffe240, > argv=0x7fffffffe250) at perf.c:409 > #12 0x00005555562f47bd in main (argc=5, argv=0x7fffffffe468) at perf.c:539 > > Thanks, > Ian > > > jirka > > > > > ff->ph->env.__feat_env = do_read_string(ff); \ > > > return ff->ph->env.__feat_env ? 0 : -ENOMEM; \ > > > } > > > @@ -4124,6 +4125,7 @@ int perf_event__process_feature(struct perf_session *session, > > > struct perf_record_header_feature *fe = (struct perf_record_header_feature *)event; > > > int type = fe->header.type; > > > u64 feat = fe->feat_id; > > > + int ret = 0; > > > > > > if (type < 0 || type >= PERF_RECORD_HEADER_MAX) { > > > pr_warning("invalid record type %d in pipe-mode\n", type); > > > @@ -4141,11 +4143,13 @@ int perf_event__process_feature(struct perf_session *session, > > > ff.size = event->header.size - sizeof(*fe); > > > ff.ph = &session->header; > > > > > > - if (feat_ops[feat].process(&ff, NULL)) > > > - return -1; > > > + if (feat_ops[feat].process(&ff, NULL)) { > > > + ret = -1; > > > + goto out; > > > + } > > > > > > if (!feat_ops[feat].print || !tool->show_feat_hdr) > > > - return 0; > > > + goto out; > > > > > > if (!feat_ops[feat].full_only || > > > tool->show_feat_hdr >= SHOW_FEAT_HEADER_FULL_INFO) { > > > @@ -4154,8 +4158,9 @@ int perf_event__process_feature(struct perf_session *session, > > > fprintf(stdout, "# %s info available, use -I to display\n", > > > feat_ops[feat].name); > > > } > > > - > > > - return 0; > > > +out: > > > + free_event_desc(ff.events); > > > + return ret; > > > } > > > > > > size_t perf_event__fprintf_event_update(union perf_event *event, FILE *fp) > > > -- > > > 2.34.0.rc2.393.gf8c9666880-goog > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf header: Fix memory leaks 2021-11-30 18:15 ` Jiri Olsa @ 2021-11-30 19:22 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 5+ messages in thread From: Arnaldo Carvalho de Melo @ 2021-11-30 19:22 UTC (permalink / raw) To: Jiri Olsa Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim, linux-perf-users, linux-kernel, Stephane Eranian Em Tue, Nov 30, 2021 at 07:15:19PM +0100, Jiri Olsa escreveu: > On Mon, Nov 29, 2021 at 03:38:28PM -0800, Ian Rogers wrote: > > On Sun, Nov 28, 2021 at 7:47 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Thu, Nov 18, 2021 at 12:17:30PM -0800, Ian Rogers wrote: > > > > These leaks were found with leak sanitizer running "perf pipe recording > > > > and injection test". In pipe mode feat_fd may hold onto an events struct > > > > that needs freeing. When string features are processed they may > > > > overwrite an already created string, so free this before the overwrite. > > > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > > --- > > > > tools/perf/util/header.c | 15 ++++++++++----- > > > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > > > > index 79cce216727e..e3c1a532d059 100644 > > > > --- a/tools/perf/util/header.c > > > > +++ b/tools/perf/util/header.c > > > > @@ -2321,6 +2321,7 @@ static int perf_header__read_build_ids(struct perf_header *header, > > > > #define FEAT_PROCESS_STR_FUN(__feat, __feat_env) \ > > > > static int process_##__feat(struct feat_fd *ff, void *data __maybe_unused) \ > > > > {\ > > > > + free(ff->ph->env.__feat_env); \ > > > > > > hm, how is this set before this callback is triggered? > > > > I see it for cpuid which is initially set in: > > #0 perf_env__read_cpuid (env=0x62b000007240) at util/env.c:363 > > #1 0x0000555556325153 in perf_env__cpuid (env=0x62b000007240) at util/env.c:456 > > #2 0x00005555564002ff in evlist__init_trace_event_sample_raw > > ok, I forgot we do this one, thanks for explanation > > Acked-by: Jiri Olsa <jolsa@redhat.com> Thanks, applied. - Arnaldo ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-30 19:22 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-18 20:17 [PATCH] perf header: Fix memory leaks Ian Rogers 2021-11-28 15:46 ` Jiri Olsa 2021-11-29 23:38 ` Ian Rogers 2021-11-30 18:15 ` Jiri Olsa 2021-11-30 19:22 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.