* [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>
---
| 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
--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.