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
Uh oh!
There was an error while loading.Please reload this page.
Changes from1 commit
fe12a9086f8ec6e4337e2b68011708a032180d20c6File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
- Loading branch information
Uh oh!
There was an error while loading.Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1629,27 +1629,34 @@ def test_tp_mro_is_set(self): | ||
| class TestStaticTypes(unittest.TestCase): | ||
| _has_run = False | ||
| @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 | ||
Comment on lines +1634 to +1640 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This is nice. IMO, we should use a similar approach in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others.Learn more.
Wouldn't this skip the whole There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others.Learn more. Alternatively, create a decorator for it. | ||
| @contextlib.contextmanager | ||
| def basic_static_type(self, *args): | ||
| cls = _testcapi.get_basic_static_type(*args) | ||
| yield cls | ||
| def test_pytype_ready_always_sets_tp_type(self): | ||
| # The point of this test is to prevent something like | ||
| # https://github.com/python/cpython/issues/104614 | ||
| # from happening again. | ||
| # First check when tp_base/tp_bases is *not* set before PyType_Ready(). | ||
| withself.basic_static_type() as cls: | ||
| self.assertIs(cls.__base__, object); | ||
| self.assertEqual(cls.__bases__, (object,)); | ||
| self.assertIs(type(cls), type(object)); | ||
| # Then check when we *do* set tp_base/tp_bases first. | ||
| withself.basic_static_type(object) as cls: | ||
| self.assertIs(cls.__base__, object); | ||
| self.assertEqual(cls.__bases__, (object,)); | ||
| self.assertIs(type(cls), type(object)); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -2627,13 +2627,20 @@ type_get_tp_mro(PyObject *self, PyObject *type) | ||
| } | ||
| /* We only use 2 in test_capi/test_misc.py. */ | ||
| #define NUM_BASIC_STATIC_TYPES 2 | ||
| static PyTypeObject BasicStaticTypes[NUM_BASIC_STATIC_TYPES] = { | ||
| #define INIT_BASIC_STATIC_TYPE \ | ||
| { \ | ||
| PyVarObject_HEAD_INIT(NULL, 0) \ | ||
| .tp_name = "BasicStaticType", \ | ||
| .tp_basicsize = sizeof(PyObject), \ | ||
| } | ||
| INIT_BASIC_STATIC_TYPE, | ||
| INIT_BASIC_STATIC_TYPE, | ||
| #undef INIT_BASIC_STATIC_TYPE | ||
| }; | ||
| static int num_basic_static_types_used = 0; | ||
| static PyObject * | ||
| get_basic_static_type(PyObject *self, PyObject *args) | ||
| @@ -2644,42 +2651,25 @@ get_basic_static_type(PyObject *self, PyObject *args) | ||
| } | ||
| assert(base == NULL || PyType_Check(base)); | ||
| if(num_basic_static_types_used >= NUM_BASIC_STATIC_TYPES) { | ||
| PyErr_SetString(PyExc_RuntimeError, "no more available basic static types"); | ||
| return NULL; | ||
| } | ||
| PyTypeObject *cls = &BasicStaticTypes[num_basic_static_types_used++]; | ||
| if (base != NULL) { | ||
| 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 commentThe reason will be displayed to describe this comment to others.Learn more. Should decref There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others.Learn more. Thanks for following up after-the-fact! | ||
| } | ||
| } | ||
| if (PyType_Ready(cls) < 0) { | ||
| return NULL; | ||
| } | ||
| return (PyObject *)cls; | ||
| } | ||
| // Test PyThreadState C API | ||
| static PyObject * | ||
| @@ -3439,7 +3429,6 @@ static PyMethodDef TestMethods[] = { | ||
| {"type_get_tp_bases", type_get_tp_bases, METH_O}, | ||
| {"type_get_tp_mro", type_get_tp_mro, METH_O}, | ||
| {"get_basic_static_type", get_basic_static_type, METH_VARARGS, NULL}, | ||
| {"test_tstate_capi", test_tstate_capi, METH_NOARGS, NULL}, | ||
| {"frame_getlocals", frame_getlocals, METH_O, NULL}, | ||
| {"frame_getglobals", frame_getglobals, METH_O, NULL}, | ||