[PATCH 0/2] perf: add missing evlist__delete when deleting

[PATCH 0/2] perf: add missing evlist__delete when deleting

[Riccardo Mancini]

ASan reports a memory leak caused by evlist not being deleted on exit in
perf-report and perf-script.
The problem is caused by evlist__delete not being called before
perf_session__delete, since the latter does not delete the evlist too.
These two patches add the missing deletes in both perf-report and 
perf-script.

ASan report follows:

$ ./perf script report flamegraph
=================================================================
==227640==ERROR: LeakSanitizer: detected memory leaks

<SNIP unrelated>

Indirect leak of 2704 byte(s) in 1 object(s) allocated from:
    #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
    #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
    #2 0x7f999e in evlist__new /home/user/linux/tools/perf/util/evlist.c:77:26
    #3 0x8ad938 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3797:20
    #4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #11 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

Indirect leak of 568 byte(s) in 1 object(s) allocated from:
    #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
    #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
    #2 0x80ce88 in evsel__new_idx /home/user/linux/tools/perf/util/evsel.c:268:24
    #3 0x8aed93 in evsel__new /home/user/linux/tools/perf/util/evsel.h:210:9
    #4 0x8ae07e in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3853:11
    #5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #12 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

Indirect leak of 264 byte(s) in 1 object(s) allocated from:
    #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
    #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
    #2 0xbe3e70 in xyarray__new /home/user/linux/tools/lib/perf/xyarray.c:10:23
    #3 0xbd7754 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:361:21
    #4 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
    #5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #12 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

Indirect leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
    #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
    #2 0xbd77e0 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:365:14
    #3 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
    #4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #11 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

Indirect leak of 7 byte(s) in 1 object(s) allocated from:
    #0 0x4b8207 in strdup (/home/user/linux/tools/perf/perf+0x4b8207)
    #1 0x8b4459 in evlist__set_event_name /home/user/linux/tools/perf/util/header.c:2292:16
    #2 0x89d862 in process_event_desc /home/user/linux/tools/perf/util/header.c:2313:3
    #3 0x8af319 in perf_file_section__process /home/user/linux/tools/perf/util/header.c:3651:9
    #4 0x8aa6e9 in perf_header__process_sections /home/user/linux/tools/perf/util/header.c:3427:9
    #5 0x8ae3e7 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3886:2
    #6 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
    #7 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
    #8 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
    #9 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
    #10 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
    #11 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
    #12 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
    #13 0x7f5260654b74  (/lib64/libc.so.6+0x27b74)

SUMMARY: AddressSanitizer: 3728 byte(s) leaked in 7 allocation(s).

Riccardo Mancini (2):
  perf report: delete evlist when deleting session
  perf script: delete evlist when deleting session

 tools/perf/builtin-report.c | 2 ++
 tools/perf/builtin-script.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH 1/2] perf report: delete evlist when deleting session

[Riccardo Mancini]

ASan reports a memory leak related to session->evlist never being deleted.
The evlist member is not deleted in perf_session__delete, so it should be
deleted separately.
This patch adds the missing deletion in perf-report.

Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
---
 tools/perf/builtin-report.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 36f9ccfeb38a..07d7693d44b8 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1618,6 +1618,7 @@ int cmd_report(int argc, const char **argv)
 
 	ret = __cmd_report(&report);
 	if (ret == K_SWITCH_INPUT_DATA || ret == K_RELOAD) {
+		evlist__delete(session->evlist);
 		perf_session__delete(session);
 		last_key = K_SWITCH_INPUT_DATA;
 		goto repeat;
@@ -1637,6 +1638,7 @@ int cmd_report(int argc, const char **argv)
 	}
 
 	zstd_fini(&(session->zstd_data));
+	evlist__delete(session->evlist);
 	perf_session__delete(session);
 	return ret;
 }
-- 
2.31.1

[PATCH 2/2] perf script: delete evlist when deleting session

[Riccardo Mancini]

ASan reports a memory leak related to session->evlist never being deleted.
The evlist member is not deleted in perf_session__delete, so it should be
deleted separately.
This patch adds the missing deletion in perf-script.

Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
---
 tools/perf/builtin-script.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 1280cbfad4db..635a1d9cfc88 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3991,7 +3991,7 @@ int cmd_script(int argc, const char **argv)
 		zfree(&script.ptime_range);
 	}
 
-	evlist__free_stats(session->evlist);
+	evlist__delete(session->evlist);
 	perf_session__delete(session);
 
 	if (script_started)
-- 
2.31.1

Re: [PATCH 2/2] perf script: delete evlist when deleting session

[Ian Rogers]

On Mon, Jun 21, 2021 at 4:44 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
>
> ASan reports a memory leak related to session->evlist never being deleted.
> The evlist member is not deleted in perf_session__delete, so it should be
> deleted separately.
> This patch adds the missing deletion in perf-script.
>
> Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> ---
>  tools/perf/builtin-script.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 1280cbfad4db..635a1d9cfc88 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -3991,7 +3991,7 @@ int cmd_script(int argc, const char **argv)
>                 zfree(&script.ptime_range);
>         }
>
> -       evlist__free_stats(session->evlist);

Should this be removed?

> +       evlist__delete(session->evlist);

If the perf session "owns" the evlist, would it be cleaner to add this
to perf_session__delete?

Thanks,
Ian

>         perf_session__delete(session);
>
>         if (script_started)
> --
> 2.31.1
>

Re: [PATCH 2/2] perf script: delete evlist when deleting session

[Riccardo Mancini]

Hi,

thanks for your comments.

On Mon, 2021-06-21 at 22:14 -0700, Ian Rogers wrote:
> On Mon, Jun 21, 2021 at 4:44 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> > 
> > ASan reports a memory leak related to session->evlist never being deleted.
> > The evlist member is not deleted in perf_session__delete, so it should be
> > deleted separately.
> > This patch adds the missing deletion in perf-script.
> > 
> > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > ---
> >  tools/perf/builtin-script.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > index 1280cbfad4db..635a1d9cfc88 100644
> > --- a/tools/perf/builtin-script.c
> > +++ b/tools/perf/builtin-script.c
> > @@ -3991,7 +3991,7 @@ int cmd_script(int argc, const char **argv)
> >                 zfree(&script.ptime_range);
> >         }
> > 
> > -       evlist__free_stats(session->evlist);
> 
> Should this be removed?

Probably not. I originally thought this was already taken care of by
evlist__delete, but it's not. 
Oddly, this issue is not causing a memory leak in my simple test.

> 
> > +       evlist__delete(session->evlist);
> 
> If the perf session "owns" the evlist, would it be cleaner to add this
> to perf_session__delete?

I thought about that too, but that's not always true.
E.g., in perf-record, __cmd_record calls perf_session__delete,then cmd_record
calls evlist__delete on rec->evlist, which points to the same location to which
session->evlist pointed. 

Thanks,
Riccardo

> 
> Thanks,
> Ian
> 
> >         perf_session__delete(session);
> > 
> >         if (script_started)
> > --
> > 2.31.1
> > 

Re: [PATCH 2/2] perf script: delete evlist when deleting session

[Ian Rogers]

On Tue, Jun 22, 2021 at 12:44 AM Riccardo Mancini <rickyman7@gmail.com> wrote:
>
> Hi,
>
> thanks for your comments.
>
> On Mon, 2021-06-21 at 22:14 -0700, Ian Rogers wrote:
> > On Mon, Jun 21, 2021 at 4:44 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> > >
> > > ASan reports a memory leak related to session->evlist never being deleted.
> > > The evlist member is not deleted in perf_session__delete, so it should be
> > > deleted separately.
> > > This patch adds the missing deletion in perf-script.
> > >
> > > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > > ---
> > >  tools/perf/builtin-script.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > > index 1280cbfad4db..635a1d9cfc88 100644
> > > --- a/tools/perf/builtin-script.c
> > > +++ b/tools/perf/builtin-script.c
> > > @@ -3991,7 +3991,7 @@ int cmd_script(int argc, const char **argv)
> > >                 zfree(&script.ptime_range);
> > >         }
> > >
> > > -       evlist__free_stats(session->evlist);
> >
> > Should this be removed?
>
> Probably not. I originally thought this was already taken care of by
> evlist__delete, but it's not.
> Oddly, this issue is not causing a memory leak in my simple test.
>
> >
> > > +       evlist__delete(session->evlist);
> >
> > If the perf session "owns" the evlist, would it be cleaner to add this
> > to perf_session__delete?
>
> I thought about that too, but that's not always true.
> E.g., in perf-record, __cmd_record calls perf_session__delete,then cmd_record
> calls evlist__delete on rec->evlist, which points to the same location to which
> session->evlist pointed.

Agreed. I find it hard to understand the ownership properties in the
perf code. The missing delete is an example of the owner of the evlist
(the caller) not "knowing" it needed cleaning up. I'd like it if we
documented things like perf_sessions' evlist to say not owned, user
must clean up. The makes it unambiguous who has to take
responsibility. Having things clean up after themselves is of course
easiest, hence wanting this to be in perf_session__delete.

Fwiw, I've been reading around things like sparse [1, 2] and Clang's
similar analysis [3] that people have looked to use like sparse [4]. I
don't see anything that handles memory allocation lifetimes, but
perhaps something will feed into C's standards by way of C++ [5].
Perhaps people have ideas to rewrite in checked C or Rust :-)

Some thoughts:
1) we can't have C++ as we're trying to follow kernel conventions [6]
2) we can't annotate code for things like sparse or thread safety
analysis, as checking for memory errors is out of scope for them, the
annotations don't exist, etc.
3) we can add comments, document the rules around pointers, perhaps
even invent empty annotations that may one day help with automated
checking.
4) we can try to clean up the ownership model to make bugs less likely.

I've heard concerns on non-kernel projects about annotation litter and
comments adding to complexity. I think your patch is good, it follows
the existing conventions. I wonder if we can learn something from the
fact the code was wrong to make it less likely we have wrong code in
the future. I'd be interested to hear what others think.

Thanks,
Ian

[1] https://lore.kernel.org/lkml/Pine.LNX.4.58.0410302005270.28839@ppc970.osdl.org/
[2] https://lwn.net/Articles/689907/
[3] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
[4] https://www.openwall.com/lists/kernel-hardening/2019/05/20/3
[5] https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/Lifetime.pdf
[6] even concatenating a string is error prone in C :-(
https://lore.kernel.org/lkml/YMzOpgZPJeC2jGKf@kernel.org/

> Thanks,
> Riccardo
>
> >
> > Thanks,
> > Ian
> >
> > >         perf_session__delete(session);
> > >
> > >         if (script_started)
> > > --
> > > 2.31.1
> > >
>
>

Re: [PATCH 2/2] perf script: delete evlist when deleting session

[Arnaldo Carvalho de Melo]

Em Tue, Jun 22, 2021 at 09:33:23AM -0700, Ian Rogers escreveu:
> On Tue, Jun 22, 2021 at 12:44 AM Riccardo Mancini <rickyman7@gmail.com> wrote:
> >
> > Hi,
> >
> > thanks for your comments.
> >
> > On Mon, 2021-06-21 at 22:14 -0700, Ian Rogers wrote:
> > > On Mon, Jun 21, 2021 at 4:44 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> > > >
> > > > ASan reports a memory leak related to session->evlist never being deleted.
> > > > The evlist member is not deleted in perf_session__delete, so it should be
> > > > deleted separately.
> > > > This patch adds the missing deletion in perf-script.
> > > >
> > > > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > > > ---
> > > >  tools/perf/builtin-script.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > > > index 1280cbfad4db..635a1d9cfc88 100644
> > > > --- a/tools/perf/builtin-script.c
> > > > +++ b/tools/perf/builtin-script.c
> > > > @@ -3991,7 +3991,7 @@ int cmd_script(int argc, const char **argv)
> > > >                 zfree(&script.ptime_range);
> > > >         }
> > > >
> > > > -       evlist__free_stats(session->evlist);
> > >
> > > Should this be removed?
> >
> > Probably not. I originally thought this was already taken care of by
> > evlist__delete, but it's not.
> > Oddly, this issue is not causing a memory leak in my simple test.
> >
> > >
> > > > +       evlist__delete(session->evlist);

This looks like a bug, if it is a 'session' member, its a session method
that should delete it, probably perf_session__delete().

> > > If the perf session "owns" the evlist, would it be cleaner to add this
> > > to perf_session__delete?
> >
> > I thought about that too, but that's not always true.
> > E.g., in perf-record, __cmd_record calls perf_session__delete,then cmd_record
> > calls evlist__delete on rec->evlist, which points to the same location to which
> > session->evlist pointed.
> 
> Agreed. I find it hard to understand the ownership properties in the
> perf code. The missing delete is an example of the owner of the evlist
> (the caller) not "knowing" it needed cleaning up. I'd like it if we
> documented things like perf_sessions' evlist to say not owned, user
> must clean up. The makes it unambiguous who has to take
> responsibility. Having things clean up after themselves is of course
> easiest, hence wanting this to be in perf_session__delete.

This specific case, from just reading the description on this message,
looks just like a bug/thinko.
 
> Fwiw, I've been reading around things like sparse [1, 2] and Clang's
> similar analysis [3] that people have looked to use like sparse [4]. I
> don't see anything that handles memory allocation lifetimes, but
> perhaps something will feed into C's standards by way of C++ [5].
> Perhaps people have ideas to rewrite in checked C or Rust :-)
> 
> Some thoughts:
> 1) we can't have C++ as we're trying to follow kernel conventions [6]
> 2) we can't annotate code for things like sparse or thread safety
> analysis, as checking for memory errors is out of scope for them, the
> annotations don't exist, etc.
> 3) we can add comments, document the rules around pointers, perhaps
> even invent empty annotations that may one day help with automated
> checking.
> 4) we can try to clean up the ownership model to make bugs less likely.
> 
> I've heard concerns on non-kernel projects about annotation litter and
> comments adding to complexity. I think your patch is good, it follows
> the existing conventions. I wonder if we can learn something from the
> fact the code was wrong to make it less likely we have wrong code in
> the future. I'd be interested to hear what others think.
> 
> Thanks,
> Ian
> 
> [1] https://lore.kernel.org/lkml/Pine.LNX.4.58.0410302005270.28839@ppc970.osdl.org/
> [2] https://lwn.net/Articles/689907/
> [3] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
> [4] https://www.openwall.com/lists/kernel-hardening/2019/05/20/3
> [5] https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/Lifetime.pdf
> [6] even concatenating a string is error prone in C :-(
> https://lore.kernel.org/lkml/YMzOpgZPJeC2jGKf@kernel.org/
> 
> > Thanks,
> > Riccardo
> >
> > >
> > > Thanks,
> > > Ian
> > >
> > > >         perf_session__delete(session);
> > > >
> > > >         if (script_started)
> > > > --
> > > > 2.31.1
> > > >
> >
> >

-- 

- Arnaldo

Re: [PATCH 2/2] perf script: delete evlist when deleting session

[Ian Rogers]

On Tue, Jun 22, 2021 at 10:42 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, Jun 22, 2021 at 09:33:23AM -0700, Ian Rogers escreveu:
> > On Tue, Jun 22, 2021 at 12:44 AM Riccardo Mancini <rickyman7@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > thanks for your comments.
> > >
> > > On Mon, 2021-06-21 at 22:14 -0700, Ian Rogers wrote:
> > > > On Mon, Jun 21, 2021 at 4:44 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> > > > >
> > > > > ASan reports a memory leak related to session->evlist never being deleted.
> > > > > The evlist member is not deleted in perf_session__delete, so it should be
> > > > > deleted separately.
> > > > > This patch adds the missing deletion in perf-script.
> > > > >
> > > > > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > > > > ---
> > > > >  tools/perf/builtin-script.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > > > > index 1280cbfad4db..635a1d9cfc88 100644
> > > > > --- a/tools/perf/builtin-script.c
> > > > > +++ b/tools/perf/builtin-script.c
> > > > > @@ -3991,7 +3991,7 @@ int cmd_script(int argc, const char **argv)
> > > > >                 zfree(&script.ptime_range);
> > > > >         }
> > > > >
> > > > > -       evlist__free_stats(session->evlist);
> > > >
> > > > Should this be removed?
> > >
> > > Probably not. I originally thought this was already taken care of by
> > > evlist__delete, but it's not.
> > > Oddly, this issue is not causing a memory leak in my simple test.
> > >
> > > >
> > > > > +       evlist__delete(session->evlist);
>
> This looks like a bug, if it is a 'session' member, its a session method
> that should delete it, probably perf_session__delete().
>
> > > > If the perf session "owns" the evlist, would it be cleaner to add this
> > > > to perf_session__delete?
> > >
> > > I thought about that too, but that's not always true.
> > > E.g., in perf-record, __cmd_record calls perf_session__delete,then cmd_record
> > > calls evlist__delete on rec->evlist, which points to the same location to which
> > > session->evlist pointed.
> >
> > Agreed. I find it hard to understand the ownership properties in the
> > perf code. The missing delete is an example of the owner of the evlist
> > (the caller) not "knowing" it needed cleaning up. I'd like it if we
> > documented things like perf_sessions' evlist to say not owned, user
> > must clean up. The makes it unambiguous who has to take
> > responsibility. Having things clean up after themselves is of course
> > easiest, hence wanting this to be in perf_session__delete.
>
> This specific case, from just reading the description on this message,
> looks just like a bug/thinko.

Ack. Definitely worth merging the change. I think this is about the
7th address sanitizer bug Riccardo has fixed. Namhyung, Numfor, Luke
and myself have also contributed similar fixes. We set up some fuzz
testing on libtraceevent and there are currently 12 issues we've found
there. The nice thing with sparse compared to address sanitizer is the
compiler will point at the problem, you don't need to trigger an issue
in a test. There are some complicated ownership rules in session and
also in the reference counting issues that Riccardo has raised, so
perhaps there's scope for some more comments or other tidy ups.

Thanks,
Ian
> > Fwiw, I've been reading around things like sparse [1, 2] and Clang's
> > similar analysis [3] that people have looked to use like sparse [4]. I
> > don't see anything that handles memory allocation lifetimes, but
> > perhaps something will feed into C's standards by way of C++ [5].
> > Perhaps people have ideas to rewrite in checked C or Rust :-)
> >
> > Some thoughts:
> > 1) we can't have C++ as we're trying to follow kernel conventions [6]
> > 2) we can't annotate code for things like sparse or thread safety
> > analysis, as checking for memory errors is out of scope for them, the
> > annotations don't exist, etc.
> > 3) we can add comments, document the rules around pointers, perhaps
> > even invent empty annotations that may one day help with automated
> > checking.
> > 4) we can try to clean up the ownership model to make bugs less likely.
> >
> > I've heard concerns on non-kernel projects about annotation litter and
> > comments adding to complexity. I think your patch is good, it follows
> > the existing conventions. I wonder if we can learn something from the
> > fact the code was wrong to make it less likely we have wrong code in
> > the future. I'd be interested to hear what others think.
> >
> > Thanks,
> > Ian
> >
> > [1] https://lore.kernel.org/lkml/Pine.LNX.4.58.0410302005270.28839@ppc970.osdl.org/
> > [2] https://lwn.net/Articles/689907/
> > [3] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
> > [4] https://www.openwall.com/lists/kernel-hardening/2019/05/20/3
> > [5] https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/Lifetime.pdf
> > [6] even concatenating a string is error prone in C :-(
> > https://lore.kernel.org/lkml/YMzOpgZPJeC2jGKf@kernel.org/
> >
> > > Thanks,
> > > Riccardo
> > >
> > > >
> > > > Thanks,
> > > > Ian
> > > >
> > > > >         perf_session__delete(session);
> > > > >
> > > > >         if (script_started)
> > > > > --
> > > > > 2.31.1
> > > > >
> > >
> > >
>
> --
>
> - Arnaldo