bpo-44184: Fix subtype_dealloc() for freed type by vstinner · Pull Request #26274 · python/cpython

vstinner · 2021-05-20T23:46:16Z

Fix a crash at Python exit when a deallocator function removes the
last strong reference to a heap type.

Don't read type memory after calling basedealloc() since
basedealloc() can deallocate the type and free its memory.

https://bugs.python.org/issue44184

vstinner · 2021-05-20T23:46:35Z

pablogsal

This fix makes no sense to me, if what you say in the comment was actually the root cause, we would be seeing this under vallgrind or the address sanitizer and I could not reproduce it

bedevere-bot · 2021-05-21T00:06:32Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

I was able to reproduce the crash under ASAN: https://bugs.python.org/msg394085

pablogsal · 2021-05-21T00:19:06Z

This is enough to crash it:

import ast
import builtins
import sys
import os

tree = ast.parse("pass")
x = [tree]
x.append(x)

# Put the cycle somewhere to survive until the *last* GC collection
def callback(*args):
    pass
callback.a = x
os.register_at_fork(after_in_child=callback)

can we maybe add a test based on this?

vstinner · 2021-05-21T00:27:54Z

I would prefer to not rely directly on os.register_at_fork() to ensure that the object will survive until the last GC collection. I would prefer an abstraction function in the test.support module. I will try to add a test.

erlend-aasland · 2021-05-21T07:43:50Z

I can reproduce the bug on macOS using the address sanitiser. The patch also fixes the buildbot problems on #24203 (comment)

Fix a crash at Python exit when a deallocator function removes the last strong reference to a heap type. Don't read type memory after calling basedealloc() since basedealloc() can deallocate the type and free its memory.

vstinner · 2021-05-21T16:17:08Z

@pablogsal: I added an unit test, would you mind to review the updated PR?

I was too lazy to add an abstraction. Since the test is in test_gc, IMO it's ok. First, I wanted to add the test to test_ast.

vstinner · 2021-05-21T16:36:21Z

Ah, register_at_fork() is not available on Windows. I modified my test to use codecs.register() instead.

You can test manually that the unit test fails as expected with this change:

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index af99ab79d1..acbedc64e6 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -5,6 +5,7 @@
 #include "pycore_compile.h"       // _Py_Mangle()
 #include "pycore_initconfig.h"
 #include "pycore_moduleobject.h"  // _PyModule_GetDef()
+#include "pycore_pymem.h"  // _PyModule_GetDef()
 #include "pycore_object.h"
 #include "pycore_pyerrors.h"
 #include "pycore_pystate.h"       // _PyThreadState_GET()
@@ -1459,6 +1460,7 @@ subtype_dealloc(PyObject *self)
        our type from a HEAPTYPE to a non-HEAPTYPE, so be careful about
        reference counting. Only decref if the base type is not already a heap
        allocated type. Otherwise, basedealloc should have decref'd it already */
+    assert(!_PyMem_IsPtrFreed(type->tp_name));
     if (type_needs_decref) {
         Py_DECREF(type);
     }

erlend-aasland

Looks good to me!

miss-islington · 2021-05-21T17:20:05Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

bedevere-bot · 2021-05-21T17:20:14Z

GH-26290 is a backport of this pull request to the 3.10 branch.

Fix a crash at Python exit when a deallocator function removes the last strong reference to a heap type. Don't read type memory after calling basedealloc() since basedealloc() can deallocate the type and free its memory. _PyMem_IsPtrFreed() argument is now constant. (cherry picked from commit 615069e) Co-authored-by: Victor Stinner <[email protected]>

vstinner · 2021-05-21T17:20:19Z

Thanks for reviews @pablogsal and @erlend-aasland!

Fix a crash at Python exit when a deallocator function removes the last strong reference to a heap type. Don't read type memory after calling basedealloc() since basedealloc() can deallocate the type and free its memory. _PyMem_IsPtrFreed() argument is now constant. (cherry picked from commit 615069e) Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Victor Stinner <[email protected]>

… it is also trying to use a type after it's potentially been freed.

GH-27165) The non-GC-type branch of subtype_dealloc is using the type of an object after freeing in the same unsafe way as GH-26274 fixes. (I believe the old news entry covers this change well enough.) https://bugs.python.org/issue44184

…dealloc (pythonGH-27165) The non-GC-type branch of subtype_dealloc is using the type of an object after freeing in the same unsafe way as pythonGH-26274 fixes. (I believe the old news entry covers this change well enough.) https://bugs.python.org/issue44184 (cherry picked from commit 074e765) Co-authored-by: T. Wouters <[email protected]>

miss-islington · 2021-07-15T22:42:13Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

Fix a crash at Python exit when a deallocator function removes the last strong reference to a heap type. Don't read type memory after calling basedealloc() since basedealloc() can deallocate the type and free its memory. _PyMem_IsPtrFreed() argument is now constant. (cherry picked from commit 615069e) Co-authored-by: Victor Stinner <[email protected]>

bedevere-bot · 2021-07-15T22:42:21Z

GH-27176 is a backport of this pull request to the 3.9 branch.

GH-27165) (GH-27174) The non-GC-type branch of subtype_dealloc is using the type of an object after freeing in the same unsafe way as GH-26274 fixes. (I believe the old news entry covers this change well enough.) https://bugs.python.org/issue44184 (cherry picked from commit 074e765) Co-authored-by: T. Wouters <[email protected]>

GH-27165) (GH-27175) The non-GC-type branch of subtype_dealloc is using the type of an object after freeing in the same unsafe way as GH-26274 fixes. (I believe the old news entry covers this change well enough.) https://bugs.python.org/issue44184 (cherry picked from commit 074e765) Co-authored-by: T. Wouters <[email protected]>

Fix a crash at Python exit when a deallocator function removes the last strong reference to a heap type. Don't read type memory after calling basedealloc() since basedealloc() can deallocate the type and free its memory. _PyMem_IsPtrFreed() argument is now constant. (cherry picked from commit 615069e) Co-authored-by: Victor Stinner <[email protected]>

encukou · 2021-10-21T18:09:21Z

For posterity: bpo-42961 reports the same issue and has a nice reproducer.

vstinner requested a review from markshannon as a code owner May 20, 2021 23:46

the-knights-who-say-ni added the CLA signed label May 20, 2021

bedevere-bot added the awaiting core review label May 20, 2021

erlend-aasland approved these changes May 20, 2021

View reviewed changes

pablogsal previously requested changes May 21, 2021

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels May 21, 2021

erlend-aasland mentioned this pull request May 21, 2021

bpo-42862: Use functools.lru_cache iso. _sqlite.Cache in sqlite3 module #24203

Merged

vstinner added 3 commits May 21, 2021 18:03

bpo-44184: Fix subtype_dealloc() for freed type

5bcd788

Fix a crash at Python exit when a deallocator function removes the last strong reference to a heap type. Don't read type memory after calling basedealloc() since basedealloc() can deallocate the type and free its memory.

Add an unit test

454e996

_PyMem_IsPtrFreed() argument is now const

b670383

Port the unit test to Windows

efd45ad

erlend-aasland approved these changes May 21, 2021

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting changes labels May 21, 2021

vstinner merged commit 615069e into python:main May 21, 2021

bedevere-bot removed the awaiting core review label May 21, 2021

vstinner deleted the subtype_dealloc branch May 21, 2021 17:19

vstinner added the needs backport to 3.10 only security fixes label May 21, 2021

bedevere-bot removed the needs backport to 3.10 only security fixes label May 21, 2021

erlend-aasland mentioned this pull request May 21, 2021

bpo-44032: Defer clearing last thread state until last GC has been run. #26285

Merged

Yhg1s added a commit to Yhg1s/cpython that referenced this pull request Jul 15, 2021

Apply pythonGH-26274 to the non-GC-type branch of subtype_dealloc, as…

f098930

… it is also trying to use a type after it's potentially been freed.

Yhg1s mentioned this pull request Jul 15, 2021

bpo-44184: Apply GH-26274 to the non-GC-type branch of subtype_dealloc. #27165

Merged

miss-islington mentioned this pull request Jul 15, 2021

[3.10] bpo-44184: Apply GH-26274 to the non-GC-type branch of subtype_dealloc (GH-27165) #27174

Merged

miss-islington mentioned this pull request Jul 15, 2021

[3.9] bpo-44184: Apply GH-26274 to the non-GC-type branch of subtype_dealloc (GH-27165) #27175

Merged

Yhg1s added the needs backport to 3.9 label Jul 15, 2021

bedevere-bot removed the needs backport to 3.9 label Jul 15, 2021

pablogsal mentioned this pull request Jul 18, 2021

[3.10] Correct the order of regen-abidump #27228

Closed

Uh oh!

Uh oh!

Conversation

vstinner commented May 20, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented May 20, 2021

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 21, 2021

Uh oh!

pablogsal commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented May 21, 2021

Uh oh!

erlend-aasland commented May 21, 2021

Uh oh!

vstinner commented May 21, 2021

Uh oh!

vstinner commented May 21, 2021

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented May 21, 2021

Uh oh!

bedevere-bot commented May 21, 2021

Uh oh!

vstinner commented May 21, 2021

Uh oh!

miss-islington commented Jul 15, 2021

Uh oh!

bedevere-bot commented Jul 15, 2021

Uh oh!

encukou commented Oct 21, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vstinner commented May 20, 2021 •

edited by bedevere-bot

Loading

pablogsal commented May 21, 2021 •

edited

Loading

encukou commented Oct 21, 2021 •

edited by bedevere-bot

Loading