◐ Shell
reader mode source ↗
Skip to content

gh-104614: Make Sure ob_type is Always Set Correctly by PyType_Ready()#105122

Merged
ericsnowcurrently merged 6 commits into
python:mainfrom
ericsnowcurrently:fix-ob-type-before-ready
Jun 1, 2023
Merged

gh-104614: Make Sure ob_type is Always Set Correctly by PyType_Ready()#105122
ericsnowcurrently merged 6 commits into
python:mainfrom
ericsnowcurrently:fix-ob-type-before-ready

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented May 30, 2023

Copy link
Copy Markdown
Member

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.

@ericsnowcurrently ericsnowcurrently force-pushed the fix-ob-type-before-ready branch from 116959a to 86f8ec6 Compare May 31, 2023 00:19
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review May 31, 2023 00:20
@ericsnowcurrently ericsnowcurrently removed the request for review from markshannon May 31, 2023 00:20

@lysnikolaou lysnikolaou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

@erlend-aasland

Copy link
Copy Markdown
Contributor

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

@lysnikolaou

lysnikolaou commented May 31, 2023

Copy link
Copy Markdown
Member

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.

@markshannon

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

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

@lysnikolaou

Copy link
Copy Markdown
Member

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

@ericsnowcurrently ericsnowcurrently force-pushed the fix-ob-type-before-ready branch from 99648fe to 80d20c6 Compare June 1, 2023 21:57
@ericsnowcurrently ericsnowcurrently enabled auto-merge (squash) June 1, 2023 22:25
@ericsnowcurrently ericsnowcurrently merged commit 1469393 into python:main Jun 1, 2023
@ericsnowcurrently ericsnowcurrently deleted the fix-ob-type-before-ready branch June 1, 2023 22:34
@ericsnowcurrently ericsnowcurrently added the needs backport to 3.12 only security fixes label Jun 1, 2023
@miss-islington

Copy link
Copy Markdown
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 1, 2023
…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

Copy link
Copy Markdown

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

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jun 1, 2023
ericsnowcurrently pushed a commit that referenced this pull request Jun 1, 2023
…_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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants