gh-104614: Make Sure ob_type is Always Set Correctly by PyType_Ready()#105122
Conversation
116959a to
86f8ec6
Compare
May 31, 2023 00:19
lysnikolaou
left a comment
There was a problem hiding this comment.
Thanks @ericsnowcurrently! I can verify that this solves numpy/numpy#23766.
Sorry, something went wrong.
|
Is it possible to use a reduced variant of the numpy case as a regression test? |
Sorry, something went wrong.
|
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). #define PY_SSIZE_T_CLEAN
#include "Python.h"
static PyTypeObject CustomBaseType1 = {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name = "custom.Base1",
.tp_basicsize = sizeof(PyObject),
};
static PyTypeObject CustomBaseType2 = {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name = "custom.Base2",
.tp_basicsize = sizeof(PyObject),
};
static PyTypeObject CustomDerivedType = {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name = "custom.Derived",
.tp_basicsize = sizeof(PyObject),
};
static PyModuleDef custommodule = {
PyModuleDef_HEAD_INIT,
.m_name = "custom",
.m_doc = "Example module that creates an extension type.",
.m_size = -1,
};
PyMODINIT_FUNC
PyInit_custom(void)
{
PyObject *m;
if (PyType_Ready(&CustomBaseType1) < 0)
return NULL;
if (PyType_Ready(&CustomBaseType2) < 0)
return NULL;
CustomDerivedType.tp_base = &CustomBaseType2;
CustomDerivedType.tp_bases =
Py_BuildValue("(OO)", &CustomBaseType2, &CustomBaseType1);
CustomDerivedType.tp_hash = CustomBaseType1.tp_hash;
if (PyType_Ready(&CustomDerivedType) < 0)
return NULL;
m = PyModule_Create(&custommodule);
if (m == NULL)
return NULL;
Py_INCREF(&CustomDerivedType);
if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomDerivedType) < 0) {
Py_DECREF(&CustomDerivedType);
Py_DECREF(m);
return NULL;
}
return m;
}It segfaults on import on main, and is okay with this PR. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Yeah. However, that's orthogonal to this PR. |
Sorry, something went wrong.
|
@lysnikolaou, your example helped. I've added a regression test. |
Sorry, something went wrong.
|
|
Sorry, something went wrong.
99648fe to
80d20c6
Compare
June 1, 2023 21:57
|
Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
Sorry, something went wrong.
…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>
…_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
When I added the relevant condition to
type_ready_set_bases()in gh-103912, I had missed that the function also setstp_baseandob_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.