[PATCH] Fix Python crash on getprop deallocation

[PATCH] Fix Python crash on getprop deallocation

[Luca Weiss]

Fatal Python error: none_dealloc: deallocating None
Python runtime state: finalizing (tstate=0x000055c9bac70920)

Current thread 0x00007fbe34e47740 (most recent call first):
  <no Python frame>
Aborted (core dumped)

This is caused by a missing Py_INCREF on the returned Py_None, as
demonstrated e.g. in https://github.com/mythosil/swig-python-incref or
described at https://edcjones.tripod.com/refcount.html ("Remember to
INCREF Py_None!")

A PoC for triggering this crash is uploaded to
https://github.com/z3ntu/pylibfdt-crash .
With this patch applied to pylibfdt the crash does not happen.

Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
---
Unrelated but I've noticed that in this file the indentation is quite
mixed between spaces and tabs. This patch tries to keep to the style in
the lines around.

 pylibfdt/libfdt.i | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
index 075ef70..9ccc57b 100644
--- a/pylibfdt/libfdt.i
+++ b/pylibfdt/libfdt.i
@@ -1040,14 +1040,16 @@ typedef uint32_t fdt32_t;
 
 /* typemap used for fdt_getprop() */
 %typemap(out) (const void *) {
-	if (!$1)
+	if (!$1) {
 		$result = Py_None;
-	else
+		Py_INCREF($result);
+	} else {
         %#if PY_VERSION_HEX >= 0x03000000
             $result = Py_BuildValue("y#", $1, (Py_ssize_t)*arg4);
         %#else
             $result = Py_BuildValue("s#", $1, (Py_ssize_t)*arg4);
         %#endif
+    }
 }
 
 /* typemap used for fdt_setprop() */
-- 
2.34.1

Re: [PATCH] Fix Python crash on getprop deallocation

[Simon Glass]

Hi Luca,

On Fri, 24 Dec 2021 at 03:38, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
>
> Fatal Python error: none_dealloc: deallocating None
> Python runtime state: finalizing (tstate=0x000055c9bac70920)
>
> Current thread 0x00007fbe34e47740 (most recent call first):
>   <no Python frame>
> Aborted (core dumped)
>
> This is caused by a missing Py_INCREF on the returned Py_None, as
> demonstrated e.g. in https://github.com/mythosil/swig-python-incref or
> described at https://edcjones.tripod.com/refcount.html ("Remember to
> INCREF Py_None!")
>
> A PoC for triggering this crash is uploaded to
> https://github.com/z3ntu/pylibfdt-crash .
> With this patch applied to pylibfdt the crash does not happen.
>
> Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> ---
> Unrelated but I've noticed that in this file the indentation is quite
> mixed between spaces and tabs. This patch tries to keep to the style in
> the lines around.
>
>  pylibfdt/libfdt.i | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

My original idea was to use tabs for the C code (to match the libfdt
style) and spaces for the Python code (for PEP8). Looking at it now,
that idea has not continued and I'm not even sure it was a good idea.

Regards,
Simon

Re: [PATCH] Fix Python crash on getprop deallocation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]

On Fri, Dec 24, 2021 at 06:17:25AM -0700, Simon Glass wrote:
> Hi Luca,
> 
> On Fri, 24 Dec 2021 at 03:38, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> >
> > Fatal Python error: none_dealloc: deallocating None
> > Python runtime state: finalizing (tstate=0x000055c9bac70920)
> >
> > Current thread 0x00007fbe34e47740 (most recent call first):
> >   <no Python frame>
> > Aborted (core dumped)
> >
> > This is caused by a missing Py_INCREF on the returned Py_None, as
> > demonstrated e.g. in https://github.com/mythosil/swig-python-incref or
> > described at https://edcjones.tripod.com/refcount.html ("Remember to
> > INCREF Py_None!")
> >
> > A PoC for triggering this crash is uploaded to
> > https://github.com/z3ntu/pylibfdt-crash .
> > With this patch applied to pylibfdt the crash does not happen.
> >
> > Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> > ---
> > Unrelated but I've noticed that in this file the indentation is quite
> > mixed between spaces and tabs. This patch tries to keep to the style in
> > the lines around.
> >
> >  pylibfdt/libfdt.i | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> My original idea was to use tabs for the C code (to match the libfdt
> style) and spaces for the Python code (for PEP8). Looking at it now,
> that idea has not continued and I'm not even sure it was a good idea.

Sounds like a good rationale, but probably not practical when combined
into a single file.  I'd suggest making it all spaces.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix Python crash on getprop deallocation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

On Fri, Dec 24, 2021 at 11:28:12AM +0100, Luca Weiss wrote:
> Fatal Python error: none_dealloc: deallocating None
> Python runtime state: finalizing (tstate=0x000055c9bac70920)
> 
> Current thread 0x00007fbe34e47740 (most recent call first):
>   <no Python frame>
> Aborted (core dumped)
> 
> This is caused by a missing Py_INCREF on the returned Py_None, as
> demonstrated e.g. in https://github.com/mythosil/swig-python-incref or
> described at https://edcjones.tripod.com/refcount.html ("Remember to
> INCREF Py_None!")
> 
> A PoC for triggering this crash is uploaded to
> https://github.com/z3ntu/pylibfdt-crash .
> With this patch applied to pylibfdt the crash does not happen.

Any chance you could rework your testcase into the libfdt testsuite
(make check)?

> Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> ---
> Unrelated but I've noticed that in this file the indentation is quite
> mixed between spaces and tabs. This patch tries to keep to the style in
> the lines around.
> 
>  pylibfdt/libfdt.i | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> index 075ef70..9ccc57b 100644
> --- a/pylibfdt/libfdt.i
> +++ b/pylibfdt/libfdt.i
> @@ -1040,14 +1040,16 @@ typedef uint32_t fdt32_t;
>  
>  /* typemap used for fdt_getprop() */
>  %typemap(out) (const void *) {
> -	if (!$1)
> +	if (!$1) {
>  		$result = Py_None;
> -	else
> +		Py_INCREF($result);
> +	} else {
>          %#if PY_VERSION_HEX >= 0x03000000
>              $result = Py_BuildValue("y#", $1, (Py_ssize_t)*arg4);
>          %#else
>              $result = Py_BuildValue("s#", $1, (Py_ssize_t)*arg4);
>          %#endif
> +    }
>  }
>  
>  /* typemap used for fdt_setprop() */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix Python crash on getprop deallocation

[Luca Weiss]

Hi David,

On Samstag, 25. Dezember 2021 07:29:58 CET David Gibson wrote:
> On Fri, Dec 24, 2021 at 11:28:12AM +0100, Luca Weiss wrote:
> > Fatal Python error: none_dealloc: deallocating None
> > Python runtime state: finalizing (tstate=0x000055c9bac70920)
> > 
> > Current thread 0x00007fbe34e47740 (most recent call first):
> >   <no Python frame>
> > 
> > Aborted (core dumped)
> > 
> > This is caused by a missing Py_INCREF on the returned Py_None, as
> > demonstrated e.g. in https://github.com/mythosil/swig-python-incref or
> > described at https://edcjones.tripod.com/refcount.html ("Remember to
> > INCREF Py_None!")
> > 
> > A PoC for triggering this crash is uploaded to
> > https://github.com/z3ntu/pylibfdt-crash .
> > With this patch applied to pylibfdt the crash does not happen.
> 
> Any chance you could rework your testcase into the libfdt testsuite
> (make check)?
> 

To be completely honest I don't exactly understand why this crash is 
happening. If you reduce the iteration count in my PoC from the "10" I used to 
just 1 or 2, then the crash doesn't happen. But I don't have any insights into 
how Python actually allocates and deallocates things internally, as this crash 
happens during dellocation when Python exits and after the supplied code is 
already run.

Regards
Luca

> > Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> > ---
> > Unrelated but I've noticed that in this file the indentation is quite
> > mixed between spaces and tabs. This patch tries to keep to the style in
> > the lines around.
> > 
> >  pylibfdt/libfdt.i | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > index 075ef70..9ccc57b 100644
> > --- a/pylibfdt/libfdt.i
> > +++ b/pylibfdt/libfdt.i
> > @@ -1040,14 +1040,16 @@ typedef uint32_t fdt32_t;
> > 
> >  /* typemap used for fdt_getprop() */
> >  %typemap(out) (const void *) {
> > 
> > -	if (!$1)
> > +	if (!$1) {
> > 
> >  		$result = Py_None;
> > 
> > -	else
> > +		Py_INCREF($result);
> > +	} else {
> > 
> >          %#if PY_VERSION_HEX >= 0x03000000
> >          
> >              $result = Py_BuildValue("y#", $1, (Py_ssize_t)*arg4);
> >          
> >          %#else
> >          
> >              $result = Py_BuildValue("s#", $1, (Py_ssize_t)*arg4);
> >          
> >          %#endif
> > 
> > +    }
> > 
> >  }
> >  
> >  /* typemap used for fdt_setprop() */

Re: [PATCH] Fix Python crash on getprop deallocation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]

On Sat, Dec 25, 2021 at 11:25:51AM +0100, Luca Weiss wrote:
> Hi David,
> 
> On Samstag, 25. Dezember 2021 07:29:58 CET David Gibson wrote:
> > On Fri, Dec 24, 2021 at 11:28:12AM +0100, Luca Weiss wrote:
> > > Fatal Python error: none_dealloc: deallocating None
> > > Python runtime state: finalizing (tstate=0x000055c9bac70920)
> > > 
> > > Current thread 0x00007fbe34e47740 (most recent call first):
> > >   <no Python frame>
> > > 
> > > Aborted (core dumped)
> > > 
> > > This is caused by a missing Py_INCREF on the returned Py_None, as
> > > demonstrated e.g. in https://github.com/mythosil/swig-python-incref or
> > > described at https://edcjones.tripod.com/refcount.html ("Remember to
> > > INCREF Py_None!")
> > > 
> > > A PoC for triggering this crash is uploaded to
> > > https://github.com/z3ntu/pylibfdt-crash .
> > > With this patch applied to pylibfdt the crash does not happen.
> > 
> > Any chance you could rework your testcase into the libfdt testsuite
> > (make check)?
> > 
> 
> To be completely honest I don't exactly understand why this crash is 
> happening. If you reduce the iteration count in my PoC from the "10" I used to 
> just 1 or 2, then the crash doesn't happen. But I don't have any insights into 
> how Python actually allocates and deallocates things internally, as this crash 
> happens during dellocation when Python exits and after the supplied code is 
> already run.

Ok, fair enough.  Applied to main branch.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]