GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-104614: Make Sure ob_type is Always Set Correctly by PyType_Ready() by ericsnowcurrently · Pull Request #105122 · python/cpython

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented May 30, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ericsnowcurrently ericsnowcurrently added the skip news label May 30, 2023
@ericsnowcurrently
ericsnowcurrently requested review from Yhg1s and lysnikolaou May 30, 2023 23:01
@bedevere-bot bedevere-bot mentioned this pull request May 30, 2023
Type object's ob_type does not get set when tp_bases is set before PyType_Ready #104614
Closed
ericsnowcurrently added 2 commits May 30, 2023 18:17
@ericsnowcurrently
Split up type_ready_set_bases().
fe12a90
@ericsnowcurrently
Fix the short-circuit in type_ready_set_bases().
86f8ec6
@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 requested a review from markshannon as a code owner May 31, 2023 00:20
@bedevere-bot bedevere-bot added the awaiting core review label May 31, 2023
@ericsnowcurrently
ericsnowcurrently removed the request for review from markshannon May 31, 2023 00:20
@ericsnowcurrently
Fix the empty tuple case for clear_tp_bases().
e4337e2
lysnikolaou
lysnikolaou approved these changes May 31, 2023
View reviewed changes

@lysnikolaou lysnikolaou left a comment

Copy link
Copy Markdown
Member

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 @ericsnowcurrently! I can verify that this solves numpy/numpy#23766.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 31, 2023
@erlend-aasland

erlend-aasland commented May 31, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@lysnikolaou

lysnikolaou commented May 31, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

erlend-aasland and ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented May 31, 2023

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ericsnowcurrently

ericsnowcurrently commented May 31, 2023

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ericsnowcurrently
Add a regression test.
b680117
@ericsnowcurrently

ericsnowcurrently commented May 31, 2023

Copy link
Copy Markdown
Member Author

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

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ericsnowcurrently
Ignore the test static type.
08a0321
@lysnikolaou

lysnikolaou commented Jun 1, 2023

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ericsnowcurrently
Fix the test.
80d20c6
@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
@bedevere-bot bedevere-bot removed the awaiting merge label 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

miss-islington commented Jun 1, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 1, 2023
@ericsnowcurrently @miss-islington
pythongh-104614: Make Sure ob_type is Always Set Correctly by PyType_…
d374433
…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 <[email protected]>
@bedevere-bot

bedevere-bot commented Jun 1, 2023

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
@miss-islington
[3.12] gh-104614: Make Sure ob_type is Always Set Correctly by PyType…
d57ee81
…_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 [email protected]
erlend-aasland
erlend-aasland reviewed Jun 2, 2023
View reviewed changes
Comment thread Modules/_testcapimodule.c
cls->tp_base = (PyTypeObject *)Py_NewRef(base);
cls->tp_bases = Py_BuildValue("(O)", base);
if (cls->tp_bases == NULL) {
return NULL;

@erlend-aasland erlend-aasland Jun 2, 2023

Copy link
Copy Markdown
Contributor

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 decref tp_base and tp_bases before returning.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@erlend-aasland erlend-aasland Jun 2, 2023

Copy link
Copy Markdown
Contributor

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.

Opened gh-105225.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@ericsnowcurrently ericsnowcurrently Jun 2, 2023

Copy link
Copy Markdown
Member Author

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
erlend-aasland
erlend-aasland reviewed Jun 2, 2023
View reviewed changes
Comment thread Lib/test/test_capi/test_misc.py
Comment on lines +1634 to +1640
@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

@erlend-aasland erlend-aasland Jun 2, 2023

Copy link
Copy Markdown
Contributor

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 in test_import instead of the hack suggested in gh-104796.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@erlend-aasland erlend-aasland Jun 2, 2023

Copy link
Copy Markdown
Contributor

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 also gh-105085 should have been solved like this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ericsnowcurrently ericsnowcurrently Jun 2, 2023

Copy link
Copy Markdown
Member Author

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 there may 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@sunmy2019 sunmy2019 Jun 3, 2023

Copy link
Copy Markdown
Member

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 whole class Test instead of a smaller def test_...?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@erlend-aasland erlend-aasland Jun 3, 2023

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@erlend-aasland erlend-aasland Jun 3, 2023

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@erlend-aasland erlend-aasland mentioned this pull request Jun 5, 2023
gh-102251: add missing cleanups for test_import #104796
Closed
@sunmy2019 sunmy2019 mentioned this pull request Jun 5, 2023
Allow test cases to skip rerun #105310
Closed
@erlend-aasland erlend-aasland mentioned this pull request Jun 6, 2023
3.12 backport gh 105236 #105358
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments
@sunmy2019 sunmy2019 sunmy2019 left review comments
@lysnikolaou lysnikolaou lysnikolaou approved these changes
@Yhg1s Yhg1s Awaiting requested review from Yhg1s

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@ericsnowcurrently @erlend-aasland @lysnikolaou @markshannon @miss-islington @bedevere-bot @sunmy2019
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.