diff mbox series

perf data convert: Fix segfault when converting to json on arm64

Message ID 20240111232923.8138-1-ilkka@os.amperecomputing.com
State New
Headers
Series perf data convert: Fix segfault when converting to json on arm64 |

Commit Message

Ilkka Koskinen Jan. 11, 2024, 11:29 p.m. UTC 
  Arm64 doesn't have Model in /proc/cpuinfo and, thus, cpu_desc doesn't get
assigned.

Running
	$ perf data convert --to-json perf.data.json

ends up calling output_json_string() with NULL pointer, which causes a
segmentation fault.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 tools/perf/util/data-convert-json.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

James Clark Jan. 12, 2024, 11:35 a.m. UTC | #1 
On 11/01/2024 23:29, Ilkka Koskinen wrote:
> Arm64 doesn't have Model in /proc/cpuinfo and, thus, cpu_desc doesn't get
> assigned.
> 
> Running
> 	$ perf data convert --to-json perf.data.json
> 
> ends up calling output_json_string() with NULL pointer, which causes a
> segmentation fault.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
>  tools/perf/util/data-convert-json.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
> index 5bb3c2ba95ca..5d6de1cef546 100644
> --- a/tools/perf/util/data-convert-json.c
> +++ b/tools/perf/util/data-convert-json.c
> @@ -97,6 +97,11 @@ static void output_json_format(FILE *out, bool comma, int depth, const char *for
>  static void output_json_key_string(FILE *out, bool comma, int depth,
>  		const char *key, const char *value)
>  {
> +	if (!value) {
> +		pr_info("No value set for key %s\n", key);
> +		return;
> +	}
> +
>  	output_json_delimiters(out, comma, depth);
>  	output_json_string(out, key);
>  	fputs(": ", out);


It looks like this would hide new errors on any of the other fields that
output_json_key_string() is called on. Maybe it would be better to only
wrap the call to output cpu_desc with the if? If that's the only one
that we think is optional, and even better only do it for arm64.

I mention this because the test for 'perf data convert' only checks for
valid json syntax, but not any fields. So we might want to avoid others
going missing.

Thanks
James
Namhyung Kim Jan. 16, 2024, 9:53 p.m. UTC | #2 
Hello,

On Fri, Jan 12, 2024 at 3:35 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 11/01/2024 23:29, Ilkka Koskinen wrote:
> > Arm64 doesn't have Model in /proc/cpuinfo and, thus, cpu_desc doesn't get
> > assigned.
> >
> > Running
> >       $ perf data convert --to-json perf.data.json
> >
> > ends up calling output_json_string() with NULL pointer, which causes a
> > segmentation fault.
> >
> > Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> > ---
> >  tools/perf/util/data-convert-json.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
> > index 5bb3c2ba95ca..5d6de1cef546 100644
> > --- a/tools/perf/util/data-convert-json.c
> > +++ b/tools/perf/util/data-convert-json.c
> > @@ -97,6 +97,11 @@ static void output_json_format(FILE *out, bool comma, int depth, const char *for
> >  static void output_json_key_string(FILE *out, bool comma, int depth,
> >               const char *key, const char *value)
> >  {
> > +     if (!value) {
> > +             pr_info("No value set for key %s\n", key);
> > +             return;
> > +     }
> > +
> >       output_json_delimiters(out, comma, depth);
> >       output_json_string(out, key);
> >       fputs(": ", out);
>
>
> It looks like this would hide new errors on any of the other fields that
> output_json_key_string() is called on. Maybe it would be better to only
> wrap the call to output cpu_desc with the if? If that's the only one
> that we think is optional, and even better only do it for arm64.
>
> I mention this because the test for 'perf data convert' only checks for
> valid json syntax, but not any fields. So we might want to avoid others
> going missing.

Makes sense.  Ilkka, can you send v2 with this?

Thanks,
Namhyung
Ilkka Koskinen Jan. 17, 2024, 4:47 a.m. UTC | #3 
On Tue, 16 Jan 2024, Namhyung Kim wrote:
> Hello,
>
> On Fri, Jan 12, 2024 at 3:35 AM James Clark <james.clark@arm.com> wrote:
>>
>>
>>
>> On 11/01/2024 23:29, Ilkka Koskinen wrote:
>>> Arm64 doesn't have Model in /proc/cpuinfo and, thus, cpu_desc doesn't get
>>> assigned.
>>>
>>> Running
>>>       $ perf data convert --to-json perf.data.json
>>>
>>> ends up calling output_json_string() with NULL pointer, which causes a
>>> segmentation fault.
>>>
>>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>>> ---
>>>  tools/perf/util/data-convert-json.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
>>> index 5bb3c2ba95ca..5d6de1cef546 100644
>>> --- a/tools/perf/util/data-convert-json.c
>>> +++ b/tools/perf/util/data-convert-json.c
>>> @@ -97,6 +97,11 @@ static void output_json_format(FILE *out, bool comma, int depth, const char *for
>>>  static void output_json_key_string(FILE *out, bool comma, int depth,
>>>               const char *key, const char *value)
>>>  {
>>> +     if (!value) {
>>> +             pr_info("No value set for key %s\n", key);
>>> +             return;
>>> +     }
>>> +
>>>       output_json_delimiters(out, comma, depth);
>>>       output_json_string(out, key);
>>>       fputs(": ", out);
>>
>>
>> It looks like this would hide new errors on any of the other fields that
>> output_json_key_string() is called on. Maybe it would be better to only
>> wrap the call to output cpu_desc with the if? If that's the only one
>> that we think is optional, and even better only do it for arm64.
>>
>> I mention this because the test for 'perf data convert' only checks for
>> valid json syntax, but not any fields. So we might want to avoid others
>> going missing.
>
> Makes sense.  Ilkka, can you send v2 with this?

I initially considered the choice James suggested but I kind of thought
that pr_info() might be enough. However, I don't have strong prefence on
either. I'll send an updated version soon.

Cheers, Ilkka

>
> Thanks,
> Namhyung
>
diff mbox series

Patch

diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
index 5bb3c2ba95ca..5d6de1cef546 100644
--- a/tools/perf/util/data-convert-json.c
+++ b/tools/perf/util/data-convert-json.c
@@ -97,6 +97,11 @@  static void output_json_format(FILE *out, bool comma, int depth, const char *for
 static void output_json_key_string(FILE *out, bool comma, int depth,
 		const char *key, const char *value)
 {
+	if (!value) {
+		pr_info("No value set for key %s\n", key);
+		return;
+	}
+
 	output_json_delimiters(out, comma, depth);
 	output_json_string(out, key);
 	fputs(": ", out);