Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-104614: Make Sure ob_type is Always Set Correctly by PyType_Ready()#105122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-104614: Make Sure ob_type is Always Set Correctly by PyType_Ready()#105122
Uh oh!
There was an error while loading.Please reload this page.
Conversation
116959a
to86f8ec6
CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks@ericsnowcurrently! I can verify that this solvesnumpy/numpy#23766.
Is it possible to use a reduced variant of the numpy case as a regression test? |
lysnikolaou commentedMay 31, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
Would it make sense to allocate the tuples with 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. |
Yeah. However, that's orthogonal to this PR. |
@lysnikolaou, your example helped. I've added a regression test. |
|
99648fe
to80d20c6
CompareThanks@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() inpythongh-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 commit1469393)Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
bedevere-bot commentedJun 1, 2023
GH-105211 is a backport of this pull request to the3.12 branch. |
…_Ready() (gh-105122) (gh-105211)When I added the relevant condition to type_ready_set_bases() ingh-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 commit1469393)Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
cls->tp_base = (PyTypeObject *)Py_NewRef(base); | ||
cls->tp_bases = Py_BuildValue("(O)", base); | ||
if (cls->tp_bases == NULL) { | ||
return NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should decreftp_base
andtp_bases
before returning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Openedgh-105225.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for following up after-the-fact!
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is nice. IMO, we should use a similar approach intest_import
instead of the hack suggested ingh-104796.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perhaps alsogh-105085 should have been solved like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 theremay 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we should use a similar approach
Wouldn't this skip the wholeclass Test
instead of a smallerdef test_...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, so you make sure to collect all your "ref leak unsafe" tests in a single unittest class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alternatively, create a decorator for it.
Uh oh!
There was an error while loading.Please reload this page.
When I added the relevant condition to
type_ready_set_bases()
ingh-103912, I had missed that the function also setstp_base
andob_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.