* [PATCH] perf data convert: Fix segfault when converting to json on arm64 @ 2024-01-11 23:29 Ilkka Koskinen 2024-01-12 11:35 ` James Clark 0 siblings, 1 reply; 4+ messages in thread From: Ilkka Koskinen @ 2024-01-11 23:29 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter Cc: Ilkka Koskinen, linux-perf-users, linux-kernel 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); -- 2.43.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] perf data convert: Fix segfault when converting to json on arm64 2024-01-11 23:29 [PATCH] perf data convert: Fix segfault when converting to json on arm64 Ilkka Koskinen @ 2024-01-12 11:35 ` James Clark 2024-01-16 21:53 ` Namhyung Kim 0 siblings, 1 reply; 4+ messages in thread From: James Clark @ 2024-01-12 11:35 UTC (permalink / raw) To: Ilkka Koskinen Cc: linux-perf-users, linux-kernel, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] perf data convert: Fix segfault when converting to json on arm64 2024-01-12 11:35 ` James Clark @ 2024-01-16 21:53 ` Namhyung Kim 2024-01-17 4:47 ` Ilkka Koskinen 0 siblings, 1 reply; 4+ messages in thread From: Namhyung Kim @ 2024-01-16 21:53 UTC (permalink / raw) To: Ilkka Koskinen Cc: James Clark, linux-perf-users, linux-kernel, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] perf data convert: Fix segfault when converting to json on arm64 2024-01-16 21:53 ` Namhyung Kim @ 2024-01-17 4:47 ` Ilkka Koskinen 0 siblings, 0 replies; 4+ messages in thread From: Ilkka Koskinen @ 2024-01-17 4:47 UTC (permalink / raw) To: Namhyung Kim Cc: Ilkka Koskinen, James Clark, linux-perf-users, linux-kernel, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter [-- Attachment #1: Type: text/plain, Size: 2085 bytes --] 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-01-17 4:47 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-11 23:29 [PATCH] perf data convert: Fix segfault when converting to json on arm64 Ilkka Koskinen 2024-01-12 11:35 ` James Clark 2024-01-16 21:53 ` Namhyung Kim 2024-01-17 4:47 ` Ilkka Koskinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).