Repository navigation
New 3.7 startup sequence crashes PyInstaller #77223
Description
| BPO | 33042 |
|---|---|
| Nosy | @ncoghlan, @vstinner, @ned-deily, @pmp-p, @ericsnowcurrently, @miss-islington, @springermac |
| PRs | |
| Files |
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/ncoghlan'
closed_at = <Date 2018-03-26.12:34:49.856>
created_at = <Date 2018-03-10.17:00:47.436>
labels = ['interpreter-core', '3.8', 'type-bug', '3.7']
title = 'New 3.7 startup sequence crashes PyInstaller'
updated_at = <Date 2018-03-26.12:50:47.154>
user = 'https://bugs.python.org/htgoebel'bugs.python.org fields:
activity = <Date 2018-03-26.12:50:47.154>
actor = 'ncoghlan'
assignee = 'ncoghlan'
closed = True
closed_date = <Date 2018-03-26.12:34:49.856>
closer = 'ncoghlan'
components = ['Interpreter Core']
creation = <Date 2018-03-10.17:00:47.436>
creator = 'htgoebel'
dependencies = []
files = ['47477', '47485']
hgrepos = []
issue_num = 33042
keywords = ['patch']
message_count = 22.0
messages = ['313546', '313643', '313649', '313651', '313815', '313816', '313817', '313823', '313871', '313903', '314006', '314138', '314194', '314366', '314396', '314405', '314407', '314429', '314433', '314434', '314449', '314450']
nosy_count = 8.0
nosy_names = ['htgoebel', 'ncoghlan', 'vstinner', 'ned.deily', 'pmpp', 'eric.snow', 'miss-islington', 'springermac']
pr_nums = ['6157', '6232']
priority = 'critical'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33042'
versions = ['Python 3.7', 'Python 3.8']
Activity
htgoebel commentedon Mar 10, 2018
PyInstaller is a tool for freezing Python applications into stand-alone packages, much like py2exe. py2app, and bbfreeze. PyInstaller is providing *one* bootloader for all versions of Python supported (2.7, 3.4-3.6).
In PyInstaller the startup sequence is implemented in
pyi_pylib_start_python() in bootloader/src/pyi_pythonlib.c. The workflow roughly is:
The crash occurs due to tstate (thread state) not being initialized when
calling PySys_AddWarnOption.
ncoghlan commentedon Mar 12, 2018
I don't think we made any start-up changes that were specific to PySys_AddWarnOption, so my suspicion is that the crash is going to be related to a change in the constraints on either the unicode object creation or the list append operation.
The attached patch adds a new test case (I also cleaned up several other details related to test_embed execution in order to more easily see where the segfault happens on failure).
Unfortunately, it isn't yet suitable for use with
git bisect, as I checked 3.7.0a3 (the earliest tag where the patch applied cleanly), and that commit also segfaults. So setting up for git bisect testing (as per the hot-fix example in https://git-scm.com/docs/git-bisect#_examples ) will require:vstinner commentedon Mar 12, 2018
Python documentation was enhanced in Python 3.7 to explicitly list all functions safe to call *before* Py_Initialize():
https://docs.python.org/dev/c-api/init.html#before-python-initialization
PySys_AddWarnOption() is not part of the list. While it's not in the list, I'm kind of unhappy that we broke your use case: it wasn't my intent. Because I broken your use case with this change part of the big bpo-32030 refactoring:
commit f7e5b56
Author: Victor Stinner <[email protected]>
Date: Wed Nov 15 15:48:08 2017 -0800
IHMO the regression is that PySys_AddWarnOption() now calls _PySys_GetObjectId(): in Python 3.6, it wasn't the case.
Python 3.6 code:
---
Again, it's a bad idea to use the Python API before Py_Initialize(): you likely have to build a Unicode string and you use a list, whereas these two object types are not properly initialized...
The PEP-432 and bpo-32030 prepared Python to have a much better API in Python 3.8 for embedding Python. You will be able to use a wchar_t* string to pass warning options.
ncoghlan commentedon Mar 12, 2018
Adding a bit of context from my prior email discussion with Hartmut: CPython actually reads sys.warnoptions at the end of Py_Initialize (its the last thing we do before the function returns).
It also reads sys._xoptions during startup, since that's one way of enabling settings like dev mode and UTF-8 mode.
That means that even though PySys_AddWarnOption and PySys_AddXOption weren't documented as being safe to call before Py_Initialize, they likely *won't* have the desired effect if an embedding application defers calling them until later, and their absence from the list of "safe to call before Py_Initialize" functions was itself a documentation bug.
For 3.8, I'd hoped that the resolution might be as simple as a hard requirement on embedding applications to call PyRuntime_Initialize() before doing *anything* with the C API, but including the "internal/pystate.h" header and adding a call to _PyRuntime_Initialize() wasn't enough to keep the draft test case in my patch from segfaulting :(
ncoghlan commentedon Mar 14, 2018
b4d1e1f (the first attempted fix for bpo-20891) is the last commit where the draft test case patch applies cleanly.
That still segfaults, so I'll jump all the way back to 3.7.0a1, rework the patch to apply there, and see if pre-init use of these APIs were still working back then.
ncoghlan commentedon Mar 14, 2018
After reworking the patch to backport the pre-initialization embedding tests to 3.7.0a1, they *both* failed, as that was before Victor fixed Py_DecodeLocale to work prior to initialization again.
As a result, I tried 43605e6 in November, after the initial merge of the PEP-432 working branch, but just before the start of Victor's follow on refactoring to really take advantage of the new internal structures: double failure there as well.
f5ea83f is the next point I checked (just before Eric's September changes to global state management), and it looks like we have a new chief suspect, as applying the attached "earlier-tree" version of the patch at this point in the history gives two passing test cases :)
Next I'll work out a git bisect run based on those two start and end commits and the "earlier-tree" patch, and attempt to narrow the failure down to a specific commit (probably tomorrow).
(Even without that bisect run though, the warnoptions changes in 2ebc5ce#diff-f38879f4833a6b6847e556b9a07bf4ed are now looking *very* suspicious)
vstinner commentedon Mar 14, 2018
I suggest to revert my changes on PySys_AddWarnOption*() and "just" re-use the code from Python 3.6.
Previously, I made the code more complex since I had to expose a private function to add manually warning options. But later, the code changed again, so the private function is gone.
ncoghlan commentedon Mar 14, 2018
While I haven't definitely narrowed it down yet, I suspect it isn't your changes that are the problem: since the reported crash relates to attempting to access a not-yet-created thread state,
warnoptionsand_xoptionslikely need to become C level globals again for 3.7 (along with anything needed to create list and Unicode objects).We can then transfer them from there into Eric's new runtime state objects in _PyRuntime_Initialize (or potentially _Py_InitializeCore).
ncoghlan commentedon Mar 15, 2018
Patched test_capi results for 2ebc5ce (the global state consolidation commit): both pre-initialization tests fail
Patched test_capi results for bab21fa (the immediately preceding commit): both pre-initialization tests pass
Ding, ding, ding, we have a winner :)
Rather than reverting the options-related aspects of that change though, I think a nicer way to handle it will be to add a "pre-init fallback" path to those functions that stores the raw wchar_t data, and postpones converting those values to Py_Unicode objects until we need them in _PyInitialize_Core.
I'm not sure what to do about PySys_AddWarnOptionUnicode though: I think that's just outright incoherent as an API, since you need to call it before Py_Initialize for it to do anything useful, but we don't support calling any of the Unicode creation APIs until *after* Py_Initialize.
ned-deily commentedon Mar 15, 2018
Nick, what's your assessment of this issue's priority? Is it a release blocker for 3.7.0b3 (proposed ABI freeze)? For 3.7.0rc1 (code freeze)? None of the above?
ncoghlan commentedon Mar 17, 2018
This shouldn't affect the public ABI, so I don't think it's a blocker for the ABI freeze, but it's a regression that effectively makes PySys_AddWarnOption (and related APIs) unusable, so we should definitely fix it before shipping 3.7.
For now, I've marked the bug as critical, and I'll be aiming to have it fixed before 3.7.0b3 at the end of the month.
For PySys_AddWarnOption and PySys_ResetWarnOptions, my current plan is to adjust them to maintain a singly-linked list of "wchar *" pointers when called prior to _Py_InitializeCore (i.e. when "PyThreadState_GET() == NULL"), while using the existing 3.7.0b2 path when the thread state is available. _Py_InitializeCore will then take care of cleaning up the memory allocations while converting them to Python unicode entries in a Python list object.
_PySys_AddXOption would work similarly (just with a separate list).
For PySys_AddWarnOptionUnicode, I think we need to make it call Py_FatalError for 3.7 when called prior to Py_Initialize, with a recommendation to use PySys_AddWarnOption instead. In 3.8, that API will become useful again, once the public Py_InitializeCore API provides a supported multi-stage startup process that allows unicode objects to be created safely before Py_InitializeMainInterpreter (or whatever we end up calling that API) reads sys.warnoptions and configures the warning subsystem accordingly.
ncoghlan commentedon Mar 20, 2018
The linked PR has the draft test case for this - it goes beyond the ones I used to find the cause of the problem by actually checking that sys.warnoptions and sys._xoptions have been populated as expected.
ncoghlan commentedon Mar 21, 2018
Thinking about PySys_AddWarnOptionUnicode a little further, I'm not sure we actually need to change anything in the implementation there - I think we can just make it clearer in the docs that *only* PySys_AddWarnOption is supported prior to Py_Initialize for the embedding use case - the unicode version of the API currently only makes sense in CPython, where we can call it after _Py_InitializeCore.
It will make sense for embedding applications in 3.8 once we make Py_InitializeCore a public API.
ncoghlan commentedon Mar 24, 2018
PR has been updated to be mostly complete (just pending docs changes now), but I think I've found a potential issue with the interaction between the way I've currently implemented it and the way _Py_InitializeCore and _Py_InitializeMainInterpreter work.
Specifically, sys.warnoptions and sys._xoptions don't get created until _PySys_EndInit, so that's where I added the code to read the pre-initialization linked lists and move those values into sys.warnoptions and sys._xoptions. The current test is just checking that those sys attribute have the expected contents - it isn't checking that the consequences of those settings have correctly propagated to the warnings filter list.
For the default filters add by
_PyWarnings_Initat the end of_Py_InitializeCore, I think that's fine - we're going to want the embedding application's filters to be add after the default filter list anyway.However, the code in
_PyInitialize_MainInterpreterto actually import the warnings module (which then readssys.warnoptions) is guarded by a check for a non-empty config->warnoptions, and that's not going to trip in the case where get_warnoptions() has created a new sys.warnoptions list, and config->warnoptions is still NULL.Rather than changing the preinit sys module code to be config-aware, I'm thinking that what I'd like to do is:
ncoghlan commentedon Mar 25, 2018
I added the new test case, and found to my surprise that it didn't fail, even in debug mode (where there aren't any default filters).
The point I had missed was that even though warnoptions can be NULL in CoreConfig, _Py_MainInterpreterConfig_Read ensures that it's never NULL for the main interpreter config (which is what _PySys_EndInit reads), which means that neither we nor any embedding application can currently trigger the code path in get_warnoptions() that lazily creates the warnings list without adding the reference back into the config settings.
We'll need to fix that before we make the new configuration API public, but for now I'm just going to note it in the source code as a "PEP-432 TODO".
miss-islington commentedon Mar 25, 2018
New changeset c6d94c3 by Miss Islington (bot) in branch '3.7':
bpo-33042: Fix pre-initialization sys module configuration (GH-6157)
c6d94c3
ncoghlan commentedon Mar 25, 2018
While I've merged an initial fix for this (so it should be working again in 3.7.0b3), I'm not going to close the issue until folks have had a chance to review and comment on the linked list based approach I've taken.
ncoghlan commentedon Mar 25, 2018
New changeset bc77eff by Nick Coghlan in branch 'master':
bpo-33042: Fix pre-initialization sys module configuration (GH-6157)
bc77eff
vstinner commentedon Mar 25, 2018
Would it be possible to not officially allow calling PySys_AddWarnOption() before Py_Initialize()? Fix the bug, but don't promote doing that. I just suggest to revert:
Doc/c-api/init.rst:
+ * :c:func:`PySys_AddWarnOption`
+ * :c:func:`PySys_AddXOption`
+ * :c:func:`PySys_ResetWarnOptions`
htgoebel commentedon Mar 25, 2018
I can confirm that c6d94c3 makes the PyInstaller test-case, which war the origin of this bug-report, pass.
Thanks for fixing!
ncoghlan commentedon Mar 26, 2018
Harmut, thanks for the status update!
Victor, the problem is that both sys.warnoptions and sys._xoptions get read by Py_Initialize(), so setting them afterwards is essentially pointless - while you do still change the contents of the sys attributes, doing so doesn't have any further effect.
In 3.8, we'll hopefully make an updated version of PEP-432 public, and hence be able to deprecate calling them prior to Py_InitializeCore, and either advise folks to use the the new config structs instead, or else adjust the way that _PySys_BeginInit and _PySys_EndInit work so that the relevant sys module attributes get created in Py_InitializeCore rather than in Py_InitializeMainInterpreter.
ncoghlan commentedon Mar 26, 2018
Just noting that while I've closed this issue as fixed, I think we still have quite a bit of work to do to get our handling of these pre-init config settings to a place we're genuinely happy with (or as happy as backwards compatibility constraints permit).
However, the next step in that process is likely to involve at least myself, Victor & Eric getting together in person to discuss it at PyCon US (as I'm pretty sure we'll all be there this year), and deciding on how we'd like PEP-432 to look for Python 3.8.