* [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 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).