[PATCH] perf script python: Fix dict reference counting

[PATCH] perf script python: Fix dict reference counting

[Janne Huttunen]

The dictionaries are attached to the parameter tuple that steals the
references. The code should not decrement the reference counters
explicitly. Otherwise the objects might be released while they are
still in use which may cause perf crashes, assertions or just plain
weird behavior like unexpected data changes in stored objects.

Signed-off-by: Janne Huttunen <janne.huttunen@nokia.com>
---
 tools/perf/util/scripting-engines/trace-event-python.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 46e9e19..60fce44 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -908,14 +908,11 @@ static void python_process_tracepoint(struct perf_sample *sample,
 	if (_PyTuple_Resize(&t, n) == -1)
 		Py_FatalError("error resizing Python tuple");
 
-	if (!dict) {
+	if (!dict)
 		call_object(handler, t, handler_name);
-	} else {
+	else
 		call_object(handler, t, default_handler_name);
-		Py_DECREF(dict);
-	}
 
-	Py_XDECREF(all_entries_dict);
 	Py_DECREF(t);
 }
 
@@ -1235,7 +1232,6 @@ static void python_process_general_event(struct perf_sample *sample,
 
 	call_object(handler, t, handler_name);
 
-	Py_DECREF(dict);
 	Py_DECREF(t);
 }
 
-- 
2.5.5

Re: [PATCH] perf script python: Fix dict reference counting

[Jiri Olsa]

On Fri, Jul 06, 2018 at 09:53:44AM +0300, Janne Huttunen wrote:
> The dictionaries are attached to the parameter tuple that steals the
> references. The code should not decrement the reference counters
> explicitly. Otherwise the objects might be released while they are
> still in use which may cause perf crashes, assertions or just plain
> weird behavior like unexpected data changes in stored objects.
> 
> Signed-off-by: Janne Huttunen <janne.huttunen@nokia.com>
> ---
>  tools/perf/util/scripting-engines/trace-event-python.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> index 46e9e19..60fce44 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> @@ -908,14 +908,11 @@ static void python_process_tracepoint(struct perf_sample *sample,
>  	if (_PyTuple_Resize(&t, n) == -1)
>  		Py_FatalError("error resizing Python tuple");
>  
> -	if (!dict) {
> +	if (!dict)
>  		call_object(handler, t, handler_name);
> -	} else {
> +	else
>  		call_object(handler, t, default_handler_name);
> -		Py_DECREF(dict);
> -	}
>  
> -	Py_XDECREF(all_entries_dict);
>  	Py_DECREF(t);
>  }
>  
> @@ -1235,7 +1232,6 @@ static void python_process_general_event(struct perf_sample *sample,
>  
>  	call_object(handler, t, handler_name);
>  
> -	Py_DECREF(dict);
>  	Py_DECREF(t);

so the dict is released when the tuple is released?

jirka

Re: [PATCH] perf script python: Fix dict reference counting

[Janne Huttunen]

On Sun, 2018-07-08 at 13:17 +0200, Jiri Olsa wrote:
> On Fri, Jul 06, 2018 at 09:53:44AM +0300, Janne Huttunen wrote:
> > 
> > The dictionaries are attached to the parameter tuple that steals the
> > references. The code should not decrement the reference counters
> > explicitly. Otherwise the objects might be released while they are
> > still in use which may cause perf crashes, assertions or just plain
> > weird behavior like unexpected data changes in stored objects.
> > 
> > Signed-off-by: Janne Huttunen <janne.huttunen@nokia.com>
> > ---
> >  tools/perf/util/scripting-engines/trace-event-python.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> > index 46e9e19..60fce44 100644
> > --- a/tools/perf/util/scripting-engines/trace-event-python.c
> > +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> > @@ -908,14 +908,11 @@ static void python_process_tracepoint(struct perf_sample *sample,
> >  	if (_PyTuple_Resize(&t, n) == -1)
> >  		Py_FatalError("error resizing Python tuple");
> >  
> > -	if (!dict) {
> > +	if (!dict)
> >  		call_object(handler, t, handler_name);
> > -	} else {
> > +	else
> >  		call_object(handler, t, default_handler_name);
> > -		Py_DECREF(dict);
> > -	}
> >  
> > -	Py_XDECREF(all_entries_dict);
> >  	Py_DECREF(t);
> >  }
> >  
> > @@ -1235,7 +1232,6 @@ static void python_process_general_event(struct perf_sample *sample,
> >  
> >  	call_object(handler, t, handler_name);
> >  
> > -	Py_DECREF(dict);
> >  	Py_DECREF(t);
>
> so the dict is released when the tuple is released?

To the best of my knowledge, yes.

As far as I can see, there is only a single reference to each dict
and according to the Python documentation PyTuple_SetItem() "steals"
the reference passed to it. If so, afterwards the tuple owns the
only reference to the dict(s) and should take care of releasing
them when appropriate.

I even built libpython with reference debugging enabled and when I
run perf without the fix I get this:

Fatal Python error: Objects/tupleobject.c:238 object at 0x7f10f2041b40 has negative ref count -1
Aborted (core dumped)

With the fix I get no errors.

Re: [PATCH] perf script python: Fix dict reference counting

[Jiri Olsa]

On Mon, Jul 09, 2018 at 12:25:33PM +0300, Janne Huttunen wrote:
> On Sun, 2018-07-08 at 13:17 +0200, Jiri Olsa wrote:
> > On Fri, Jul 06, 2018 at 09:53:44AM +0300, Janne Huttunen wrote:
> > > 
> > > The dictionaries are attached to the parameter tuple that steals the
> > > references. The code should not decrement the reference counters
> > > explicitly. Otherwise the objects might be released while they are
> > > still in use which may cause perf crashes, assertions or just plain
> > > weird behavior like unexpected data changes in stored objects.
> > > 
> > > Signed-off-by: Janne Huttunen <janne.huttunen@nokia.com>
> > > ---
> > >  tools/perf/util/scripting-engines/trace-event-python.c | 8 ++------
> > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> > > index 46e9e19..60fce44 100644
> > > --- a/tools/perf/util/scripting-engines/trace-event-python.c
> > > +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> > > @@ -908,14 +908,11 @@ static void python_process_tracepoint(struct perf_sample *sample,
> > >  	if (_PyTuple_Resize(&t, n) == -1)
> > >  		Py_FatalError("error resizing Python tuple");
> > >  
> > > -	if (!dict) {
> > > +	if (!dict)
> > >  		call_object(handler, t, handler_name);
> > > -	} else {
> > > +	else
> > >  		call_object(handler, t, default_handler_name);
> > > -		Py_DECREF(dict);
> > > -	}
> > >  
> > > -	Py_XDECREF(all_entries_dict);
> > >  	Py_DECREF(t);
> > >  }
> > >  
> > > @@ -1235,7 +1232,6 @@ static void python_process_general_event(struct perf_sample *sample,
> > >  
> > >  	call_object(handler, t, handler_name);
> > >  
> > > -	Py_DECREF(dict);
> > >  	Py_DECREF(t);
> >
> > so the dict is released when the tuple is released?
> 
> To the best of my knowledge, yes.
> 
> As far as I can see, there is only a single reference to each dict
> and according to the Python documentation PyTuple_SetItem() "steals"
> the reference passed to it. If so, afterwards the tuple owns the
> only reference to the dict(s) and should take care of releasing
> them when appropriate.
> 
> I even built libpython with reference debugging enabled and when I
> run perf without the fix I get this:
> 
> Fatal Python error: Objects/tupleobject.c:238 object at 0x7f10f2041b40 has negative ref count -1
> Aborted (core dumped)
> 
> With the fix I get no errors.
> 

ok, please include and mention that crash in the changelog

thanks,
jirka

[PATCH v2] perf script python: Fix dict reference counting

[Janne Huttunen]

The dictionaries are attached to the parameter tuple that steals
the references and takes care of releasing them when appropriate.
The code should not decrement the reference counts explicitly.
E.g. if libpython has been built with reference debugging enabled,
the superfluous DECREFs will trigger this error when running perf
script:

  Fatal Python error: Objects/tupleobject.c:238 object at
  0x7f10f2041b40 has negative ref count -1
  Aborted (core dumped)

If the reference debugging is not enabled, the superfluous DECREFs
might cause the dict objects to be silently released while they are
still in use. This may trigger various other assertions or just
cause perf crashes and/or weird and unexpected data changes in the
stored Python objects.

Signed-off-by: Janne Huttunen <janne.huttunen@nokia.com>
---
 tools/perf/util/scripting-engines/trace-event-python.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 46e9e19..60fce44 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -908,14 +908,11 @@ static void python_process_tracepoint(struct perf_sample *sample,
 	if (_PyTuple_Resize(&t, n) == -1)
 		Py_FatalError("error resizing Python tuple");
 
-	if (!dict) {
+	if (!dict)
 		call_object(handler, t, handler_name);
-	} else {
+	else
 		call_object(handler, t, default_handler_name);
-		Py_DECREF(dict);
-	}
 
-	Py_XDECREF(all_entries_dict);
 	Py_DECREF(t);
 }
 
@@ -1235,7 +1232,6 @@ static void python_process_general_event(struct perf_sample *sample,
 
 	call_object(handler, t, handler_name);
 
-	Py_DECREF(dict);
 	Py_DECREF(t);
 }
 
-- 
2.5.5

Re: [PATCH v2] perf script python: Fix dict reference counting

[Jiri Olsa]

On Mon, Jul 09, 2018 at 01:59:50PM +0300, Janne Huttunen wrote:
> The dictionaries are attached to the parameter tuple that steals
> the references and takes care of releasing them when appropriate.
> The code should not decrement the reference counts explicitly.
> E.g. if libpython has been built with reference debugging enabled,
> the superfluous DECREFs will trigger this error when running perf
> script:
> 
>   Fatal Python error: Objects/tupleobject.c:238 object at
>   0x7f10f2041b40 has negative ref count -1
>   Aborted (core dumped)
> 
> If the reference debugging is not enabled, the superfluous DECREFs
> might cause the dict objects to be silently released while they are
> still in use. This may trigger various other assertions or just
> cause perf crashes and/or weird and unexpected data changes in the
> stored Python objects.
> 
> Signed-off-by: Janne Huttunen <janne.huttunen@nokia.com>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

Re: [PATCH v2] perf script python: Fix dict reference counting

[Namhyung Kim]

Hell,

On Mon, Jul 09, 2018 at 01:59:50PM +0300, Janne Huttunen wrote:
> The dictionaries are attached to the parameter tuple that steals
> the references and takes care of releasing them when appropriate.
> The code should not decrement the reference counts explicitly.
> E.g. if libpython has been built with reference debugging enabled,
> the superfluous DECREFs will trigger this error when running perf
> script:
> 
>   Fatal Python error: Objects/tupleobject.c:238 object at
>   0x7f10f2041b40 has negative ref count -1
>   Aborted (core dumped)
> 
> If the reference debugging is not enabled, the superfluous DECREFs
> might cause the dict objects to be silently released while they are
> still in use. This may trigger various other assertions or just
> cause perf crashes and/or weird and unexpected data changes in the
> stored Python objects.
> 
> Signed-off-by: Janne Huttunen <janne.huttunen@nokia.com>

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

Thanks,
Namhyung

[tip:perf/urgent] perf script python: Fix dict reference counting

[tip-bot for Janne Huttunen]

Commit-ID:  db0ba84c04ef2cf293aaada5ae97531127844d9d
Gitweb:     https://git.kernel.org/tip/db0ba84c04ef2cf293aaada5ae97531127844d9d
Author:     Janne Huttunen <janne.huttunen@nokia.com>
AuthorDate: Mon, 9 Jul 2018 13:59:50 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 11 Jul 2018 09:45:24 -0400

perf script python: Fix dict reference counting

The dictionaries are attached to the parameter tuple that steals the
references and takes care of releasing them when appropriate.  The code
should not decrement the reference counts explicitly.  E.g. if libpython
has been built with reference debugging enabled, the superfluous DECREFs
will trigger this error when running perf script:

  Fatal Python error: Objects/tupleobject.c:238 object at
  0x7f10f2041b40 has negative ref count -1
  Aborted (core dumped)

If the reference debugging is not enabled, the superfluous DECREFs might
cause the dict objects to be silently released while they are still in
use. This may trigger various other assertions or just cause perf
crashes and/or weird and unexpected data changes in the stored Python
objects.

Signed-off-by: Janne Huttunen <janne.huttunen@nokia.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jaroslav Skarvada <jskarvad@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1531133990-17485-1-git-send-email-janne.huttunen@nokia.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/scripting-engines/trace-event-python.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 8b2eb6dbff4d..bc32e57d17be 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -908,14 +908,11 @@ static void python_process_tracepoint(struct perf_sample *sample,
 	if (_PyTuple_Resize(&t, n) == -1)
 		Py_FatalError("error resizing Python tuple");
 
-	if (!dict) {
+	if (!dict)
 		call_object(handler, t, handler_name);
-	} else {
+	else
 		call_object(handler, t, default_handler_name);
-		Py_DECREF(dict);
-	}
 
-	Py_XDECREF(all_entries_dict);
 	Py_DECREF(t);
 }
 
@@ -1235,7 +1232,6 @@ static void python_process_general_event(struct perf_sample *sample,
 
 	call_object(handler, t, handler_name);
 
-	Py_DECREF(dict);
 	Py_DECREF(t);
 }