Repository navigation
Py_DecodeLocale() fails if used before the runtime is initialized. #76277
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 = 'https://github.com/ericsnowcurrently'
closed_at = <Date 2017-11-26.05:41:53.556>
created_at = <Date 2017-11-20.21:28:36.310>
labels = ['interpreter-core', '3.7', 'type-crash']
title = 'Py_DecodeLocale() fails if used before the runtime is initialized.'
updated_at = <Date 2017-11-26.05:41:53.555>
user = 'https://github.com/ericsnowcurrently'bugs.python.org fields:
activity = <Date 2017-11-26.05:41:53.555>
actor = 'ncoghlan'
assignee = 'eric.snow'
closed = True
closed_date = <Date 2017-11-26.05:41:53.556>
closer = 'ncoghlan'
components = ['Interpreter Core']
creation = <Date 2017-11-20.21:28:36.310>
creator = 'eric.snow'
dependencies = []
files = []
hgrepos = []
issue_num = 32096
keywords = ['patch']
message_count = 32.0
messages = ['306585', '306587', '306591', '306614', '306654', '306655', '306656', '306658', '306659', '306660', '306662', '306689', '306708', '306717', '306718', '306737', '306740', '306764', '306766', '306769', '306772', '306774', '306889', '306893', '306895', '306896', '306938', '306949', '306982', '306987', '306989', '306993']
nosy_count = 5.0
nosy_names = ['ncoghlan', 'vstinner', 'eric.snow', 'serhiy.storchaka', 'Decorater']
pr_nums = ['4481', '4495', '4532', '4566']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue32096'
versions = ['Python 3.7']
Activity
ericsnowcurrently commentedon Nov 20, 2017
(see the python-dev thread [1])
(related: issue bpo-32086)
When I consolidated the global runtime state into a single global, _PyRuntime, calls Py_DecodeLocale() started to break if the runtime hadn't been intialized yet. This is because that function relies on PyMem_RawMalloc() and PyMem_RawFree(), which rely on the raw allocator having been initialized as part of the runtime (it used to be intialized statically).
The documentation for various "Process-wide parameters" [2] explicitly directs users to call Py_DecodeLocale() where necessary. The docs for Py_DecodeLocale(), in turn, explicitly refer to calling PyMem_RawFree(). So changing the pre-runtime-init behavior of Py_DecodeLocale() and PyMem_RawFree() is a regression that should be fixed.
[1] https://mail.python.org/pipermail/python-dev/2017-November/150605.html
[2] https://docs.python.org/3/c-api/init.html#process-wide-parameters
AraHaan commentedon Nov 20, 2017
Interesting, on 3.6.3 on my embedded program it seems to work just fine.
Did anything change in it since then?
https://github.com/AraHaan/Els_kom_new/blob/master/PC/komextract_new.c
vstinner commentedon Nov 20, 2017
Duplicate of bpo-32086.
ncoghlan commentedon Nov 21, 2017
While they're definitely closely related, I don't think this and bpo-32086 are actually duplicates: this issue is "fix the current Py_DecodeLocale regression in 3.7 by reverting back to the 3.6 behaviour", while bpo-32086 is "Should we deprecate our implied support for calling Py_DecodeLocale() before calling Py_Initialize()?".
The latter is certainly a valid question, but we should restore the 3.6 behaviour while we're considering it.
ericsnowcurrently commentedon Nov 21, 2017
I see at least 3 ways to sort this out:
("revert the change on memory allocators, and retry later to fix
it, once other initializations issues are fixed", as suggested by
Victor in the email thread)
to make PyMem_RawMalloc() and PyMem_RawFree() work pre-init (this
is what my PR does)
if the runtime has not been initialized yet
I considered implementing #3 instead, but wasn't sure about the performance impact. It would add a pointer comparison to NULL and a branch on each PyMem_RawMalloc() and PyMem_RawFree() call. I'm not convinved it would make much of a difference. Furthermore, I consider #3 to be the simplest solution, both to implement and to maintain, so I'll probably try it out.
ericsnowcurrently commentedon Nov 21, 2017
I thought issue bpo-32086 was about documentation (which is worth having a separate issue for), not about a fix to the regression.
vstinner commentedon Nov 21, 2017
"3. use hard-coded defaults in PyMem_RawMalloc() and PyMem_RawFree() if the runtime has not been initialized yet"
I dislike this option since it can have a negative impact on performances. The PEP-445 already added a new level of indirection and so made memory allocations a little bit slower.
vstinner commentedon Nov 21, 2017
I marked bpo-32096 as a duplicate of this one. I don't want to discuss the same issue in 3 places (2 bpo and python-dev).
vstinner commentedon Nov 21, 2017
Nick: "Should we deprecate our implied support for calling Py_DecodeLocale() before calling Py_Initialize()?"
Please don't do that. Py_DecodeLocale() is the best available function to decode paths and environment variables to initialize Python. The implementation of Py_DecodeLocale() is very complex, but we need this complexity to decode "correctly" OS data.
10 remaining items
ncoghlan commentedon Nov 23, 2017
Follow up: this also came up on https://bugs.python.org/issue32030#msg306763, and I think Victor and I are on the same page now :)
Since MainInterpreterConfig is currently still a private struct, we can store the existing C level config state directly in there for now, and then look at upgrading to Python types on a case by case basis.
Once they're all both consolidated *and* upgraded, then we can consider making the new incremental configuration API public.
vstinner commentedon Nov 23, 2017
"Victor, I think you're fundamentally misunderstanding the goals of PEP-432. The entire point is to let people have a *mostly working Python runtime* during CPython startup. (...)"
While the PEP-432 is nice, all changes are currently done in private APIs, symbols starting with _Py. I would prefer that nobody uses these new APIs before the conversion is complete. And from what I saw, I can say that the conversion just started, there are still a lot of changes that should be done.
While having _PyRuntime.mem is nice to have in the long term, it doesn't add any value *right now*, except of making the existing C API harder to use.
I would prefer to do things in this order:
Maybe we can complete these 3 steps before Python 3.7, but I'm not sure about that.
ncoghlan commentedon Nov 23, 2017
Even the public implementation of PEP-432 is going to bound by the requirement to keep existing embedding logic working, and that's encapsulated in the new test Eric added in his PR:
So even if we were to revert the _PyRuntime.mem change in 3.7, we'd still face the same problem in 3.8, because we'd still be exposing the traditional configuration API - the new multi-step configuration API would be *optional* for folks that wanted to override the default settings more easily, rather than a backwards compatibility break with the previously supported way of doing things.
As a result, my preferred option is now to make exactly the promises we need to ensure that the above code works correctly, and then have Py_Initialize and Py_Finalize enforce those constraints:
vstinner commentedon Nov 24, 2017
New changeset 9e87e77 by Victor Stinner in branch 'master':
bpo-32096: Remove obj and mem from _PyRuntime (bpo-4532)
9e87e77
vstinner commentedon Nov 24, 2017
The newly added test failed on AMD64 Debian root 3.x:
http://buildbot.python.org/all/#/builders/27/builds/226
======================================================================
FAIL: test_pre_initialization_api (test.test_capi.EmbeddingTests)
----------------------------------------------------------------------
vstinner commentedon Nov 24, 2017
We now check that Py_DecodeLocale() can be called before Py_Initialize(). IMHO we need to document this property in the documentation: I opened bpo-32124 and wrote a PR for that.
vstinner commentedon Nov 24, 2017
The test also failed on x86 Tiger 3.x:
http://buildbot.python.org/all/#/builders/30/builds/212
======================================================================
FAIL: test_pre_initialization_api (test.test_capi.EmbeddingTests)
----------------------------------------------------------------------
ncoghlan commentedon Nov 25, 2017
Huh, those crashes are interesting - I'd guess that it means we have a platform-dependent dependency from Py_DecodeLocale on to Py_SetPythonHome in order to locate the encodings module. If I'm right, that dependency would then mean that embedding applications can only rely on Py_DecodeLocale to do "char *" to "wchar_t *" conversions if they can also rely on the locale encoding always being a builtin one that bypasses the search for the encodings module.
Perhaps we should be recommending temporarily doing 'setenv("PYTHONHOME", home)' (and then reverting that after calling Py_Initialize so it doesn't get inherited by subprocesses) as the preferred approach to handling platforms with "char *" based native filesystem APIs, and adding such a setting to that particular
_testembedtest?vstinner commentedon Nov 25, 2017
The test calls Py_SetProgramName(). IMHO the bug is that the program name
is needed to locate the Python std lib.
I don't think that the bug is triggered by Py_DecodeLocale().
ncoghlan commentedon Nov 26, 2017
Ah, you're right - I forgot about this little hack in the other embedding tests: https://github.com/vstinner/cpython/blob/3fda852ba4d4040657a1b616a1ef60ad437b7845/Programs/_testembed.c#L11
I'll add "./" to the program name in the new test as well, and see if that's enough to make the failing build bots happy.
ncoghlan commentedon Nov 26, 2017
New changeset 4274609 by Nick Coghlan in branch 'master':
bpo-32096: Ensure new embedding test can find the encodings module (GH-4566)
4274609
ncoghlan commentedon Nov 26, 2017
Looking more closely at the code, I've realised Victor's right - there's no way for Py_DecodeLocale() to accidentally trigger an attempt to import the "encodings" module.
Instead, the error is likely coming from the init_sys_streams step towards the end of the initialization process. The way the embedded test cases are currently being run unfortunately truncated that traceback.
Rather than trying to improve the test case error reporting under the scope of this issue, I've instead filed https://bugs.python.org/issue32136 to cover factoring the runtime embedding tests out to their own test file.
ncoghlan commentedon Nov 26, 2017
Successful test run on the Debian machine that failed above:
And for the macOS Tiger machine:
So I think we can call the regression fixed, which is where we wanted to get to before the next alpha release.