[PATCH] perf session: Fix infinite loop on invalid perf.data file

[PATCH] perf session: Fix infinite loop on invalid perf.data file

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

perf-record updates the header in the perf.data file at termination.
Without this update perf-report (and other processing built-ins) it
caused an infinite loop when perf report (or something like) called.

This is because the algorithm in __perf_session__process_events()
depends on the data_size which is read from file header.  Use file
size directly instead in this case to do the best-effort processing.

Cc: David Ahern <dsahern@gmail.com>
Cc: Sonny Rao <sonnyrao@chromium.org>
Signed-off-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/header.c  | 9 +++++++++
 tools/perf/util/session.c | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 26441d0e571b..cedccde6a6b2 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2753,6 +2753,15 @@ int perf_session__read_header(struct perf_session *session)
 	if (perf_file_header__read(&f_header, header, fd) < 0)
 		return -EINVAL;
 
+	/*
+	 * Sanity check that perf.data was written cleanly; data size is
+	 * initialized to 0 and updated only if the on_exit function is run.
+	 * If data size is still 0 then the file contains only partial
+	 * information.  Just warn user and process it as much as it can.
+	 */
+	if (f_header.data.size == 0)
+		pr_warning("Data size is 0. Was the record command properly terminated?\n");
+
 	nr_attrs = f_header.attrs.size / f_header.attr_size;
 	lseek(fd, f_header.attrs.offset, SEEK_SET);
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 6c1d4447c5b4..7bbd60c0a0ed 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1323,7 +1323,7 @@ int __perf_session__process_events(struct perf_session *session,
 	file_offset = page_offset;
 	head = data_offset - page_offset;
 
-	if (data_offset + data_size < file_size)
+	if (data_size && (data_offset + data_size < file_size))
 		file_size = data_offset + data_size;
 
 	progress_next = file_size / 16;
-- 
1.7.11.7

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[David Ahern]

On 9/30/13 2:19 AM, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
>
> perf-record updates the header in the perf.data file at termination.
> Without this update perf-report (and other processing built-ins) it
> caused an infinite loop when perf report (or something like) called.
>
> This is because the algorithm in __perf_session__process_events()
> depends on the data_size which is read from file header.  Use file
> size directly instead in this case to do the best-effort processing.
>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Sonny Rao <sonnyrao@chromium.org>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

worked ok for me. Sonny can you verify?

David

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[Sonny Rao]

On Mon, Sep 30, 2013 at 6:49 AM, David Ahern <dsahern@gmail.com> wrote:
> On 9/30/13 2:19 AM, Namhyung Kim wrote:
>>
>> From: Namhyung Kim <namhyung.kim@lge.com>
>>
>> perf-record updates the header in the perf.data file at termination.
>> Without this update perf-report (and other processing built-ins) it
>> caused an infinite loop when perf report (or something like) called.
>>
>> This is because the algorithm in __perf_session__process_events()
>> depends on the data_size which is read from file header.  Use file
>> size directly instead in this case to do the best-effort processing.
>>
>> Cc: David Ahern <dsahern@gmail.com>
>> Cc: Sonny Rao <sonnyrao@chromium.org>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
>
> worked ok for me. Sonny can you verify?

Yes, it works for me as well, thanks!


> David
>

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2753,6 +2753,15 @@ int perf_session__read_header(struct perf_session *session)
>  	if (perf_file_header__read(&f_header, header, fd) < 0)
>  		return -EINVAL;
>  
> +	/*
> +	 * Sanity check that perf.data was written cleanly; data size is
> +	 * initialized to 0 and updated only if the on_exit function is run.
> +	 * If data size is still 0 then the file contains only partial
> +	 * information.  Just warn user and process it as much as it can.
> +	 */
> +	if (f_header.data.size == 0)
> +		pr_warning("Data size is 0. Was the record command properly terminated?\n");

Just a detail: it would be nice to make all the user facing messages in 
tools/perf/util/header.c more specific and more structured. For example 
prefixing it with 'perf header:' would be fine:

	WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?

or something like that.

etc.

	Ingo

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[David Ahern]

On 10/1/13 1:16 AM, Ingo Molnar wrote:
> Just a detail: it would be nice to make all the user facing messages in
> tools/perf/util/header.c more specific and more structured. For example
> prefixing it with 'perf header:' would be fine:
>
> 	WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
>

Why put code references in the messages? The message is all on one line 
so grep finds it quickly for people working on the code.

David

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> On 10/1/13 1:16 AM, Ingo Molnar wrote:
> >Just a detail: it would be nice to make all the user facing messages in
> >tools/perf/util/header.c more specific and more structured. For example
> >prefixing it with 'perf header:' would be fine:
> >
> >	WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
> >
> 
> Why put code references in the messages? The message is all on one
> line so grep finds it quickly for people working on the code.

I'd agree if this was some internal error that should never really trigger 
(so at most developers see it).

But if I understood it correctly this particular message could trigger for 
regular users of perf as well, of the perf record is terminated in some 
unusual fashion. Regular users might not have the perf code handy (and 
might not know about git grep either).

Thanks,

	Ingo

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[David Ahern]

On 10/1/13 8:21 AM, Ingo Molnar wrote:
> But if I understood it correctly this particular message could trigger for
> regular users of perf as well, of the perf record is terminated in some
> unusual fashion. Regular users might not have the perf code handy (and
> might not know about git grep either).

This is the case I was referring to -- normal users don't care about the 
code reference, hence the more specific question about how the 
perf-record session ended.

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> On 10/1/13 8:21 AM, Ingo Molnar wrote:
> >But if I understood it correctly this particular message could trigger for
> >regular users of perf as well, of the perf record is terminated in some
> >unusual fashion. Regular users might not have the perf code handy (and
> >might not know about git grep either).
> 
> This is the case I was referring to -- normal users don't care about
> the code reference, hence the more specific question about how the
> perf-record session ended.

Hm, what do you call 'code reference'?

The message I suggested is:

  WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?

I didn't intend 'perf/header' to be a code reference - it wanted to refer 
to the perf.data header. Maybe that should be formulated in a less 
confusing manner? Something like:

  WARNING: The perf.data file's data size field is 0 which is unexpected.
           Was the 'perf record' command properly terminated?

Thanks,

	Ingo

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[Arnaldo Carvalho de Melo]

Em Tue, Oct 01, 2013 at 05:33:09PM +0200, Ingo Molnar escreveu:
> 
> * David Ahern <dsahern@gmail.com> wrote:
> 
> > On 10/1/13 8:21 AM, Ingo Molnar wrote:
> > >But if I understood it correctly this particular message could trigger for
> > >regular users of perf as well, of the perf record is terminated in some
> > >unusual fashion. Regular users might not have the perf code handy (and
> > >might not know about git grep either).
> > 
> > This is the case I was referring to -- normal users don't care about
> > the code reference, hence the more specific question about how the
> > perf-record session ended.
> 
> Hm, what do you call 'code reference'?
> 
> The message I suggested is:
> 
>   WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
> 
> I didn't intend 'perf/header' to be a code reference - it wanted to refer 
> to the perf.data header. Maybe that should be formulated in a less 
> confusing manner? Something like:
 
I liked this last one:

>   WARNING: The perf.data file's data size field is 0 which is unexpected.
>            Was the 'perf record' command properly terminated?

Can I patch that up into Namhyung's latest patch?

Sonny, David, from your replies I think I can add Tested-by: tags for
both of you?

Thanks,

- Arnaldo

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[David Ahern]

On 10/1/13 10:54 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Oct 01, 2013 at 05:33:09PM +0200, Ingo Molnar escreveu:
>>
>> * David Ahern <dsahern@gmail.com> wrote:
>>
>>> On 10/1/13 8:21 AM, Ingo Molnar wrote:
>>>> But if I understood it correctly this particular message could trigger for
>>>> regular users of perf as well, of the perf record is terminated in some
>>>> unusual fashion. Regular users might not have the perf code handy (and
>>>> might not know about git grep either).
>>>
>>> This is the case I was referring to -- normal users don't care about
>>> the code reference, hence the more specific question about how the
>>> perf-record session ended.
>>
>> Hm, what do you call 'code reference'?
>>
>> The message I suggested is:
>>
>>    WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
>>
>> I didn't intend 'perf/header' to be a code reference - it wanted to refer
>> to the perf.data header. Maybe that should be formulated in a less
>> confusing manner? Something like:
>
> I liked this last one:
>
>>    WARNING: The perf.data file's data size field is 0 which is unexpected.
>>             Was the 'perf record' command properly terminated?
>
> Can I patch that up into Namhyung's latest patch?
>
> Sonny, David, from your replies I think I can add Tested-by: tags for
> both of you?

I'm fine with the last message. And yes a Tested by

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[Arnaldo Carvalho de Melo]

Em Tue, Oct 01, 2013 at 10:59:06AM -0600, David Ahern escreveu:
> On 10/1/13 10:54 AM, Arnaldo Carvalho de Melo wrote:
> >Em Tue, Oct 01, 2013 at 05:33:09PM +0200, Ingo Molnar escreveu:
> >>* David Ahern <dsahern@gmail.com> wrote:
> >>>On 10/1/13 8:21 AM, Ingo Molnar wrote:
> >I liked this last one:
> >
> >>   WARNING: The perf.data file's data size field is 0 which is unexpected.
> >>            Was the 'perf record' command properly terminated?
> >
> >Can I patch that up into Namhyung's latest patch?
> >
> >Sonny, David, from your replies I think I can add Tested-by: tags for
> >both of you?
> 
> I'm fine with the last message. And yes a Tested by

Got to process this now, changing it a bit to replace "perf.data" with
"%s" + session->filename, ack?

Otherwise the user may have a 'perf.data' file and despite having
informed -i with a different file name still get confused... ;-)

- Arnaldo

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[David Ahern]

On 10/4/13 9:25 AM, Arnaldo Carvalho de Melo wrote:
> Got to process this now, changing it a bit to replace "perf.data" with
> "%s" + session->filename, ack?
>
> Otherwise the user may have a 'perf.data' file and despite having
> informed -i with a different file name still get confused... ;-)
>

ack. agreed that's the better way to go.

David

Re: [PATCH] perf session: Fix infinite loop on invalid perf.data file

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Tue, Oct 01, 2013 at 05:33:09PM +0200, Ingo Molnar escreveu:
> > 
> > * David Ahern <dsahern@gmail.com> wrote:
> > 
> > > On 10/1/13 8:21 AM, Ingo Molnar wrote:
> > > >But if I understood it correctly this particular message could trigger for
> > > >regular users of perf as well, of the perf record is terminated in some
> > > >unusual fashion. Regular users might not have the perf code handy (and
> > > >might not know about git grep either).
> > > 
> > > This is the case I was referring to -- normal users don't care about
> > > the code reference, hence the more specific question about how the
> > > perf-record session ended.
> > 
> > Hm, what do you call 'code reference'?
> > 
> > The message I suggested is:
> > 
> >   WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
> > 
> > I didn't intend 'perf/header' to be a code reference - it wanted to refer 
> > to the perf.data header. Maybe that should be formulated in a less 
> > confusing manner? Something like:
>  
> I liked this last one:
> 
> >   WARNING: The perf.data file's data size field is 0 which is unexpected.
> >            Was the 'perf record' command properly terminated?
> 
> Can I patch that up into Namhyung's latest patch?

Sure, feel free!

Thanks,

	Ingo

[tip:perf/urgent] perf session: Fix infinite loop on invalid perf.data file

[tip-bot for Namhyung Kim]

Commit-ID:  b314e5cfd11fd78545ce6c2be42646254390c1aa
Gitweb:     http://git.kernel.org/tip/b314e5cfd11fd78545ce6c2be42646254390c1aa
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Mon, 30 Sep 2013 17:19:48 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 4 Oct 2013 15:17:46 -0300

perf session: Fix infinite loop on invalid perf.data file

perf-record updates the header in the perf.data file at termination.
Without this update perf-report (and other processing built-ins) it
caused an infinite loop when perf report (or something like) called.

This is because the algorithm in __perf_session__process_events()
depends on the data_size which is read from file header.  Use file size
directly instead in this case to do the best-effort processing.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: David Ahern <dsahern@gmail.com>
Tested-by: Sonny Rao <sonnyrao@chromium.org>
Acked-by: Ingo Molnar <mingo@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Sonny Rao <sonnyrao@chromium.org>
Link: http://lkml.kernel.org/r/1380529188-27193-1-git-send-email-namhyung@kernel.org
Signed-off-by: David Ahern <dsahern@gmail.com>
[ Reworded warning as per Ingo Molnar suggestion, replaces 'perf.data'
  with session->filename, to precisely identify the data file involved ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c  | 12 ++++++++++++
 tools/perf/util/session.c |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index ce69901..c3e5a3b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2768,6 +2768,18 @@ int perf_session__read_header(struct perf_session *session)
 	if (perf_file_header__read(&f_header, header, fd) < 0)
 		return -EINVAL;
 
+	/*
+	 * Sanity check that perf.data was written cleanly; data size is
+	 * initialized to 0 and updated only if the on_exit function is run.
+	 * If data size is still 0 then the file contains only partial
+	 * information.  Just warn user and process it as much as it can.
+	 */
+	if (f_header.data.size == 0) {
+		pr_warning("WARNING: The %s file's data size field is 0 which is unexpected.\n"
+			   "Was the 'perf record' command properly terminated?\n",
+			   session->filename);
+	}
+
 	nr_attrs = f_header.attrs.size / f_header.attr_size;
 	lseek(fd, f_header.attrs.offset, SEEK_SET);
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 37c4718..568b750 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1312,7 +1312,7 @@ int __perf_session__process_events(struct perf_session *session,
 	file_offset = page_offset;
 	head = data_offset - page_offset;
 
-	if (data_offset + data_size < file_size)
+	if (data_size && (data_offset + data_size < file_size))
 		file_size = data_offset + data_size;
 
 	progress_next = file_size / 16;