* [PATCH] perf-script: check session->header.env.arch before using it
@ 2021-10-04 5:32 Song Liu
2021-10-16 0:59 ` Song Liu
2021-10-27 12:39 ` Arnaldo Carvalho de Melo
0 siblings, 2 replies; 5+ messages in thread
From: Song Liu @ 2021-10-04 5:32 UTC (permalink / raw)
To: linux-kernel; +Cc: kernel-team, acme, peterz, mingo, Song Liu, stable
When perf.data is not written cleanly, we would like to process existing
data as much as possible (please see f_header.data.size == 0 condition
in perf_session__read_header). However, perf.data with partial data may
crash perf. Specifically, we see crash in perf-script for NULL
session->header.env.arch.
Fix this by checking session->header.env.arch before using it to determine
native_arch. Also split the if condition so it is easier to read.
Cc: stable@vger.kernel.org
Signed-off-by: Song Liu <songliubraving@fb.com>
---
tools/perf/builtin-script.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 6211d0b84b7a6..7821f6740ac1d 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -4039,12 +4039,17 @@ int cmd_script(int argc, const char **argv)
goto out_delete;
uname(&uts);
- if (data.is_pipe || /* assume pipe_mode indicates native_arch */
- !strcmp(uts.machine, session->header.env.arch) ||
- (!strcmp(uts.machine, "x86_64") &&
- !strcmp(session->header.env.arch, "i386")))
+ if (data.is_pipe) /* assume pipe_mode indicates native_arch */
native_arch = true;
+ if (session->header.env.arch) {
+ if (!strcmp(uts.machine, session->header.env.arch))
+ native_arch = true;
+ else if (!strcmp(uts.machine, "x86_64") &&
+ !strcmp(session->header.env.arch, "i386"))
+ native_arch = true;
+ }
+
script.session = session;
script__setup_sample_type(&script);
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] perf-script: check session->header.env.arch before using it
2021-10-04 5:32 [PATCH] perf-script: check session->header.env.arch before using it Song Liu
@ 2021-10-16 0:59 ` Song Liu
2021-10-27 12:39 ` Arnaldo Carvalho de Melo
1 sibling, 0 replies; 5+ messages in thread
From: Song Liu @ 2021-10-16 0:59 UTC (permalink / raw)
To: LKML, Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo
Cc: Kernel Team, Peter Zijlstra, Ingo Molnar, stable
Hi Arnaldo,
Could you please share your comments on this one?
Thanks,
Song
> On Oct 3, 2021, at 10:32 PM, Song Liu <songliubraving@fb.com> wrote:
>
> When perf.data is not written cleanly, we would like to process existing
> data as much as possible (please see f_header.data.size == 0 condition
> in perf_session__read_header). However, perf.data with partial data may
> crash perf. Specifically, we see crash in perf-script for NULL
> session->header.env.arch.
>
> Fix this by checking session->header.env.arch before using it to determine
> native_arch. Also split the if condition so it is easier to read.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> tools/perf/builtin-script.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 6211d0b84b7a6..7821f6740ac1d 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -4039,12 +4039,17 @@ int cmd_script(int argc, const char **argv)
> goto out_delete;
>
> uname(&uts);
> - if (data.is_pipe || /* assume pipe_mode indicates native_arch */
> - !strcmp(uts.machine, session->header.env.arch) ||
> - (!strcmp(uts.machine, "x86_64") &&
> - !strcmp(session->header.env.arch, "i386")))
> + if (data.is_pipe) /* assume pipe_mode indicates native_arch */
> native_arch = true;
>
> + if (session->header.env.arch) {
> + if (!strcmp(uts.machine, session->header.env.arch))
> + native_arch = true;
> + else if (!strcmp(uts.machine, "x86_64") &&
> + !strcmp(session->header.env.arch, "i386"))
> + native_arch = true;
> + }
> +
> script.session = session;
> script__setup_sample_type(&script);
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf-script: check session->header.env.arch before using it
2021-10-04 5:32 [PATCH] perf-script: check session->header.env.arch before using it Song Liu
2021-10-16 0:59 ` Song Liu
@ 2021-10-27 12:39 ` Arnaldo Carvalho de Melo
2021-10-27 16:41 ` Song Liu
1 sibling, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-10-27 12:39 UTC (permalink / raw)
To: Song Liu; +Cc: linux-kernel, kernel-team, peterz, mingo, stable
Em Sun, Oct 03, 2021 at 10:32:38PM -0700, Song Liu escreveu:
> When perf.data is not written cleanly, we would like to process existing
> data as much as possible (please see f_header.data.size == 0 condition
> in perf_session__read_header). However, perf.data with partial data may
> crash perf. Specifically, we see crash in perf-script for NULL
> session->header.env.arch.
>
> Fix this by checking session->header.env.arch before using it to determine
> native_arch. Also split the if condition so it is easier to read.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> tools/perf/builtin-script.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 6211d0b84b7a6..7821f6740ac1d 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -4039,12 +4039,17 @@ int cmd_script(int argc, const char **argv)
> goto out_delete;
>
> uname(&uts);
> - if (data.is_pipe || /* assume pipe_mode indicates native_arch */
> - !strcmp(uts.machine, session->header.env.arch) ||
> - (!strcmp(uts.machine, "x86_64") &&
> - !strcmp(session->header.env.arch, "i386")))
> + if (data.is_pipe) /* assume pipe_mode indicates native_arch */
> native_arch = true;
>
> + if (session->header.env.arch) {
Shouldn't the above be:
else if (session->header.env.arch) {
?
> + if (!strcmp(uts.machine, session->header.env.arch))
> + native_arch = true;
> + else if (!strcmp(uts.machine, "x86_64") &&
> + !strcmp(session->header.env.arch, "i386"))
> + native_arch = true;
> + }
> +
> script.session = session;
> script__setup_sample_type(&script);
>
> --
> 2.30.2
--
- Arnaldo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf-script: check session->header.env.arch before using it
2021-10-27 12:39 ` Arnaldo Carvalho de Melo
@ 2021-10-27 16:41 ` Song Liu
2021-10-27 16:43 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 5+ messages in thread
From: Song Liu @ 2021-10-27 16:41 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: LKML, Kernel Team, Peter Zijlstra, Ingo Molnar, stable
> On Oct 27, 2021, at 5:39 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Sun, Oct 03, 2021 at 10:32:38PM -0700, Song Liu escreveu:
>> When perf.data is not written cleanly, we would like to process existing
>> data as much as possible (please see f_header.data.size == 0 condition
>> in perf_session__read_header). However, perf.data with partial data may
>> crash perf. Specifically, we see crash in perf-script for NULL
>> session->header.env.arch.
>>
>> Fix this by checking session->header.env.arch before using it to determine
>> native_arch. Also split the if condition so it is easier to read.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> tools/perf/builtin-script.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
>> index 6211d0b84b7a6..7821f6740ac1d 100644
>> --- a/tools/perf/builtin-script.c
>> +++ b/tools/perf/builtin-script.c
>> @@ -4039,12 +4039,17 @@ int cmd_script(int argc, const char **argv)
>> goto out_delete;
>>
>> uname(&uts);
>> - if (data.is_pipe || /* assume pipe_mode indicates native_arch */
>> - !strcmp(uts.machine, session->header.env.arch) ||
>> - (!strcmp(uts.machine, "x86_64") &&
>> - !strcmp(session->header.env.arch, "i386")))
>> + if (data.is_pipe) /* assume pipe_mode indicates native_arch */
>> native_arch = true;
>>
>> + if (session->header.env.arch) {
>
> Shouldn't the above be:
>
> else if (session->header.env.arch) {
>
> ?
Yes! That's better.
Do you want me to send v2 with the change?
Thanks,
Song
>
>> + if (!strcmp(uts.machine, session->header.env.arch))
>> + native_arch = true;
>> + else if (!strcmp(uts.machine, "x86_64") &&
>> + !strcmp(session->header.env.arch, "i386"))
>> + native_arch = true;
>> + }
>> +
>> script.session = session;
>> script__setup_sample_type(&script);
>>
>> --
>> 2.30.2
>
> --
>
> - Arnaldo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf-script: check session->header.env.arch before using it
2021-10-27 16:41 ` Song Liu
@ 2021-10-27 16:43 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-10-27 16:43 UTC (permalink / raw)
To: Song Liu, Arnaldo Carvalho de Melo
Cc: LKML, Kernel Team, Peter Zijlstra, Ingo Molnar, stable
On October 27, 2021 1:41:50 PM GMT-03:00, Song Liu <songliubraving@fb.com> wrote:
>
>
>> On Oct 27, 2021, at 5:39 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>>
>> Em Sun, Oct 03, 2021 at 10:32:38PM -0700, Song Liu escreveu:
>>> When perf.data is not written cleanly, we would like to process existing
>>> data as much as possible (please see f_header.data.size == 0 condition
>>> in perf_session__read_header). However, perf.data with partial data may
>>> crash perf. Specifically, we see crash in perf-script for NULL
>>> session->header.env.arch.
>>>
>>> Fix this by checking session->header.env.arch before using it to determine
>>> native_arch. Also split the if condition so it is easier to read.
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>> ---
>>> tools/perf/builtin-script.c | 13 +++++++++----
>>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
>>> index 6211d0b84b7a6..7821f6740ac1d 100644
>>> --- a/tools/perf/builtin-script.c
>>> +++ b/tools/perf/builtin-script.c
>>> @@ -4039,12 +4039,17 @@ int cmd_script(int argc, const char **argv)
>>> goto out_delete;
>>>
>>> uname(&uts);
>>> - if (data.is_pipe || /* assume pipe_mode indicates native_arch */
>>> - !strcmp(uts.machine, session->header.env.arch) ||
>>> - (!strcmp(uts.machine, "x86_64") &&
>>> - !strcmp(session->header.env.arch, "i386")))
>>> + if (data.is_pipe) /* assume pipe_mode indicates native_arch */
>>> native_arch = true;
>>>
>>> + if (session->header.env.arch) {
>>
>> Shouldn't the above be:
>>
>> else if (session->header.env.arch) {
>>
>> ?
>
>Yes! That's better.
>
>Do you want me to send v2 with the change?
No need, it's simple enough, I'll do it myself,
- Arnaldo
>
>Thanks,
>Song
>
>>
>>> + if (!strcmp(uts.machine, session->header.env.arch))
>>> + native_arch = true;
>>> + else if (!strcmp(uts.machine, "x86_64") &&
>>> + !strcmp(session->header.env.arch, "i386"))
>>> + native_arch = true;
>>> + }
>>> +
>>> script.session = session;
>>> script__setup_sample_type(&script);
>>>
>>> --
>>> 2.30.2
>>
>> --
>>
>> - Arnaldo
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-27 16:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-04 5:32 [PATCH] perf-script: check session->header.env.arch before using it Song Liu
2021-10-16 0:59 ` Song Liu
2021-10-27 12:39 ` Arnaldo Carvalho de Melo
2021-10-27 16:41 ` Song Liu
2021-10-27 16:43 ` 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.