Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
lysnikolaou opened this issue May 18, 2023 · 14 comments
Closed
Assignees
Labels
type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@lysnikolaou
Copy link
Member

lysnikolaou commented May 18, 2023

Bug report

This came up while testing numpy on 3.12. A bug report has been filed on numpy/numpy#23766. It's happening due to #103912, which introduced an additional check for tp_bases not being NULL in type_ready_set_bases, which is called from PyType_Ready.

numpy sets tp_bases manually before calling PyType_Ready, which means that the afore-mentioned check succeeds, and so, the line that sets ob_type does not get executed (it used to before #103912), which leads to a segmentation fault later on, when trying to set mro.

This looks like a bug, but I'm not sure whether that's expected and numpy should be adjusted. If the latter is true, should a note be added in the What's new document?

Linked PRs

@lysnikolaou lysnikolaou added the type-crash A hard crash of the interpreter, possibly with a core dump label May 18, 2023
@lysnikolaou
Copy link
Member Author

cc @ericsnowcurrently as the author of #103912.

@lysnikolaou
Copy link
Member Author

Verified that the following diff solves this issue, and the test suite passes. (Still not sure whether it has any unintended side-effects though)

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index 2fbcafe91a..495e123f8f 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -6990,10 +6990,6 @@ type_ready_pre_checks(PyTypeObject *type)
 static int
 type_ready_set_bases(PyTypeObject *type)
 {
-    if (lookup_tp_bases(type) != NULL) {
-        return 0;
-    }
-
     /* Initialize tp_base (defaults to BaseObject unless that's us) */
     PyTypeObject *base = type->tp_base;
     if (base == NULL && type != &PyBaseObject_Type) {

@ericsnowcurrently
Copy link
Member

I should have a fix up shortly.

@ericsnowcurrently
Copy link
Member

@lysnikolaou, please verify that gh-105122 fixes the problem. If not, what would be a good way to reproduce the problem?

ericsnowcurrently added a commit that referenced this issue Jun 1, 2023
gh-105122)

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.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue 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>
ericsnowcurrently pushed a commit that referenced this issue 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
@ericsnowcurrently
Copy link
Member

main and 3.12 should be good now.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jun 2, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 2, 2023
…_static_type() (pythonGH-105225)

(cherry picked from commit e01b04c)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit that referenced this issue Jun 2, 2023
…c_static_type() (GH-105225) (#105248)

(cherry picked from commit e01b04c)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@scoder
Copy link
Contributor

scoder commented Jun 4, 2023

Thanks for the fix. This currently crashes multiple-inheritance code generated by both Cython and pybind11 (@rwgk), so this is a blocker for us (and at least some of our users) testing Py3.12b1. The bug is also difficult to work around, so I'd rather wait for Py3.12b2.

scoder added a commit to cython/cython that referenced this issue Jun 5, 2023
scoder added a commit to cython/cython that referenced this issue Jun 5, 2023
@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Jun 5, 2023

@scoder, do you mean the issue is fixed for you with the patch I landed, or that the patch caused a new problem, or that the issue still isn't fixed, or something else? I think you meant the first one, but I want to be sure. 😄

@scoder
Copy link
Contributor

scoder commented Jun 5, 2023 via email

@ericsnowcurrently
Copy link
Member

Thanks for clarifying. It's a relief to hear that. 😄

@tacaswell
Copy link
Contributor

per https://www.riverbankcomputing.com/pipermail/pyqt/2023-June/045325.html There may still be issues with PyQt6 / sip .

I still see this segfault with current CPython main and it is reported that the failure bisects back to #103912 as well.

@The-Compiler
Copy link
Contributor

The-Compiler commented Jun 9, 2023

I'm the person who posted that email - the issue currently is that:

import sys
from PyQt6.QtWidgets import QWidget, QApplication

class Mixin:
    pass

class Command(Mixin, QWidget):
    pass

app = QApplication(sys.argv)
cmd = Command()
cmd.show()
app.exec()

Segfaults in:

#0  _PyDict_Next (op=0x0, ppos=0x7fffffffbcd0, pkey=0x7fffffffbcd8, pvalue=0x7fffffffbce0, phash=phash@entry=0x0) at Objects/dictobject.c:2114
#1  0x000055555572a2d9 in PyDict_Next (op=<optimized out>, ppos=<optimized out>, pkey=<optimized out>, pvalue=<optimized out>) at Objects/dictobject.c:2189
#2  0x00007ffff2436499 in trawl_hierarchy(_typeobject*, qpycore_metaobject*, QMetaObjectBuilder&, QList<EnumFlag>&, QList<_qpycore_pyqtSignal const*>&, QMap<unsigned int, std::pair<_object*, _object*> >&) ()
   from /home/florian/proj/qutebrowser/.venv/lib/python3.13/site-packages/PyQt6/QtCore.abi3.so
#3  0x00007ffff2436cc7 in trawl_hierarchy(_typeobject*, qpycore_metaobject*, QMetaObjectBuilder&, QList<EnumFlag>&, QList<_qpycore_pyqtSignal const*>&, QMap<unsigned int, std::pair<_object*, _object*> >&) ()
   from /home/florian/proj/qutebrowser/.venv/lib/python3.13/site-packages/PyQt6/QtCore.abi3.so
#4  0x00007ffff2436cc7 in trawl_hierarchy(_typeobject*, qpycore_metaobject*, QMetaObjectBuilder&, QList<EnumFlag>&, QList<_qpycore_pyqtSignal const*>&, QMap<unsigned int, std::pair<_object*, _object*> >&) ()
   from /home/florian/proj/qutebrowser/.venv/lib/python3.13/site-packages/PyQt6/QtCore.abi3.so
#5  0x00007ffff24357ba in qpycore_get_dynamic_metaobject(_sipWrapperType*) [clone .part.0] () from /home/florian/proj/qutebrowser/.venv/lib/python3.13/site-packages/PyQt6/QtCore.abi3.so
#6  0x00007ffff24363d8 in qpycore_get_qmetaobject(_sipWrapperType*, _sipTypeDef const*) () from /home/florian/proj/qutebrowser/.venv/lib/python3.13/site-packages/PyQt6/QtCore.abi3.so
#7  0x00007ffff242a4e5 in qpycore_qobject_finalisation(_object*, QObject*, _object*, _object**) () from /home/florian/proj/qutebrowser/.venv/lib/python3.13/site-packages/PyQt6/QtCore.abi3.so
#8  0x00007ffff74f0569 in sipSimpleWrapper_init (self=0x7ffff7652840, args=(), kwds=0x0) at sip_core.c:8950
#9  0x000055555575906b in type_call (type=<optimized out>, type@entry=0x555555ea2470, args=args@entry=(), kwds=kwds@entry=0x0) at Objects/typeobject.c:1671
#10 0x00005555556eb3b2 in _PyObject_MakeTpCall (tstate=tstate@entry=0x555555c76688 <_PyRuntime+475432>, callable=callable@entry=<PyQt6.sip.wrappertype at remote 0x555555ea2470>, args=args@entry=0x7ffff7f7f078, nargs=<optimized out>, 
    keywords=keywords@entry=0x0) at Objects/call.c:240
[...]

And indeed I could bisect this to de64e75. The main
branch fails in the same way.

Happy to open a new issue, but I'm first trying to see if the PyQt author can perhaps
help boil this down to a more minimal example not requiring PyQt.

@ericsnowcurrently
Copy link
Member

@The-Compiler, this is a separate issue, related to tp_dict rather than tp_bases and ob_type. It would be worth opening a new issue for this.

In the meantime, is the failure with a builtin static type or a user-defined type? Was PyType_Ready() called on that type already? Do you have the full stack trace?

@ericsnowcurrently
Copy link
Member

Presumably the crashing code is trying to use the dict returned by sipPyTypeDict() and I'm guessing that that function basically returns pytype->tp_dict and that the pytype in question is a static builtin type. If that's the case then the failure makes sense since tp_dict for static builtin types is now NULL. In 3.12 we added an internal getter function (_PyType_GetDict() via Py_BUILD_CORE) but not a public one.

This sounds like gh-105227. If so, let's continue the conversation there. If not then let's create a new issue.

@ericsnowcurrently
Copy link
Member

Either way, I'm closing this issue again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Development

No branches or pull requests

5 participants