Re: [PATCH] perf probe: Fix uninitialized variable

[James Clark]

On 11/12/2024 12:43 am, Arnaldo Carvalho de Melo wrote:
> On Tue, Dec 10, 2024, 7:55 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> > Hi,
> >
> > On Mon,  9 Dec 2024 17:12:21 +0000
> > James Clark <james.clark@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>