gh-104614: Make Sure ob_type is Always Set Correctly by PyType_Ready() by ericsnowcurrently · Pull Request #105122 · python/cpython

ericsnowcurrently · 2023-05-30T23:01:54Z

When I added the relevant condition to type_ready_set_bases() in gh-103912, I had missed that the function also sets tp_base and ob_type (if necessary). That led to problems for third-party static types.

We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific.

Issue: Type object's ob_type does not get set when tp_bases is set before PyType_Ready #104614

lysnikolaou

Thanks @ericsnowcurrently! I can verify that this solves numpy/numpy#23766.

erlend-aasland · 2023-05-31T09:25:54Z

Is it possible to use a reduced variant of the numpy case as a regression test?

lysnikolaou · 2023-05-31T10:48:40Z

Without knowing too much around the issue, I was able to come up with this, which is really close to what numpy does (not sure how we would go about adding a test for it).

#definePY_SSIZE_T_CLEAN#include"Python.h"staticPyTypeObjectCustomBaseType1= {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name="custom.Base1",
.tp_basicsize=sizeof(PyObject),
};
staticPyTypeObjectCustomBaseType2= {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name="custom.Base2",
.tp_basicsize=sizeof(PyObject),
};
staticPyTypeObjectCustomDerivedType= {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name="custom.Derived",
.tp_basicsize=sizeof(PyObject),
};
staticPyModuleDefcustommodule= {
PyModuleDef_HEAD_INIT,
.m_name="custom",
.m_doc="Example module that creates an extension type.",
.m_size=-1,
};
PyMODINIT_FUNCPyInit_custom(void)
{
PyObject*m;
if (PyType_Ready(&CustomBaseType1) <0)
returnNULL;
if (PyType_Ready(&CustomBaseType2) <0)
returnNULL;
CustomDerivedType.tp_base=&CustomBaseType2;
CustomDerivedType.tp_bases=Py_BuildValue("(OO)", &CustomBaseType2, &CustomBaseType1);
CustomDerivedType.tp_hash=CustomBaseType1.tp_hash;
if (PyType_Ready(&CustomDerivedType) <0)
returnNULL;
m=PyModule_Create(&custommodule);
if (m==NULL)
returnNULL;
Py_INCREF(&CustomDerivedType);
if (PyModule_AddObject(m, "Custom", (PyObject*) &CustomDerivedType) <0) {
Py_DECREF(&CustomDerivedType);
Py_DECREF(m);
returnNULL;
}
returnm;
}

It segfaults on import on main, and is okay with this PR.

markshannon · 2023-05-31T15:37:45Z

Would it make sense to allocate the tuples with malloc at start up, and never free them?

There would be no need to free them at the end of the interpreters lifetime, and there would be no issue if an interpreter other than the main interpreter outlives the main interpreter.

ericsnowcurrently · 2023-05-31T21:24:32Z

Would it make sense to allocate the tuples with malloc at start up, and never free them?

Yeah. However, that's orthogonal to this PR.

ericsnowcurrently · 2023-05-31T23:12:49Z

@lysnikolaou, your example helped. I've added a regression test.

lysnikolaou · 2023-06-01T09:52:35Z

test_capi seems to be failing the first time, but it succeeds when re-run. Not sure what's wrong.

miss-islington · 2023-06-01T22:35:17Z

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

…Ready() (pythongh-105122) When I added the relevant condition to type_ready_set_bases() in pythongh-103912, I had missed that the function also sets tp_base and ob_type (if necessary). That led to problems for third-party static types. We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific. (cherry picked from commit 1469393) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

bedevere-bot · 2023-06-01T22:35:31Z

GH-105211 is a backport of this pull request to the 3.12 branch.

…_Ready() (gh-105122) (gh-105211) When I added the relevant condition to type_ready_set_bases() in gh-103912, I had missed that the function also sets tp_base and ob_type (if necessary). That led to problems for third-party static types. We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific. (cherry picked from commit 1469393) Co-authored-by: Eric Snow ericsnowcurrently@gmail.com

erlend-aasland · 2023-06-02T04:49:21Z

+ cls->tp_base = (PyTypeObject *)Py_NewRef(base);
+ cls->tp_bases = Py_BuildValue("(O)", base);
+ if (cls->tp_bases == NULL) {
+ return NULL;


Should decref tp_base and tp_bases before returning.

Opened gh-105225.

Thanks for following up after-the-fact!

erlend-aasland · 2023-06-02T08:46:45Z

+ @classmethod
+ def setUpClass(cls):
+ # The tests here don't play nice with our approach to refleak
+ # detection, so we bail out in that case.
+ if cls._has_run:
+ raise unittest.SkipTest('these tests do not support re-running')
+ cls._has_run = True


This is nice. IMO, we should use a similar approach in test_import instead of the hack suggested in gh-104796.

Perhaps also gh-105085 should have been solved like this.

I'm glad you appreciate the solution 😄, but actually am a bit torn about this. On the one hand, it gets us what we want in a simple, clear way (IMHO). On the other hand, this approach means there may be code paths that don't get checked for ref leaks, which we'd want to avoid.
If we use this pattern elsewhere, we'll need to be vigilant about making sure we don't miss any ref leak coverage. I'm not sure how easy that would be generally, though it's unlikely that there are more than a handful of tests where we'd want to skip refleak detection. FWIW, in this case I don't think ref leaks are much of a concern.

we should use a similar approach
Wouldn't this skip the whole class Test instead of a smaller def test_...?

Yes, so you make sure to collect all your "ref leak unsafe" tests in a single unittest class.

Alternatively, create a decorator for it.

ericsnowcurrently added the skip news label May 30, 2023

ericsnowcurrently requested review from Yhg1s and lysnikolaou May 30, 2023 23:01

bedevere-bot mentioned this pull request May 30, 2023
Type object's ob_type does not get set when tp_bases is set before PyType_Ready #104614
Closed

ericsnowcurrently added 2 commits May 30, 2023 18:17

Split up type_ready_set_bases().
fe12a90

Fix the short-circuit in type_ready_set_bases().
86f8ec6

ericsnowcurrently force-pushed the fix-ob-type-before-ready branch from 116959a to 86f8ec6Compare May 31, 2023 00:19

ericsnowcurrently marked this pull request as ready for review May 31, 2023 00:20

ericsnowcurrently requested a review from markshannon as a code owner May 31, 2023 00:20

bedevere-bot added the awaiting core review label May 31, 2023

ericsnowcurrently removed the request for review from markshannon May 31, 2023 00:20

Fix the empty tuple case for clear_tp_bases().
e4337e2

lysnikolaou approved these changes May 31, 2023
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels May 31, 2023

Add a regression test.
b680117

Ignore the test static type.
08a0321

Fix the test.
80d20c6

ericsnowcurrently force-pushed the fix-ob-type-before-ready branch from 99648fe to 80d20c6Compare June 1, 2023 21:57

ericsnowcurrently enabled auto-merge (squash) June 1, 2023 22:25

ericsnowcurrently merged commit 1469393 into python:mainJun 1, 2023

bedevere-bot removed the awaiting merge label Jun 1, 2023

ericsnowcurrently deleted the fix-ob-type-before-ready branch June 1, 2023 22:34

ericsnowcurrently added the needs backport to 3.12 only security fixes label Jun 1, 2023

bedevere-bot removed the needs backport to 3.12 only security fixes label Jun 1, 2023

erlend-aasland reviewed Jun 2, 2023
View reviewed changes

erlend-aasland mentioned this pull request Jun 5, 2023
gh-102251: add missing cleanups for test_import#104796
Closed

sunmy2019 mentioned this pull request Jun 5, 2023
Allow test cases to skip rerun #105310
Closed

erlend-aasland mentioned this pull request Jun 6, 2023
3.12 backport gh 105236 #105358
Closed

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

lysnikolaou left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented May 31, 2023

Uh oh!

lysnikolaou commented May 31, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon commented May 31, 2023

Uh oh!

ericsnowcurrently commented May 31, 2023

Uh oh!

ericsnowcurrently commented May 31, 2023

Uh oh!

lysnikolaou commented Jun 1, 2023

Uh oh!

miss-islington commented Jun 1, 2023

Uh oh!

bedevere-bot commented Jun 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ericsnowcurrently commented May 30, 2023•
edited by bedevere-bot
Loading

lysnikolaou commented May 31, 2023•
edited
Loading