* [PATCH 1/1] perf script: Fix allocation of evsel->priv related to per-event dump files
@ 2023-06-06 19:15 Arnaldo Carvalho de Melo
2023-06-06 23:14 ` Ian Rogers
0 siblings, 1 reply; 3+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-06-06 19:15 UTC (permalink / raw)
To: Ravi Bangoria
Cc: Ian Rogers, Adrian Hunter, Jiri Olsa, Namhyung Kim,
Linux Kernel Mailing List
I'm carrying this in my perf-tools-next, please ack :-)
- Arnaldo
---
When printing output we may want to generate per event files, where the
--per-event-dump option should be used, creating perf.data.EVENT.dump
files instead of printing to stdout.
The callback thar processes event thus expects that evsel->priv->fp
should point to either the per-event FILE descriptor or to stdout.
The a3af66f51bd0bca7 ("perf script: Fix crash because of missing
evsel->priv") changeset fixed a case where evsel->priv wasn't setup,
thus set to NULL, causing a segfault when trying to access
evsel->priv->fp.
But it did it for the non --per-event-dump case by allocating a 'struct
perf_evsel_script' just to set its ->fp to stdout.
Since evsel->priv is only freed when --per-event-dump is used, we ended
up with a memory leek, detected using ASAN.
Fix it by using the same method as perf_script__setup_per_event_dump(),
and reuse that static 'struct perf_evsel_script'.
Also check if evsel_script__new() failed.
Fixes: a3af66f51bd0bca7 ("perf script: Fix crash because of missing evsel->priv")
Reported-by: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-script.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 70549fc93b125394..b02ad386a55baf07 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -2410,6 +2410,9 @@ static int process_sample_event(struct perf_tool *tool,
return ret;
}
+// Used when scr->per_event_dump is not set
+static struct evsel_script es_stdout;
+
static int process_attr(struct perf_tool *tool, union perf_event *event,
struct evlist **pevlist)
{
@@ -2418,7 +2421,6 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
struct evsel *evsel, *pos;
u64 sample_type;
int err;
- static struct evsel_script *es;
err = perf_event__process_attr(tool, event, pevlist);
if (err)
@@ -2428,14 +2430,13 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
evsel = evlist__last(*pevlist);
if (!evsel->priv) {
- if (scr->per_event_dump) {
+ if (scr->per_event_dump) {
evsel->priv = evsel_script__new(evsel, scr->session->data);
- } else {
- es = zalloc(sizeof(*es));
- if (!es)
+ if (!evsel->priv)
return -ENOMEM;
- es->fp = stdout;
- evsel->priv = es;
+ } else { // Replicate what is done in perf_script__setup_per_event_dump()
+ es_stdout.fp = stdout;
+ evsel->priv = &es_stdout;
}
}
@@ -2741,7 +2742,6 @@ static int perf_script__fopen_per_event_dump(struct perf_script *script)
static int perf_script__setup_per_event_dump(struct perf_script *script)
{
struct evsel *evsel;
- static struct evsel_script es_stdout;
if (script->per_event_dump)
return perf_script__fopen_per_event_dump(script);
--
2.37.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] perf script: Fix allocation of evsel->priv related to per-event dump files
2023-06-06 19:15 [PATCH 1/1] perf script: Fix allocation of evsel->priv related to per-event dump files Arnaldo Carvalho de Melo
@ 2023-06-06 23:14 ` Ian Rogers
2023-06-12 18:58 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2023-06-06 23:14 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Ravi Bangoria, Adrian Hunter, Jiri Olsa, Namhyung Kim,
Linux Kernel Mailing List
On Tue, Jun 6, 2023 at 12:15 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> I'm carrying this in my perf-tools-next, please ack :-)
>
> - Arnaldo
>
> ---
>
> When printing output we may want to generate per event files, where the
> --per-event-dump option should be used, creating perf.data.EVENT.dump
> files instead of printing to stdout.
>
> The callback thar processes event thus expects that evsel->priv->fp
> should point to either the per-event FILE descriptor or to stdout.
>
> The a3af66f51bd0bca7 ("perf script: Fix crash because of missing
> evsel->priv") changeset fixed a case where evsel->priv wasn't setup,
> thus set to NULL, causing a segfault when trying to access
> evsel->priv->fp.
>
> But it did it for the non --per-event-dump case by allocating a 'struct
> perf_evsel_script' just to set its ->fp to stdout.
>
> Since evsel->priv is only freed when --per-event-dump is used, we ended
> up with a memory leek, detected using ASAN.
nit: s/leek/leak/
>
> Fix it by using the same method as perf_script__setup_per_event_dump(),
> and reuse that static 'struct perf_evsel_script'.
>
> Also check if evsel_script__new() failed.
>
> Fixes: a3af66f51bd0bca7 ("perf script: Fix crash because of missing evsel->priv")
> Reported-by: Ian Rogers <irogers@google.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested on top of my asan work:
Tested-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/builtin-script.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 70549fc93b125394..b02ad386a55baf07 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -2410,6 +2410,9 @@ static int process_sample_event(struct perf_tool *tool,
> return ret;
> }
>
> +// Used when scr->per_event_dump is not set
> +static struct evsel_script es_stdout;
> +
> static int process_attr(struct perf_tool *tool, union perf_event *event,
> struct evlist **pevlist)
> {
> @@ -2418,7 +2421,6 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
> struct evsel *evsel, *pos;
> u64 sample_type;
> int err;
> - static struct evsel_script *es;
>
> err = perf_event__process_attr(tool, event, pevlist);
> if (err)
> @@ -2428,14 +2430,13 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
> evsel = evlist__last(*pevlist);
>
> if (!evsel->priv) {
> - if (scr->per_event_dump) {
> + if (scr->per_event_dump) {
nit: whitespace issue.
> evsel->priv = evsel_script__new(evsel, scr->session->data);
> - } else {
> - es = zalloc(sizeof(*es));
> - if (!es)
> + if (!evsel->priv)
> return -ENOMEM;
> - es->fp = stdout;
> - evsel->priv = es;
> + } else { // Replicate what is done in perf_script__setup_per_event_dump()
> + es_stdout.fp = stdout;
> + evsel->priv = &es_stdout;
> }
> }
>
> @@ -2741,7 +2742,6 @@ static int perf_script__fopen_per_event_dump(struct perf_script *script)
> static int perf_script__setup_per_event_dump(struct perf_script *script)
> {
> struct evsel *evsel;
> - static struct evsel_script es_stdout;
>
> if (script->per_event_dump)
> return perf_script__fopen_per_event_dump(script);
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] perf script: Fix allocation of evsel->priv related to per-event dump files
2023-06-06 23:14 ` Ian Rogers
@ 2023-06-12 18:58 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 3+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-06-12 18:58 UTC (permalink / raw)
To: Ian Rogers
Cc: Ravi Bangoria, Adrian Hunter, Jiri Olsa, Namhyung Kim,
Linux Kernel Mailing List
Em Tue, Jun 06, 2023 at 04:14:51PM -0700, Ian Rogers escreveu:
> On Tue, Jun 6, 2023 at 12:15 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > I'm carrying this in my perf-tools-next, please ack :-)
> >
> > - Arnaldo
> >
> > ---
> >
> > When printing output we may want to generate per event files, where the
> > --per-event-dump option should be used, creating perf.data.EVENT.dump
> > files instead of printing to stdout.
> >
> > The callback thar processes event thus expects that evsel->priv->fp
> > should point to either the per-event FILE descriptor or to stdout.
> >
> > The a3af66f51bd0bca7 ("perf script: Fix crash because of missing
> > evsel->priv") changeset fixed a case where evsel->priv wasn't setup,
> > thus set to NULL, causing a segfault when trying to access
> > evsel->priv->fp.
> >
> > But it did it for the non --per-event-dump case by allocating a 'struct
> > perf_evsel_script' just to set its ->fp to stdout.
> >
> > Since evsel->priv is only freed when --per-event-dump is used, we ended
> > up with a memory leek, detected using ASAN.
>
> nit: s/leek/leak/
Fixed
> >
> > Fix it by using the same method as perf_script__setup_per_event_dump(),
> > and reuse that static 'struct perf_evsel_script'.
> >
> > Also check if evsel_script__new() failed.
> >
> > Fixes: a3af66f51bd0bca7 ("perf script: Fix crash because of missing evsel->priv")
> > Reported-by: Ian Rogers <irogers@google.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> Tested on top of my asan work:
> Tested-by: Ian Rogers <irogers@google.com>
Thanks,
- Arnaldo
> > ---
> > tools/perf/builtin-script.c | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > index 70549fc93b125394..b02ad386a55baf07 100644
> > --- a/tools/perf/builtin-script.c
> > +++ b/tools/perf/builtin-script.c
> > @@ -2410,6 +2410,9 @@ static int process_sample_event(struct perf_tool *tool,
> > return ret;
> > }
> >
> > +// Used when scr->per_event_dump is not set
> > +static struct evsel_script es_stdout;
> > +
> > static int process_attr(struct perf_tool *tool, union perf_event *event,
> > struct evlist **pevlist)
> > {
> > @@ -2418,7 +2421,6 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
> > struct evsel *evsel, *pos;
> > u64 sample_type;
> > int err;
> > - static struct evsel_script *es;
> >
> > err = perf_event__process_attr(tool, event, pevlist);
> > if (err)
> > @@ -2428,14 +2430,13 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
> > evsel = evlist__last(*pevlist);
> >
> > if (!evsel->priv) {
> > - if (scr->per_event_dump) {
> > + if (scr->per_event_dump) {
>
> nit: whitespace issue.
>
> > evsel->priv = evsel_script__new(evsel, scr->session->data);
> > - } else {
> > - es = zalloc(sizeof(*es));
> > - if (!es)
> > + if (!evsel->priv)
> > return -ENOMEM;
> > - es->fp = stdout;
> > - evsel->priv = es;
> > + } else { // Replicate what is done in perf_script__setup_per_event_dump()
> > + es_stdout.fp = stdout;
> > + evsel->priv = &es_stdout;
> > }
> > }
> >
> > @@ -2741,7 +2742,6 @@ static int perf_script__fopen_per_event_dump(struct perf_script *script)
> > static int perf_script__setup_per_event_dump(struct perf_script *script)
> > {
> > struct evsel *evsel;
> > - static struct evsel_script es_stdout;
> >
> > if (script->per_event_dump)
> > return perf_script__fopen_per_event_dump(script);
> > --
> > 2.37.1
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-12 18:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-06 19:15 [PATCH 1/1] perf script: Fix allocation of evsel->priv related to per-event dump files Arnaldo Carvalho de Melo
2023-06-06 23:14 ` Ian Rogers
2023-06-12 18:58 ` 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.