Re: [PATCH] perf probe: Fix uninitialized variable

[James Clark <james.clark@linaro.org>]

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