Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrentlyericsnowcurrently commentedMay 30, 2023
edited by bedevere-bot
Loading

When I added the relevant condition totype_ready_set_bases() ingh-103912, I had missed that the function also setstp_base andob_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.

@ericsnowcurrentlyericsnowcurrently marked this pull request as ready for reviewMay 31, 2023 00:20
@ericsnowcurrentlyericsnowcurrently removed the request for review frommarkshannonMay 31, 2023 00:20
Copy link
Member

@lysnikolaoulysnikolaou left a 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 solvesnumpy/numpy#23766.

@erlend-aasland
Copy link
Contributor

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

@lysnikolaou
Copy link
Member

lysnikolaou commentedMay 31, 2023
edited
Loading

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).

#definePY_SSIZE_T_CLEAN#include"Python.h"staticPyTypeObjectCustomBaseType1= {PyVarObject_HEAD_INIT(NULL,0)    .tp_name="custom.Base1",    .tp_basicsize=sizeof(PyObject),};staticPyTypeObjectCustomBaseType2= {PyVarObject_HEAD_INIT(NULL,0)    .tp_name="custom.Base2",    .tp_basicsize=sizeof(PyObject),};staticPyTypeObjectCustomDerivedType= {PyVarObject_HEAD_INIT(NULL,0)    .tp_name="custom.Derived",    .tp_basicsize=sizeof(PyObject),};staticPyModuleDefcustommodule= {PyModuleDef_HEAD_INIT,    .m_name="custom",    .m_doc="Example module that creates an extension type.",    .m_size=-1,};PyMODINIT_FUNCPyInit_custom(void){PyObject*m;if (PyType_Ready(&CustomBaseType1)<0)returnNULL;if (PyType_Ready(&CustomBaseType2)<0)returnNULL;CustomDerivedType.tp_base=&CustomBaseType2;CustomDerivedType.tp_bases=Py_BuildValue("(OO)",&CustomBaseType2,&CustomBaseType1);CustomDerivedType.tp_hash=CustomBaseType1.tp_hash;if (PyType_Ready(&CustomDerivedType)<0)returnNULL;m=PyModule_Create(&custommodule);if (m==NULL)returnNULL;Py_INCREF(&CustomDerivedType);if (PyModule_AddObject(m,"Custom", (PyObject*)&CustomDerivedType)<0) {Py_DECREF(&CustomDerivedType);Py_DECREF(m);returnNULL;    }returnm;}

It segfaults on import on main, and is okay with this PR.

erlend-aasland and ericsnowcurrently reacted with thumbs up emoji

@markshannon
Copy link
Member

Would it make sense to allocate the tuples withmalloc 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
MemberAuthor

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
MemberAuthor

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

erlend-aasland reacted with thumbs up emoji

@lysnikolaou
Copy link
Member

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

@ericsnowcurrentlyericsnowcurrentlyenabled auto-merge (squash)June 1, 2023 22:25
@ericsnowcurrentlyericsnowcurrently merged commit1469393 intopython:mainJun 1, 2023
@ericsnowcurrentlyericsnowcurrently deleted the fix-ob-type-before-ready branchJune 1, 2023 22:34
@ericsnowcurrentlyericsnowcurrently added the needs backport to 3.12only security fixes labelJun 1, 2023
@miss-islington
Copy link
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 requestJun 1, 2023
…Ready() (pythongh-105122)When I added the relevant condition to type_ready_set_bases() inpythongh-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 commit1469393)Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
@bedevere-bot
Copy link

GH-105211 is a backport of this pull request to the3.12 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.12only security fixes labelJun 1, 2023
ericsnowcurrently pushed a commit that referenced this pull requestJun 1, 2023
…_Ready() (gh-105122) (gh-105211)When I added the relevant condition to type_ready_set_bases() ingh-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 commit1469393)Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
cls->tp_base = (PyTypeObject *)Py_NewRef(base);
cls->tp_bases = Py_BuildValue("(O)", base);
if (cls->tp_bases == NULL) {
return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should decreftp_base andtp_bases before returning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Openedgh-105225.

ericsnowcurrently reacted with thumbs up emoji
Copy link
MemberAuthor

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!

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
Copy link
Contributor

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 intest_import instead of the hack suggested ingh-104796.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps alsogh-105085 should have been solved like this.

Copy link
MemberAuthor

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

erlend-aasland reacted with thumbs up emoji
Copy link
Member

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 wholeclass Test instead of a smallerdef test_...?

Copy link
Contributor

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.

Copy link
Contributor

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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@erlend-aaslanderlend-aaslanderlend-aasland left review comments

@sunmy2019sunmy2019sunmy2019 left review comments

@lysnikolaoulysnikolaoulysnikolaou approved these changes

@Yhg1sYhg1sAwaiting requested review from Yhg1s

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@ericsnowcurrently@erlend-aasland@lysnikolaou@markshannon@miss-islington@bedevere-bot@sunmy2019

[8]ページ先頭

©2009-2025 Movatter.jp