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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
On Sun, Nov 28, 2021 at 7:47 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
>
On Mon, Nov 29, 2021 at 03:38:28PM -0800, Ian Rogers wrote:
> On Sun, Nov 28, 2021 at 7:47 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
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
> > >
> >
>
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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
Thanks, applied.
- Arnaldo