* [PATCH] perf probe: Fix uninitialized variable
@ 2024-12-09 17:12 James Clark
2024-12-09 23:10 ` Namhyung Kim
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: James Clark @ 2024-12-09 17:12 UTC (permalink / raw)
To: linux-perf-users, namhyung, acme
Cc: James Clark, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Masami Hiramatsu (Google), Leo Yan, Dima Kogan,
Dr. David Alan Gilbert, linux-kernel
Since the linked fixes: commit, err is returned uninitialized due to the
removal of "return 0". Initialize err to fix it, and rename err to out
to avoid confusion because buf is still supposed to be freed in non
error cases.
This fixes the following intermittent test failure on release builds:
$ perf test "testsuite_probe"
...
-- [ FAIL ] -- perf_probe :: test_invalid_options :: mutually exclusive options :: -L foo -V bar (output regexp parsing)
Regexp not found: \"Error: switch .+ cannot be used with switch .+\"
...
Fixes: 080e47b2a237 ("perf probe: Introduce quotation marks support")
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/perf/util/probe-event.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 6d51a4c98ad7..35af6570cf9b 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1370,7 +1370,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
{
char *buf = strdup(arg);
char *p;
- int err;
+ int err = 0;
if (!buf)
return -ENOMEM;
@@ -1383,20 +1383,20 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
if (p == buf) {
semantic_error("No file/function name in '%s'.\n", p);
err = -EINVAL;
- goto err;
+ goto out;
}
*(p++) = '\0';
err = parse_line_num(&p, &lr->start, "start line");
if (err)
- goto err;
+ goto out;
if (*p == '+' || *p == '-') {
const char c = *(p++);
err = parse_line_num(&p, &lr->end, "end line");
if (err)
- goto err;
+ goto out;
if (c == '+') {
lr->end += lr->start;
@@ -1416,11 +1416,11 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
if (lr->start > lr->end) {
semantic_error("Start line must be smaller"
" than end line.\n");
- goto err;
+ goto out;
}
if (*p != '\0') {
semantic_error("Tailing with invalid str '%s'.\n", p);
- goto err;
+ goto out;
}
}
@@ -1431,7 +1431,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
lr->file = strdup_esq(p);
if (lr->file == NULL) {
err = -ENOMEM;
- goto err;
+ goto out;
}
}
if (*buf != '\0')
@@ -1439,7 +1439,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
if (!lr->function && !lr->file) {
semantic_error("Only '@*' is not allowed.\n");
err = -EINVAL;
- goto err;
+ goto out;
}
} else if (strpbrk_esq(buf, "/."))
lr->file = strdup_esq(buf);
@@ -1448,10 +1448,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
else { /* Invalid name */
semantic_error("'%s' is not a valid function name.\n", buf);
err = -EINVAL;
- goto err;
+ goto out;
}
-err:
+out:
free(buf);
return err;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] perf probe: Fix uninitialized variable
2024-12-09 17:12 [PATCH] perf probe: Fix uninitialized variable James Clark
@ 2024-12-09 23:10 ` Namhyung Kim
2024-12-10 13:26 ` Arnaldo Carvalho de Melo
2024-12-10 22:55 ` Masami Hiramatsu
2 siblings, 0 replies; 5+ messages in thread
From: Namhyung Kim @ 2024-12-09 23:10 UTC (permalink / raw)
To: James Clark
Cc: linux-perf-users, acme, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Masami Hiramatsu (Google), Leo Yan, Dima Kogan,
Dr. David Alan Gilbert, linux-kernel
On Mon, Dec 09, 2024 at 05:12:21PM +0000, James Clark wrote:
> Since the linked fixes: commit, err is returned uninitialized due to the
> removal of "return 0". Initialize err to fix it, and rename err to out
> to avoid confusion because buf is still supposed to be freed in non
> error cases.
>
> This fixes the following intermittent test failure on release builds:
>
> $ perf test "testsuite_probe"
> ...
> -- [ FAIL ] -- perf_probe :: test_invalid_options :: mutually exclusive options :: -L foo -V bar (output regexp parsing)
> Regexp not found: \"Error: switch .+ cannot be used with switch .+\"
> ...
Tested-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
>
> Fixes: 080e47b2a237 ("perf probe: Introduce quotation marks support")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> tools/perf/util/probe-event.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 6d51a4c98ad7..35af6570cf9b 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1370,7 +1370,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> {
> char *buf = strdup(arg);
> char *p;
> - int err;
> + int err = 0;
>
> if (!buf)
> return -ENOMEM;
> @@ -1383,20 +1383,20 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> if (p == buf) {
> semantic_error("No file/function name in '%s'.\n", p);
> err = -EINVAL;
> - goto err;
> + goto out;
> }
> *(p++) = '\0';
>
> err = parse_line_num(&p, &lr->start, "start line");
> if (err)
> - goto err;
> + goto out;
>
> if (*p == '+' || *p == '-') {
> const char c = *(p++);
>
> err = parse_line_num(&p, &lr->end, "end line");
> if (err)
> - goto err;
> + goto out;
>
> if (c == '+') {
> lr->end += lr->start;
> @@ -1416,11 +1416,11 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> if (lr->start > lr->end) {
> semantic_error("Start line must be smaller"
> " than end line.\n");
> - goto err;
> + goto out;
> }
> if (*p != '\0') {
> semantic_error("Tailing with invalid str '%s'.\n", p);
> - goto err;
> + goto out;
> }
> }
>
> @@ -1431,7 +1431,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> lr->file = strdup_esq(p);
> if (lr->file == NULL) {
> err = -ENOMEM;
> - goto err;
> + goto out;
> }
> }
> if (*buf != '\0')
> @@ -1439,7 +1439,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> if (!lr->function && !lr->file) {
> semantic_error("Only '@*' is not allowed.\n");
> err = -EINVAL;
> - goto err;
> + goto out;
> }
> } else if (strpbrk_esq(buf, "/."))
> lr->file = strdup_esq(buf);
> @@ -1448,10 +1448,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> else { /* Invalid name */
> semantic_error("'%s' is not a valid function name.\n", buf);
> err = -EINVAL;
> - goto err;
> + goto out;
> }
>
> -err:
> +out:
> free(buf);
> return err;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf probe: Fix uninitialized variable
2024-12-09 17:12 [PATCH] perf probe: Fix uninitialized variable James Clark
2024-12-09 23:10 ` Namhyung Kim
@ 2024-12-10 13:26 ` Arnaldo Carvalho de Melo
2024-12-10 22:55 ` Masami Hiramatsu
2 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-12-10 13:26 UTC (permalink / raw)
To: Namhyung Kim, James Clark
Cc: linux-perf-users, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Masami Hiramatsu (Google), Leo Yan, Dima Kogan,
Dr. David Alan Gilbert, linux-kernel
On Mon, Dec 09, 2024 at 05:12:21PM +0000, James Clark wrote:
> Since the linked fixes: commit, err is returned uninitialized due to the
> removal of "return 0". Initialize err to fix it, and rename err to out
> to avoid confusion because buf is still supposed to be freed in non
> error cases.
>
> This fixes the following intermittent test failure on release builds:
>
> $ perf test "testsuite_probe"
> ...
> -- [ FAIL ] -- perf_probe :: test_invalid_options :: mutually exclusive options :: -L foo -V bar (output regexp parsing)
> Regexp not found: \"Error: switch .+ cannot be used with switch .+\"
> ...
Namhyung, I think this should go via perf-tools,
Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
- Arnaldo
> Fixes: 080e47b2a237 ("perf probe: Introduce quotation marks support")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> tools/perf/util/probe-event.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 6d51a4c98ad7..35af6570cf9b 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1370,7 +1370,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> {
> char *buf = strdup(arg);
> char *p;
> - int err;
> + int err = 0;
>
> if (!buf)
> return -ENOMEM;
> @@ -1383,20 +1383,20 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> if (p == buf) {
> semantic_error("No file/function name in '%s'.\n", p);
> err = -EINVAL;
> - goto err;
> + goto out;
> }
> *(p++) = '\0';
>
> err = parse_line_num(&p, &lr->start, "start line");
> if (err)
> - goto err;
> + goto out;
>
> if (*p == '+' || *p == '-') {
> const char c = *(p++);
>
> err = parse_line_num(&p, &lr->end, "end line");
> if (err)
> - goto err;
> + goto out;
>
> if (c == '+') {
> lr->end += lr->start;
> @@ -1416,11 +1416,11 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> if (lr->start > lr->end) {
> semantic_error("Start line must be smaller"
> " than end line.\n");
> - goto err;
> + goto out;
> }
> if (*p != '\0') {
> semantic_error("Tailing with invalid str '%s'.\n", p);
> - goto err;
> + goto out;
> }
> }
>
> @@ -1431,7 +1431,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> lr->file = strdup_esq(p);
> if (lr->file == NULL) {
> err = -ENOMEM;
> - goto err;
> + goto out;
> }
> }
> if (*buf != '\0')
> @@ -1439,7 +1439,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> if (!lr->function && !lr->file) {
> semantic_error("Only '@*' is not allowed.\n");
> err = -EINVAL;
> - goto err;
> + goto out;
> }
> } else if (strpbrk_esq(buf, "/."))
> lr->file = strdup_esq(buf);
> @@ -1448,10 +1448,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> else { /* Invalid name */
> semantic_error("'%s' is not a valid function name.\n", buf);
> err = -EINVAL;
> - goto err;
> + goto out;
> }
>
> -err:
> +out:
> free(buf);
> return err;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf probe: Fix uninitialized variable
2024-12-09 17:12 [PATCH] perf probe: Fix uninitialized variable James Clark
2024-12-09 23:10 ` Namhyung Kim
2024-12-10 13:26 ` Arnaldo Carvalho de Melo
@ 2024-12-10 22:55 ` Masami Hiramatsu
[not found] ` <CA+JHD90Brn_jLxPPMr1M0RB6+ms1O3+OvOPWJ0wkUe+MXYpH1A@mail.gmail.com>
2 siblings, 1 reply; 5+ messages in thread
From: Masami Hiramatsu @ 2024-12-10 22:55 UTC (permalink / raw)
To: James Clark
Cc: linux-perf-users, namhyung, acme, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
Adrian Hunter, Liang, Kan, Masami Hiramatsu (Google), Leo Yan,
Dima Kogan, Dr. David Alan Gilbert, linux-kernel
Hi,
On Mon, 9 Dec 2024 17:12:21 +0000
James Clark <james.clark@linaro.org> wrote:
> Since the linked fixes: commit, err is returned uninitialized due to the
> removal of "return 0". Initialize err to fix it, and rename err to out
> to avoid confusion because buf is still supposed to be freed in non
> error cases.
>
> This fixes the following intermittent test failure on release builds:
>
> $ perf test "testsuite_probe"
> ...
> -- [ FAIL ] -- perf_probe :: test_invalid_options :: mutually exclusive options :: -L foo -V bar (output regexp parsing)
> Regexp not found: \"Error: switch .+ cannot be used with switch .+\"
> ...
>
> Fixes: 080e47b2a237 ("perf probe: Introduce quotation marks support")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> tools/perf/util/probe-event.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 6d51a4c98ad7..35af6570cf9b 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1370,7 +1370,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> {
> char *buf = strdup(arg);
> char *p;
> - int err;
> + int err = 0;
I think only this is required, and others are just cleanups by renaming
err -> out (usually for-next).
But Arnaldo is OK to combine these changes, I'm OK too.
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thank you,
>
> if (!buf)
> return -ENOMEM;
> @@ -1383,20 +1383,20 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> if (p == buf) {
> semantic_error("No file/function name in '%s'.\n", p);
> err = -EINVAL;
> - goto err;
> + goto out;
> }
> *(p++) = '\0';
>
> err = parse_line_num(&p, &lr->start, "start line");
> if (err)
> - goto err;
> + goto out;
>
> if (*p == '+' || *p == '-') {
> const char c = *(p++);
>
> err = parse_line_num(&p, &lr->end, "end line");
> if (err)
> - goto err;
> + goto out;
>
> if (c == '+') {
> lr->end += lr->start;
> @@ -1416,11 +1416,11 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> if (lr->start > lr->end) {
> semantic_error("Start line must be smaller"
> " than end line.\n");
> - goto err;
> + goto out;
> }
> if (*p != '\0') {
> semantic_error("Tailing with invalid str '%s'.\n", p);
> - goto err;
> + goto out;
> }
> }
>
> @@ -1431,7 +1431,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> lr->file = strdup_esq(p);
> if (lr->file == NULL) {
> err = -ENOMEM;
> - goto err;
> + goto out;
> }
> }
> if (*buf != '\0')
> @@ -1439,7 +1439,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> if (!lr->function && !lr->file) {
> semantic_error("Only '@*' is not allowed.\n");
> err = -EINVAL;
> - goto err;
> + goto out;
> }
> } else if (strpbrk_esq(buf, "/."))
> lr->file = strdup_esq(buf);
> @@ -1448,10 +1448,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> else { /* Invalid name */
> semantic_error("'%s' is not a valid function name.\n", buf);
> err = -EINVAL;
> - goto err;
> + goto out;
> }
>
> -err:
> +out:
> free(buf);
> return err;
> }
> --
> 2.34.1
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf probe: Fix uninitialized variable
[not found] ` <CA+JHD90Brn_jLxPPMr1M0RB6+ms1O3+OvOPWJ0wkUe+MXYpH1A@mail.gmail.com>
@ 2024-12-11 8:39 ` James Clark
0 siblings, 0 replies; 5+ messages in thread
From: James Clark @ 2024-12-11 8:39 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Masami Hiramatsu
Cc: linux-perf-users, Namhyung Kim, Arnaldo Carvalho de Melo,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Ian Rogers, Adrian Hunter, Liang, Kan, Leo Yan,
Dima Kogan, Dr. David Alan Gilbert, Linux Kernel Mailing List
On 11/12/2024 12:43 am, Arnaldo Carvalho de Melo wrote:
> On Tue, Dec 10, 2024, 7:55 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
>> Hi,
>>
>> On Mon, 9 Dec 2024 17:12:21 +0000
>> James Clark <james.clark@linaro.org> wrote:
>>
>>> Since the linked fixes: commit, err is returned uninitialized due to the
>>> removal of "return 0". Initialize err to fix it, and rename err to out
>>> to avoid confusion because buf is still supposed to be freed in non
>>> error cases.
>>>
>>> This fixes the following intermittent test failure on release builds:
>>>
>>> $ perf test "testsuite_probe"
>>> ...
>>> -- [ FAIL ] -- perf_probe :: test_invalid_options :: mutually exclusive
>> options :: -L foo -V bar (output regexp parsing)
>>> Regexp not found: \"Error: switch .+ cannot be used with switch .+\"
>>> ...
>>>
>>> Fixes: 080e47b2a237 ("perf probe: Introduce quotation marks support")
>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>> ---
>>> tools/perf/util/probe-event.c | 20 ++++++++++----------
>>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/tools/perf/util/probe-event.c
>> b/tools/perf/util/probe-event.c
>>> index 6d51a4c98ad7..35af6570cf9b 100644
>>> --- a/tools/perf/util/probe-event.c
>>> +++ b/tools/perf/util/probe-event.c
>>> @@ -1370,7 +1370,7 @@ int parse_line_range_desc(const char *arg, struct
>> line_range *lr)
>>> {
>>> char *buf = strdup(arg);
>>> char *p;
>>> - int err;
>>> + int err = 0;
>>
>> I think only this is required, and others are just cleanups by renaming
>> err -> out (usually for-next).
>> But Arnaldo is OK to combine these changes, I'm OK too.
>>
>> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>>
>
> I agree, almost mentioned that, but since you mentioned this, yes, I'd
> prefer to have as small as possible a patch.
>
> - Arnaldo
>
>
Sure, I'll split it
>> Thank you,
>>
>>>
>>> if (!buf)
>>> return -ENOMEM;
>>> @@ -1383,20 +1383,20 @@ int parse_line_range_desc(const char *arg,
>> struct line_range *lr)
>>> if (p == buf) {
>>> semantic_error("No file/function name in '%s'.\n",
>> p);
>>> err = -EINVAL;
>>> - goto err;
>>> + goto out;
>>> }
>>> *(p++) = '\0';
>>>
>>> err = parse_line_num(&p, &lr->start, "start line");
>>> if (err)
>>> - goto err;
>>> + goto out;
>>>
>>> if (*p == '+' || *p == '-') {
>>> const char c = *(p++);
>>>
>>> err = parse_line_num(&p, &lr->end, "end line");
>>> if (err)
>>> - goto err;
>>> + goto out;
>>>
>>> if (c == '+') {
>>> lr->end += lr->start;
>>> @@ -1416,11 +1416,11 @@ int parse_line_range_desc(const char *arg,
>> struct line_range *lr)
>>> if (lr->start > lr->end) {
>>> semantic_error("Start line must be smaller"
>>> " than end line.\n");
>>> - goto err;
>>> + goto out;
>>> }
>>> if (*p != '\0') {
>>> semantic_error("Tailing with invalid str '%s'.\n",
>> p);
>>> - goto err;
>>> + goto out;
>>> }
>>> }
>>>
>>> @@ -1431,7 +1431,7 @@ int parse_line_range_desc(const char *arg, struct
>> line_range *lr)
>>> lr->file = strdup_esq(p);
>>> if (lr->file == NULL) {
>>> err = -ENOMEM;
>>> - goto err;
>>> + goto out;
>>> }
>>> }
>>> if (*buf != '\0')
>>> @@ -1439,7 +1439,7 @@ int parse_line_range_desc(const char *arg, struct
>> line_range *lr)
>>> if (!lr->function && !lr->file) {
>>> semantic_error("Only '@*' is not allowed.\n");
>>> err = -EINVAL;
>>> - goto err;
>>> + goto out;
>>> }
>>> } else if (strpbrk_esq(buf, "/."))
>>> lr->file = strdup_esq(buf);
>>> @@ -1448,10 +1448,10 @@ int parse_line_range_desc(const char *arg,
>> struct line_range *lr)
>>> else { /* Invalid name */
>>> semantic_error("'%s' is not a valid function name.\n",
>> buf);
>>> err = -EINVAL;
>>> - goto err;
>>> + goto out;
>>> }
>>>
>>> -err:
>>> +out:
>>> free(buf);
>>> return err;
>>> }
>>> --
>>> 2.34.1
>>>
>>
>>
>> --
>> Masami Hiramatsu (Google) <mhiramat@kernel.org>
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-12-11 8:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-09 17:12 [PATCH] perf probe: Fix uninitialized variable James Clark
2024-12-09 23:10 ` Namhyung Kim
2024-12-10 13:26 ` Arnaldo Carvalho de Melo
2024-12-10 22:55 ` Masami Hiramatsu
[not found] ` <CA+JHD90Brn_jLxPPMr1M0RB6+ms1O3+OvOPWJ0wkUe+MXYpH1A@mail.gmail.com>
2024-12-11 8:39 ` James Clark
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).