Repository navigation
Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL #76555
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2018-05-15.21:20:58.194>
created_at = <Date 2017-12-19.14:40:38.428>
labels = ['extension-modules', 'type-bug', '3.7', 'docs']
title = 'Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL'
updated_at = <Date 2018-05-28.16:37:14.368>
user = 'https://github.com/encukou'bugs.python.org fields:
activity = <Date 2018-05-28.16:37:14.368>
actor = 'miss-islington'
assignee = 'docs@python'
closed = True
closed_date = <Date 2018-05-15.21:20:58.194>
closer = 'petr.viktorin'
components = ['Documentation', 'Extension Modules']
creation = <Date 2017-12-19.14:40:38.428>
creator = 'petr.viktorin'
dependencies = []
files = []
hgrepos = []
issue_num = 32374
keywords = ['patch']
message_count = 19.0
messages = ['308647', '308648', '308650', '309052', '309709', '313987', '313988', '313989', '317506', '317615', '317616', '317621', '317625', '317660', '317684', '317834', '317840', '317844', '317885']
nosy_count = 8.0
nosy_names = ['ncoghlan', 'pitrou', 'scoder', 'vstinner', 'petr.viktorin', 'docs@python', 'Dormouse759', 'miss-islington']
pr_nums = ['4611', '5140', '6128', '6129', '7090', '7101', '7102', '7145', '7147', '7150', '7155']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32374'
versions = ['Python 3.6', 'Python 3.7']
Activity
encukou commentedon Dec 19, 2017
After the create phase of multiphase initialization, the per-module state is NULL and the module object is reachable by the garbage collector. Between the create and exec phases, Python code is run and garbage collection can be triggered.
So, any custom m_traverse/m_clear/m_free function must be prepared to handle m_state being NULL. This is currently not well documented.
It might be useful to insert a call m_traverse after the first phase, at least in --with-pydebug mode, so the potential error gets triggered early.
encukou commentedon Dec 19, 2017
Marcel, could you look into this?
pitrou commentedon Dec 19, 2017
By the way, I think you forgot to answer my question on python-dev:
The doc currently isn't very clear about this.
ncoghlan commentedon Dec 26, 2017
Yep, the requirement for supporting multiple interpreters is just that you either avoid relying on C global state entirely, or else correctly synchronise access to it. Multi-phase initialisation just provides a few nudges in the direction of doing that more consistently.
Dormouse759 commentedon Jan 9, 2018
I have created a PR at #5140
It contains documentation, plus, in --with-pydebug mode, it calls m_traverse to catch easy cases of not handling the m_state==NULL case early.
ncoghlan commentedon Mar 17, 2018
New changeset c2b0b12 by Nick Coghlan (Marcel Plch) in branch 'master':
bpo-32374: m_traverse may be called with m_state=NULL (GH-5140)
c2b0b12
miss-islington commentedon Mar 17, 2018
New changeset 136905f by Miss Islington (bot) in branch '3.7':
bpo-32374: m_traverse may be called with m_state=NULL (GH-5140)
136905f
miss-islington commentedon Mar 17, 2018
New changeset 1da0479 by Miss Islington (bot) in branch '3.6':
bpo-32374: m_traverse may be called with m_state=NULL (GH-5140)
1da0479
vstinner commentedon May 24, 2018
MultiPhaseExtensionModuleTests.test_bad_traverse() of Lib/test/test_importlib/extension/test_loader.py does create a coredump file. The script run by the test trigger a segfault. Is it deliberate? See bpo-33629 and my PR 7090.
vstinner commentedon May 24, 2018
New changeset 483000e by Victor Stinner in branch 'master':
bpo-33629: Prevent coredump in test_importlib (GH-7090)
483000e
vstinner commentedon May 24, 2018
MultiPhaseExtensionModuleTests.test_bad_traverse() of Lib/test/test_importlib/extension/test_loader.py runs the following code:
---
And then check that the Python "failed": that the exit code is non-zero...
That's a weak test: if the script fails before calling spec.loader.create_module(), the test also pass. If the function raises an exception but don't crash, the test pass as well.
More generally, I'm not sure about the idea of making sure that doing bad stuff with traverse does crash. What is the purpose of the test?
In the meanwhile, I fixed bpo-33629 by adding test.support.SuppressCrashReport().
I'm not asking to do something. Maybe it's fine to keep the current test.
miss-islington commentedon May 24, 2018
New changeset d9eb22c by Miss Islington (bot) in branch '3.7':
bpo-33629: Prevent coredump in test_importlib (GH-7090)
d9eb22c
miss-islington commentedon May 24, 2018
New changeset fc0356d by Miss Islington (bot) in branch '3.6':
bpo-33629: Prevent coredump in test_importlib (GH-7090)
fc0356d
encukou commentedon May 25, 2018
That's a good argument, thanks.
Marcel, would you mind adding a patch for that?
This particular kind of bad traverse is quite easy to write if an extension author doesn't read docs carefully, or has read an old version of them (before the fix here). And resulting errors aren't too obvious. So, there's code to crash early and loudly in "--with-pydebug" for simple oversight cases. See comment at the call site of bad_traverse_test in Objects/moduleobject.c
And since there's code, there's a test to make sure it works :)
Thanks! Didn't know about that one, will keep it in mind for next time!
vstinner commentedon May 25, 2018
"""
This particular kind of bad traverse is quite easy to write if an extension author doesn't read docs carefully, or has read an old version of them (before the fix here). And resulting errors aren't too obvious. So, there's code to crash early and loudly in "--with-pydebug" for simple oversight cases. See comment at the call site of bad_traverse_test in Objects/moduleobject.c
And since there's code, there's a test to make sure it works :)
"""
Oh ok, it makes sense. Maybe the test should test at least just before spec.loader.create_module(). Maybe using a print().
"Thanks! Didn't know about that one, will keep it in mind for next time!"
The problem is that by default, on Linux, we don't dump core files on the current directory. So such bug is silent on Linux. But it's commonly detected on FreeBSD since I configured the test runner to fail if it leaves a new file.
encukou commentedon May 28, 2018
Marcel added PR 7145 for that -- it wraps things in try/except to catch Python-level exceptions sounds like a good solution to me.
encukou commentedon May 28, 2018
New changeset 08c5aca by Petr Viktorin (Marcel Plch) in branch 'master':
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (GH-7145)
08c5aca
encukou commentedon May 28, 2018
New changeset 983c1ba by Petr Viktorin (Miss Islington (bot)) in branch '3.7':
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (GH-7145) (GH-7150)
983c1ba
miss-islington commentedon May 28, 2018
New changeset 6ec325d by Miss Islington (bot) in branch '3.6':
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (GH-7145)
6ec325d