Received-SPF: pass (google.com: domain of linux-kernel+bounces-165721-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240430184156.1824083-1-irogers@google.com>
In-Reply-To: <20240430184156.1824083-1-irogers@google.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Wed, 1 May 2024 12:46:38 -0700
Message-ID: <CAM9d7chFv38q+a9m_OE-dP1aSUpvd35tM1V7DA_t756O6hrTUA@mail.gmail.com>
Subject: Re: [PATCH v1] perf lock: More strdup argument freeing
To: Ian Rogers <irogers@google.com>
Cc: zhaimingbing <zhaimingbing@cmss.chinamobile.com>, 
	Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Kan Liang <kan.liang@linux.intel.com>, 
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 30, 2024 at 11:42=E2=80=AFAM Ian Rogers <irogers@google.com> wr=
ote:
>
> Leak sanitizer complains about the strdup-ed arguments not being
> freed. rec_argv is reordered and duplicates inserted, meaning making
> all its contents strdup-ed and freeing them all leads to double frees
> or leaks. Add an extra array to track strup-ed arguments and free
> them. This makes address sanitier running `perf test` "kernel lock
> contention analysis test" memory leak free.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-lock.c | 44 +++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 230461280e45..26c059397cdf 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -2230,10 +2230,11 @@ static int __cmd_record(int argc, const char **ar=
gv)
>         const char *callgraph_args[] =3D {
>                 "--call-graph", "fp," __stringify(CONTENTION_STACK_DEPTH)=
,
>         };
> -       unsigned int rec_argc, i, j, ret;
> +       unsigned int rec_argc, i, j, dups =3D 0, ret;
>         unsigned int nr_tracepoints;
>         unsigned int nr_callgraph_args =3D 0;
>         const char **rec_argv;
> +       char **to_free;
>         bool has_lock_stat =3D true;
>
>         for (i =3D 0; i < ARRAY_SIZE(lock_tracepoints); i++) {
> @@ -2270,28 +2271,25 @@ static int __cmd_record(int argc, const char **ar=
gv)
>         /* factor of 2 is for -e in front of each tracepoint */
>         rec_argc +=3D 2 * nr_tracepoints;
>
> -       rec_argv =3D calloc(rec_argc + 1, sizeof(char *));
> +       rec_argv =3D calloc(rec_argc + 1, sizeof(*rec_argv));
>         if (!rec_argv)
>                 return -ENOMEM;
>
> -       for (i =3D 0; i < ARRAY_SIZE(record_args); i++)
> -               rec_argv[i] =3D strdup(record_args[i]);
> -
> -       for (j =3D 0; j < nr_tracepoints; j++) {
> -               const char *ev_name;
> +       to_free =3D calloc(rec_argc, sizeof(*to_free));
> +       if (!to_free)
> +               return -ENOMEM;

Need to free rec_argv.  'goto out' would be fine.

>
> -               if (has_lock_stat)
> -                       ev_name =3D strdup(lock_tracepoints[j].name);
> -               else
> -                       ev_name =3D strdup(contention_tracepoints[j].name=
);
> -
> -               if (!ev_name) {
> -                       free(rec_argv);
> -                       return -ENOMEM;
> -               }
>
> +       for (i =3D 0; i < ARRAY_SIZE(record_args);) {
> +               to_free[dups] =3D strdup(record_args[i]);
> +               rec_argv[i++] =3D to_free[dups++];
> +       }
> +       for (j =3D 0; j < nr_tracepoints; j++) {
> +               to_free[dups] =3D strdup(has_lock_stat
> +                                      ? lock_tracepoints[j].name
> +                                      : contention_tracepoints[j].name);
>                 rec_argv[i++] =3D "-e";
> -               rec_argv[i++] =3D ev_name;
> +               rec_argv[i++] =3D to_free[dups++];

Now I'm curious why we copy the string in the first place.
Maybe not needed..?

Thanks,
Namhyung


>         }
>
>         for (j =3D 0; j < nr_callgraph_args; j++, i++)
> @@ -2302,7 +2300,17 @@ static int __cmd_record(int argc, const char **arg=
v)
>
>         BUG_ON(i !=3D rec_argc);
>
> -       ret =3D cmd_record(i, rec_argv);
> +       for (i =3D 0; i < dups; i++) {
> +               if (to_free[i] =3D=3D NULL) {
> +                       ret =3D -ENOMEM;
> +                       goto out;
> +               }
> +       }
> +       ret =3D cmd_record(rec_argc, rec_argv);
> +out:
> +       for (i =3D 0; i < dups; i++)
> +               zfree(&to_free[i]);
> +       free(to_free);
>         free(rec_argv);
>         return ret;
>  }
> --
> 2.45.0.rc0.197.gbae5840b3b-goog
>