[PATCH v1 1/2] perf disasm: Fix memory leak for locked operations

[PATCH v1 1/2] perf disasm: Fix memory leak for locked operations

[Ian Rogers]

lock__parse calls disasm_line__parse passing
&ops->locked.ins.name. Ensure ops->locked.ins.name is freed in
lock__delete.

Found with lock/leak sanitizer.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/disasm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 22289003e16d..226d2181f694 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -566,6 +566,7 @@ static void lock__delete(struct ins_operands *ops)
 		ins_ops__delete(ops->locked.ops);
 
 	zfree(&ops->locked.ops);
+	zfree(&ops->locked.ins.name);
 	zfree(&ops->target.raw);
 	zfree(&ops->target.name);
 }
-- 
2.46.0.76.ge559c4bf1a-goog

[PATCH v1 2/2] perf test annotate: Dump trapping test in trap handler

[Ian Rogers]

Help to better identify the location of test failures but dumping the
failing test in the trap handler.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/annotate.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/shell/annotate.sh b/tools/perf/tests/shell/annotate.sh
index b072d9b97387..2ccf4f1d46b6 100755
--- a/tools/perf/tests/shell/annotate.sh
+++ b/tools/perf/tests/shell/annotate.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 # perf annotate basic tests
 # SPDX-License-Identifier: GPL-2.0
 
@@ -28,6 +28,7 @@ cleanup() {
 }
 
 trap_cleanup() {
+  echo "Unexpected signal in ${FUNCNAME[1]}"
   cleanup
   exit 1
 }
-- 
2.46.0.76.ge559c4bf1a-goog

Re: [PATCH v1 1/2] perf disasm: Fix memory leak for locked operations

[Arnaldo Carvalho de Melo]

On Mon, Aug 12, 2024 at 09:06:12PM -0700, Ian Rogers wrote:
> lock__parse calls disasm_line__parse passing
> &ops->locked.ins.name. Ensure ops->locked.ins.name is freed in
> lock__delete.
> 
> Found with lock/leak sanitizer.

Applied both patches to perf-tools-next.

Thanks,

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/disasm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 22289003e16d..226d2181f694 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -566,6 +566,7 @@ static void lock__delete(struct ins_operands *ops)
>  		ins_ops__delete(ops->locked.ops);
>  
>  	zfree(&ops->locked.ops);
> +	zfree(&ops->locked.ins.name);
>  	zfree(&ops->target.raw);
>  	zfree(&ops->target.name);
>  }
> -- 
> 2.46.0.76.ge559c4bf1a-goog

Re: [PATCH v1 1/2] perf disasm: Fix memory leak for locked operations

[Ian Rogers]

On Tue, Aug 13, 2024 at 7:53 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Aug 12, 2024 at 09:06:12PM -0700, Ian Rogers wrote:
> > lock__parse calls disasm_line__parse passing
> > &ops->locked.ins.name. Ensure ops->locked.ins.name is freed in
> > lock__delete.
> >
> > Found with lock/leak sanitizer.

Ooops, I meant address/leak sanitizer.

> Applied both patches to perf-tools-next.

Thanks, could you fix the commit message.

Thanks,
Ian

> Thanks,
>
> - Arnaldo
>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/disasm.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> > index 22289003e16d..226d2181f694 100644
> > --- a/tools/perf/util/disasm.c
> > +++ b/tools/perf/util/disasm.c
> > @@ -566,6 +566,7 @@ static void lock__delete(struct ins_operands *ops)
> >               ins_ops__delete(ops->locked.ops);
> >
> >       zfree(&ops->locked.ops);
> > +     zfree(&ops->locked.ins.name);
> >       zfree(&ops->target.raw);
> >       zfree(&ops->target.name);
> >  }
> > --
> > 2.46.0.76.ge559c4bf1a-goog

Re: [PATCH v1 1/2] perf disasm: Fix memory leak for locked operations

[Arnaldo Carvalho de Melo]

On Tue, Aug 13, 2024 at 09:04:57AM -0700, Ian Rogers wrote:
> On Tue, Aug 13, 2024 at 7:53 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Mon, Aug 12, 2024 at 09:06:12PM -0700, Ian Rogers wrote:
> > > lock__parse calls disasm_line__parse passing
> > > &ops->locked.ins.name. Ensure ops->locked.ins.name is freed in
> > > lock__delete.
> > >
> > > Found with lock/leak sanitizer.
> 
> Ooops, I meant address/leak sanitizer.
> 
> > Applied both patches to perf-tools-next.
> 
> Thanks, could you fix the commit message.

Sure,

- Arnaldo

Re: [PATCH v1 1/2] perf disasm: Fix memory leak for locked operations

[Ian Rogers]

On Tue, Aug 13, 2024 at 11:11 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, Aug 13, 2024 at 09:04:57AM -0700, Ian Rogers wrote:
> > On Tue, Aug 13, 2024 at 7:53 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > On Mon, Aug 12, 2024 at 09:06:12PM -0700, Ian Rogers wrote:
> > > > lock__parse calls disasm_line__parse passing
> > > > &ops->locked.ins.name. Ensure ops->locked.ins.name is freed in
> > > > lock__delete.
> > > >
> > > > Found with lock/leak sanitizer.
> >
> > Ooops, I meant address/leak sanitizer.
> >
> > > Applied both patches to perf-tools-next.
> >
> > Thanks, could you fix the commit message.
>
> Sure,

Also, it'd be good if maybe Namhyung could take a look. I did things
this way as it made sense to me, but we have nested things going on
and potentially the free would be more natural in ins_ops__delete.

Thanks,
Ian

Re: [PATCH v1 1/2] perf disasm: Fix memory leak for locked operations

[Arnaldo Carvalho de Melo]

On Tue, Aug 13, 2024 at 12:11:21PM -0700, Ian Rogers wrote:
> On Tue, Aug 13, 2024 at 11:11 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > On Tue, Aug 13, 2024 at 09:04:57AM -0700, Ian Rogers wrote:
> > > On Tue, Aug 13, 2024 at 7:53 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > > On Mon, Aug 12, 2024 at 09:06:12PM -0700, Ian Rogers wrote:
> > > > > lock__parse calls disasm_line__parse passing
> > > > > &ops->locked.ins.name. Ensure ops->locked.ins.name is freed in
> > > > > lock__delete.

> > > > > Found with lock/leak sanitizer.

> > > Ooops, I meant address/leak sanitizer.

> > > > Applied both patches to perf-tools-next.

> > > Thanks, could you fix the commit message.

> > Sure,
 
> Also, it'd be good if maybe Namhyung could take a look. I did things
> this way as it made sense to me, but we have nested things going on
> and potentially the free would be more natural in ins_ops__delete.

ins_ops__delete() would have to look it is operating on a lock operation
to access the right member of the unnamed union (locked) to get that
string pointer to free.

lock__delete() is the 'lock' destructor, that knows these details, so
its the right place to do the freeing.

I.e.:

static struct ins_ops lock_ops = {
        .free      = lock__delete,
        .parse     = lock__parse,
        .scnprintf = lock__scnprintf,
};

When it was introduced:

commit c46219ac34f0f365bac700ca6a10ef979c643233
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Sat May 12 13:26:20 2012 -0300

    perf annotate: Introduce ->free() method in ins_ops
    
    So that we don't special case disasm_line__free, allowing each
    instruction class to provide a specialized destructor, like is needed
    for 'lock'.

- Arnaldo

Re: [PATCH v1 1/2] perf disasm: Fix memory leak for locked operations

[Namhyung Kim]

Hi Ian,

On Tue, Aug 13, 2024 at 12:11 PM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Aug 13, 2024 at 11:11 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Tue, Aug 13, 2024 at 09:04:57AM -0700, Ian Rogers wrote:
> > > On Tue, Aug 13, 2024 at 7:53 AM Arnaldo Carvalho de Melo
> > > <acme@kernel.org> wrote:
> > > >
> > > > On Mon, Aug 12, 2024 at 09:06:12PM -0700, Ian Rogers wrote:
> > > > > lock__parse calls disasm_line__parse passing
> > > > > &ops->locked.ins.name. Ensure ops->locked.ins.name is freed in
> > > > > lock__delete.
> > > > >
> > > > > Found with lock/leak sanitizer.
> > >
> > > Ooops, I meant address/leak sanitizer.
> > >
> > > > Applied both patches to perf-tools-next.
> > >
> > > Thanks, could you fix the commit message.
> >
> > Sure,
>
> Also, it'd be good if maybe Namhyung could take a look. I did things
> this way as it made sense to me, but we have nested things going on
> and potentially the free would be more natural in ins_ops__delete.

Looks good to me.

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

Re: [PATCH v1 1/2] perf disasm: Fix memory leak for locked operations

[Ian Rogers]

On Tue, Aug 13, 2024 at 3:25 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Tue, Aug 13, 2024 at 12:11 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Tue, Aug 13, 2024 at 11:11 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > On Tue, Aug 13, 2024 at 09:04:57AM -0700, Ian Rogers wrote:
> > > > On Tue, Aug 13, 2024 at 7:53 AM Arnaldo Carvalho de Melo
> > > > <acme@kernel.org> wrote:
> > > > >
> > > > > On Mon, Aug 12, 2024 at 09:06:12PM -0700, Ian Rogers wrote:
> > > > > > lock__parse calls disasm_line__parse passing
> > > > > > &ops->locked.ins.name. Ensure ops->locked.ins.name is freed in
> > > > > > lock__delete.
> > > > > >
> > > > > > Found with lock/leak sanitizer.
> > > >
> > > > Ooops, I meant address/leak sanitizer.
> > > >
> > > > > Applied both patches to perf-tools-next.
> > > >
> > > > Thanks, could you fix the commit message.
> > >
> > > Sure,
> >
> > Also, it'd be good if maybe Namhyung could take a look. I did things
> > this way as it made sense to me, but we have nested things going on
> > and potentially the free would be more natural in ins_ops__delete.
>
> Looks good to me.
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks Namhyung, thanks Arnaldo. I think git blame said Namhyung
because of a refactoring. I'm glad the free is in the correct
location.

Thanks,
Ian