bpo-35381 Make all posixmodule types heap-allocated by eduardo-elizondo · Pull Request #10854 · python/cpython · GitHub

After #9665, this moves the remaining types in posixmodule to be heap-allocated to make it compatible with PEP384

https://bugs.python.org/issue35381

LGTM

I'm not super familiar with the heap type machinery, but this lines up with other examples I've seen. Thanks for updating the argument clinic stuff.

os.DirEntry() will crash. Prevent the crash and add a test. There should be similar issue with ScandirIteratorType.

See bpo-26979 and related issues for details.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again

cc @vstinner @ericsnowcurrently @serhiy-storchaka

Hi Serihy. Thanks for pointing out the bug, I've updated the change with the appropriate fix. 
I was able to come up with a generic solution which can solve many of the bugs that have come up from converting a PyType_Ready to a PyType_FromSpec. I used that to solve the specific case that is presented in posixmodule.c

Problem

Most of the issues in converting a PyType_Ready to a PyType_FromSpec come from implicit assumptions about how these types should behave. This results in missing signals which cause incorrect behavior. For example, uninstantiable types becoming instantiable, the runtime crashing, or refcnt leaks. To understand these issues I have broken down the scenarios that might arise:

tp_new is defined and tp_dealloc is defined.

Result: Refcnt Leak or Crash

Instances are most likely instantiated in Python-land and it uses PyType_GenericAlloc to allocate the instance, increfing the type. However, under the assumption that the type is static, it is likely that tp_dealloc is not decrefing the type. This results in a net refcnt of +1 per object instantiation.
Another sub-scenario is when tp_dealloc actually decrefs the type. This results in correct behavior.
However, if the user creates the instance through PyObject_New{Var} it won't incref its type. This results in a net refcnt of -1 per object instantiation, causing a a segfault.

tp_new is defined and tp_dealloc is undefined.

Result: Correct Behavior

Instances are most likely instantiated in Python-land and it uses PyType_GenericAlloc to allocate the instance, increfing the type. tp_dealloc is undefined and will inherit subtype_dealloc which will decref the type. This results in correct behavior.
However, if the user creates the instance through PyObject_New{Var} it won't incref its type. This results in a net refcnt of -1 per object instantiation, causing a a segfault.

tp_new is undefined and tp_dealloc is defined. Result:

Incorrect Behavior

This is an uninstantiable type from Python-land. Therefore, instantiation comes directly from PyObject_New{Var}, which won't incref the type. Therefore, the defined tp_dealloc won't decref the type either. However, the side effect is that the signal produced by tp_new being NULL will be lost given that the type will inherit its base object's tp_new. This will now make the type both instantiable and pickable in Python-land.

tp_new is undefined and tp_dealloc is unedefined.

Result: Crash

This is an uninstantiable type from Python-land. Therefore, instantiation comes directly from PyObject_New{Var}, which won't incref the type. However, the undefined tp_dealloc will inherit subtype_dealloc which will decref the type. This results in a net refcnt of -1 per object instantiation, causing a a segfault.

Solution

In order to solve these issues, we need to think of these types as participating in normal refcnting just like any other instance.

The following provides a generic solution for all these different scenarios:

Make PyObject_INIT incref heap-allocated instance's type.
Have the custom tp_dealloc decref the instance's type
Explicitly define a tp_new, __reduce__, and __reduce_ex__ that raise an exception and return NULL to make it uninstantiable and unpickable respectively.

Closing Bugs

The generic solution proposed here should be able to address all the issues and problems in the following bpos:

https://bugs.python.org/issue14936
https://bugs.python.org/issue15142
https://bugs.python.org/issue15721
https://bugs.python.org/issue16690
https://bugs.python.org/issue23815
https://bugs.python.org/issue26979

Thanks for making the requested changes!

@ericsnowcurrently, @serhiy-storchaka: please review the changes made to this pull request.

I am not sure about this part. It will affect all other heap types. It is better to defer it to a separate issue.

This is really needed for this change. PyTypeObjects should participate in reference counting. Without this change, instantiating a DirEntry or a ScandirIterator will not increase the type's refcount - which is incorrect (since they are now heap-allocated)

This ensures the correct behavior to the newly heap-allocated type. I added it in this place to ensure that any future migration from PyType_Ready to PyType_FromSpec won't have to worry about this anymore.

To expand on this a bit more, this currently DOES NOT affect any other heap types but the ones changed here (with 1 exception).

There are only 3 ways to access PyObject_{Init,INIT} which are:

Through PyType_GenericAlloc
Through PyObject_New (corollary: PyObject_NewVar, PyObject_GC_New, PyObject_GC_NewVar)
Direct calls which don't come from the previous two. (corollary: Direct calls to PyObject_Init, PyObject_INIT_VAR)

For each of these points:

All current calls to PyType_GenericAlloc are already checking if type type is a heap type. This just moves the check from PyType_GenericAlloc, to PyObject_Init so it preserves behavior.
All current calls to PyObject_New and PyObject_NewVar use static types. With 1 exception, mentioned below.
All direct calls use static types.

Outside of the exception there should not be any problems with any other heap type.

Exception:
_tkinter.c. The incref was manually added after PyObject_New.

I have the following 3 proposals:

Leave the current change. The _tkinter types will temporary leak a refcount per instance allocation. Not that big of a deal since it's just 3 different types. Then, add a new PR to fix _tkinter.c.
Modify _tkinter.c in this change to do the correct thing.
Remove the Py_INCREF in PyObject_Init in this change and add it right after PyObject_New in posixmodule.c. Then create a new PR with this change to Py_INCREF in PyObject_Init and fix both posixmodule.c and _tkinter.c

Thanks for the review @serhiy-storchaka ! I answered each of the points that you raised and marked the comments as resolved. Please feel free to raise it again if you still feel that I did not fully address either of your comments.

I have made the requested changes; please review again

Thanks for making the requested changes!

@serhiy-storchaka, @ericsnowcurrently: please review the changes made to this pull request.

I don't understand why "convert statically allocated types (DirEntryType & ScandirIteratorType) to heap-allocated types" needs to modify the very important _PyObject_INIT() function. If you want to modify it, would it be possible to do that in a first PR which only modify _PyObject_INIT() (with related changes)?

Yeah, that makes sense! I just created:
https://bugs.python.org/issue35810
#11661

I will rebase this change once that PR is merged.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

I understood in PR #11661 that this change is backward incompatible and so is a bad idea.

…ython#14568) * bpo-37459: importlib docs improperly reference get_resource_loader()

Fix multiprocessing.util.get_temp_dir() finalizer: clear also the 'tempdir' configuration of the current process, so next call to get_temp_dir() will create a new temporary directory, rather than reusing the removed temporary directory.

To get more info about this change.

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Woops, this change got messed up in a rebase

After #9665, this moves the remaining types in posixmodule to be heap-allocated to make it compatible with PEP384 as well as modifying all the type accessors to fully make the type opaque. The original PR that got messed up a rebase: #10854. All the issues in that commit have now been addressed since #11661 got committed. This change also removes any state from the data segment and onto the module state itself. https://bugs.python.org/issue35381 Automerge-Triggered-By: @encukou

After python#9665, this moves the remaining types in posixmodule to be heap-allocated to make it compatible with PEP384 as well as modifying all the type accessors to fully make the type opaque. The original PR that got messed up a rebase: python#10854. All the issues in that commit have now been addressed since python#11661 got committed. This change also removes any state from the data segment and onto the module state itself. https://bugs.python.org/issue35381 Automerge-Triggered-By: @encukou

the-knights-who-say-ni added the CLA signed label Dec 3, 2018

bedevere-bot added the awaiting review label Dec 3, 2018

ericsnowcurrently approved these changes Jan 4, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jan 4, 2019

vstinner reviewed Jan 7, 2019

View reviewed changes

Comment thread Misc/NEWS.d/next/C API/2018-12-02-22-39-41.bpo-35381.HjI3j_.rst Outdated

serhiy-storchaka requested changes Jan 8, 2019

View reviewed changes

Comment thread Misc/NEWS.d/next/C API/2018-12-02-22-39-41.bpo-35381.HjI3j_.rst Outdated

bedevere-bot removed the awaiting merge label Jan 8, 2019

bedevere-bot added the awaiting changes label Jan 8, 2019

eduardo-elizondo force-pushed the posixmodule-fromspec branch from 6d01e5f to 9479848 Compare January 19, 2019 00:46

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 19, 2019

serhiy-storchaka reviewed Jan 19, 2019

View reviewed changes

vstinner requested changes Jan 23, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jan 23, 2019

eduardo-elizondo mentioned this pull request Jan 23, 2019

bpo-35810: Incref heap-allocated types in PyObject_Init #11661

Merged

vstinner requested changes Jan 24, 2019

View reviewed changes

aldwinaldwin and others added 5 commits July 3, 2019 17:58

bpo-37459: importlib docs improperly reference get_resource_loader() (p…

b607d99

…ython#14568) * bpo-37459: importlib docs improperly reference get_resource_loader()

bpo-37483: add _PyObject_CallOneArg() function (python#14558 )

196a530

bpo-37493: use _PyObject_CallNoArg in more places (pythonGH-14575 )

7f41c8e

bpo-37266: Add bpo number to the What's New entry (GH614584)

b4e6896

To get more info about this change.

eduardo-elizondo requested review from a team, berkerpeksag, brettcannon, encukou, ericvsmith, gpshead, gvanrossum, ilevkivskyi, methane, ncoghlan, pablogsal, pganssle, rhettinger, skrah, terryjreedy, tiran and warsaw as code owners September 11, 2019 04:57

the-knights-who-say-ni added CLA not signed and removed CLA signed labels Sep 11, 2019

eduardo-elizondo closed this Sep 11, 2019

eduardo-elizondo mentioned this pull request Sep 11, 2019

bpo-35381 Remove all static state from posixmodule #15892

Merged

vstinner mentioned this pull request Jan 20, 2020

bpo-39395: Clear env vars set by Python at exit #18078

Closed

                       return PyErr_NoMemory();
                   /* Any changes should be reflected in PyObject_INIT (objimpl.h) */
                   Py_TYPE(op) = tp;
+                  if (PyType_GetFlags(tp) & Py_TPFLAGS_HEAPTYPE) {

                   assert(op != NULL);
                   Py_TYPE(op) = typeobj;
+                  if (PyType_GetFlags(typeobj) & Py_TPFLAGS_HEAPTYPE) {
+                      Py_INCREF(typeobj);

bpo-35381 Make all posixmodule types heap-allocated by eduardo-elizondo · Pull Request #10854 · python/cpython · GitHub

Repository navigation

bpo-35381 Make all posixmodule types heap-allocated - #10854

bpo-35381 Make all posixmodule types heap-allocated#10854eduardo-elizondo wants to merge 1733 commits intopython:masterpython/cpython:masterfrom eduardo-elizondo:posixmodule-fromspeceduardo-elizondo/cpython:posixmodule-fromspecCopy head branch name to clipboard

Conversation

eduardo-elizondo commented Dec 3, 2018 • edited by asvetlov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Jan 8, 2019

Uh oh!

eduardo-elizondo commented Jan 19, 2019

Problem

tp_new is defined and tp_dealloc is defined.

tp_new is defined and tp_dealloc is undefined.

tp_new is undefined and tp_dealloc is defined. Result:

tp_new is undefined and tp_dealloc is unedefined.

Solution

Closing Bugs

Uh oh!

bedevere-bot commented Jan 19, 2019

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eduardo-elizondo commented Jan 22, 2019

Uh oh!

bedevere-bot commented Jan 22, 2019

Uh oh!

vstinner Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 23, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

the-knights-who-say-ni commented Sep 11, 2019

Uh oh!

eduardo-elizondo commented Sep 11, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

19 participants

bpo-35381 Make all posixmodule types heap-allocated#10854
eduardo-elizondo wants to merge 1733 commits into
python:masterfrom
eduardo-elizondo:posixmodule-fromspec

eduardo-elizondo commented Dec 3, 2018 •
edited by asvetlov
Loading

eduardo-elizondo Jan 20, 2019 •
edited
Loading

eduardo-elizondo Jan 22, 2019 •
edited
Loading

eduardo-elizondo commented Jan 20, 2019 •
edited
Loading