[PATCH] perf jevents: Fix suspicious code in fixregex()

[PATCH] perf jevents: Fix suspicious code in fixregex()

[Namhyung Kim]

The new string should have enough space for the original string and
the back slashes IMHO.

Cc: John Garry <john.garry@huawei.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/pmu-events/jevents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index fa86c5f997cc..fc9c158bfa13 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -137,7 +137,7 @@ static char *fixregex(char *s)
 		return s;
 
 	/* allocate space for a new string */
-	fixed = (char *) malloc(len + 1);
+	fixed = (char *) malloc(len + esc_count + 1);
 	if (!fixed)
 		return NULL;
 
-- 
2.28.0.402.g5ffc5be6b7-goog

Re: [PATCH] perf jevents: Fix suspicious code in fixregex()

[Ian Rogers]

On Thu, Sep 3, 2020 at 8:25 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The new string should have enough space for the original string and
> the back slashes IMHO.
>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Kajol Jain <kjain@linux.ibm.com>
> Cc: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Definitely looks like the right fix. I'm surprised this hasn't shown
up in sanitizer testing.

Thanks,
Ian

> ---
>  tools/perf/pmu-events/jevents.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index fa86c5f997cc..fc9c158bfa13 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -137,7 +137,7 @@ static char *fixregex(char *s)
>                 return s;
>
>         /* allocate space for a new string */
> -       fixed = (char *) malloc(len + 1);
> +       fixed = (char *) malloc(len + esc_count + 1);
>         if (!fixed)
>                 return NULL;
>
> --
> 2.28.0.402.g5ffc5be6b7-goog
>

Re: [PATCH] perf jevents: Fix suspicious code in fixregex()

[Arnaldo Carvalho de Melo]

Em Thu, Sep 03, 2020 at 10:47:39AM -0700, Ian Rogers escreveu:
> On Thu, Sep 3, 2020 at 8:25 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > The new string should have enough space for the original string and
> > the back slashes IMHO.

> > Cc: John Garry <john.garry@huawei.com>
> > Cc: Kajol Jain <kjain@linux.ibm.com>
> > Cc: Ian Rogers <irogers@google.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> 
> Definitely looks like the right fix. I'm surprised this hasn't shown
> up in sanitizer testing.

Yeap, good catch! Namyung you forgot to add the Fixes tag + Cc the patch
author that introduced that bug, I did it:

Cc: William Cohen <wcohen@redhat.com>
Fixes: fbc2844e84038ce3 ("perf vendor events: Use more flexible pattern matching for CPU identification for mapfile.csv"

Please consider doing it next time :-)

Thanks a lot!

- Arnaldo
 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/pmu-events/jevents.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> > index fa86c5f997cc..fc9c158bfa13 100644
> > --- a/tools/perf/pmu-events/jevents.c
> > +++ b/tools/perf/pmu-events/jevents.c
> > @@ -137,7 +137,7 @@ static char *fixregex(char *s)
> >                 return s;
> >
> >         /* allocate space for a new string */
> > -       fixed = (char *) malloc(len + 1);
> > +       fixed = (char *) malloc(len + esc_count + 1);
> >         if (!fixed)
> >                 return NULL;
> >
> > --
> > 2.28.0.402.g5ffc5be6b7-goog
> >

-- 

- Arnaldo

Re: [PATCH] perf jevents: Fix suspicious code in fixregex()

[Namhyung Kim]

Hi Arnaldo and Ian,

On Fri, Sep 4, 2020 at 3:41 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Thu, Sep 03, 2020 at 10:47:39AM -0700, Ian Rogers escreveu:
> > On Thu, Sep 3, 2020 at 8:25 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > The new string should have enough space for the original string and
> > > the back slashes IMHO.
>
> > > Cc: John Garry <john.garry@huawei.com>
> > > Cc: Kajol Jain <kjain@linux.ibm.com>
> > > Cc: Ian Rogers <irogers@google.com>
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >
> > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> > Definitely looks like the right fix. I'm surprised this hasn't shown
> > up in sanitizer testing.

I guess the code didn't run on most arch (including x86) since
they don't have backslashes.

>
> Yeap, good catch! Namyung you forgot to add the Fixes tag + Cc the patch
> author that introduced that bug, I did it:
>
> Cc: William Cohen <wcohen@redhat.com>
> Fixes: fbc2844e84038ce3 ("perf vendor events: Use more flexible pattern matching for CPU identification for mapfile.csv"
>
> Please consider doing it next time :-)

Oh, right!  Will do it later..

Thanks
Namhyung

Re: [PATCH] perf jevents: Fix suspicious code in fixregex()

[Arnaldo Carvalho de Melo]

Em Fri, Sep 04, 2020 at 10:38:35AM +0900, Namhyung Kim escreveu:
> On Fri, Sep 4, 2020 at 3:41 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > Em Thu, Sep 03, 2020 at 10:47:39AM -0700, Ian Rogers escreveu:
> > > On Thu, Sep 3, 2020 at 8:25 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > The new string should have enough space for the original string and
> > > > the back slashes IMHO.
> > > > Cc: John Garry <john.garry@huawei.com>
> > > > Cc: Kajol Jain <kjain@linux.ibm.com>
> > > > Cc: Ian Rogers <irogers@google.com>
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> > > Definitely looks like the right fix. I'm surprised this hasn't shown
> > > up in sanitizer testing.
 
> I guess the code didn't run on most arch (including x86) since
> they don't have backslashes.

> > Yeap, good catch! Namyung you forgot to add the Fixes tag + Cc the patch
> > author that introduced that bug, I did it:

> > Cc: William Cohen <wcohen@redhat.com>
> > Fixes: fbc2844e84038ce3 ("perf vendor events: Use more flexible pattern matching for CPU identification for mapfile.csv"

> > Please consider doing it next time :-)
 
> Oh, right!  Will do it later..

Thanks!

- Arnaldo