Re: [PATCH] perf probe: Fix uninitialized variable

James Clark
2024-12-11T08:39:58Z

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>
>>
>