bpo-33042: Fix pre-initialization sys module configuration (pythonGH-6157)

ncoghlan · web-flow · commit bc77eff8b96b · 2018-03-25T20:44:30.000+10:00
- new test case for pre-initialization of sys.warnoptions and sys._xoptions
- restored ability to call these APIs prior to Py_Initialize
- updated the docs for the affected APIs to make it clear they can be
  called before Py_Initialize
- also enhanced the existing embedding test cases
  to check for expected settings in the sys module
diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst
@@ -31,6 +31,9 @@ The following functions can be safely called before Python is initialized:
   * :c:func:`Py_SetProgramName`
   * :c:func:`Py_SetPythonHome`
   * :c:func:`Py_SetStandardStreamEncoding`
+  * :c:func:`PySys_AddWarnOption`
+  * :c:func:`PySys_AddXOption`
+  * :c:func:`PySys_ResetWarnOptions`
 
 * Informative functions:
 
diff --git a/Doc/c-api/sys.rst b/Doc/c-api/sys.rst
@@ -205,16 +205,24 @@ accessible to C code.  They all work with the current interpreter thread's
 
 .. c:function:: void PySys_ResetWarnOptions()
 
-   Reset :data:`sys.warnoptions` to an empty list.
+   Reset :data:`sys.warnoptions` to an empty list. This function may be
+   called prior to :c:func:`Py_Initialize`.
 
 .. c:function:: void PySys_AddWarnOption(const wchar_t *s)
 
-   Append *s* to :data:`sys.warnoptions`.
+   Append *s* to :data:`sys.warnoptions`. This function must be called prior
+   to :c:func:`Py_Initialize` in order to affect the warnings filter list.
 
 .. c:function:: void PySys_AddWarnOptionUnicode(PyObject *unicode)
 
    Append *unicode* to :data:`sys.warnoptions`.
 
+   Note: this function is not currently usable from outside the CPython
+   implementation, as it must be called prior to the implicit import of
+   :mod:`warnings` in :c:func:`Py_Initialize` to be effective, but can't be
+   called until enough of the runtime has been initialized to permit the
+   creation of Unicode objects.
+
 .. c:function:: void PySys_SetPath(const wchar_t *path)
 
    Set :data:`sys.path` to a list object of paths found in *path* which should
@@ -260,7 +268,8 @@ accessible to C code.  They all work with the current interpreter thread's
 .. c:function:: void PySys_AddXOption(const wchar_t *s)
 
    Parse *s* as a set of :option:`-X` options and add them to the current
-   options mapping as returned by :c:func:`PySys_GetXOptions`.
+   options mapping as returned by :c:func:`PySys_GetXOptions`. This function
+   may be called prior to :c:func:`Py_Initialize`.
 
    .. versionadded:: 3.2
 
diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst
@@ -951,6 +951,14 @@ Build and C API Changes
   second argument is *NULL* and the :c:type:`wchar_t*` string contains null
   characters.  (Contributed by Serhiy Storchaka in :issue:`30708`.)
 
+- Changes to the startup sequence and the management of dynamic memory
+  allocators mean that the long documented requirement to call
+  :c:func:`Py_Initialize` before calling most C API functions is now
+  relied on more heavily, and failing to abide by it may lead to segfaults in
+  embedding applications. See the :ref:`porting-to-python-37` section in this
+  document and the :ref:`pre-init-safe` section in the C API documentation
+  for more details.
+
 
 Other CPython Implementation Changes
 ====================================
@@ -1098,6 +1106,7 @@ API and Feature Removals
   ``asyncio._overlapped``. Replace ``from asyncio import selectors`` with
   ``import selectors`` for example.
 
+.. _porting-to-python-37:
 
 Porting to Python 3.7
 =====================
@@ -1282,14 +1291,24 @@ Other CPython implementation changes
 ------------------------------------
 
 * In preparation for potential future changes to the public CPython runtime
-  initialization API (see :pep:`432` for details), CPython's internal startup
+  initialization API (see :pep:`432` for an initial, but somewhat outdated,
+  draft), CPython's internal startup
   and configuration management logic has been significantly refactored. While
   these updates are intended to be entirely transparent to both embedding
   applications and users of the regular CPython CLI, they're being mentioned
   here as the refactoring changes the internal order of various operations
   during interpreter startup, and hence may uncover previously latent defects,
   either in embedding applications, or in CPython itself.
-  (Contributed by Nick Coghlan and Eric Snow as part of :issue:`22257`.)
+  (Initially contributed by Nick Coghlan and Eric Snow as part of
+  :issue:`22257`, and further updated by Nick, Eric, and Victor Stinner in a
+  number of other issues). Some known details affected:
+
+    * :c:func:`PySys_AddWarnOptionUnicode` is not currently usable by embedding
+      applications due to the requirement to create a Unicode object prior to
+      calling `Py_Initialize`. Use :c:func:`PySys_AddWarnOption` instead.
+    * warnings filters added by an embedding application with
+      :c:func:`PySys_AddWarnOption` should now more consistently take precedence
+      over the default filters set by the interpreter
 
 * Due to changes in the way the default warnings filters are configured,
   setting :c:data:`Py_BytesWarningFlag` to a value greater than one is no longer
diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py
@@ -51,7 +51,7 @@ def run_embedded_interpreter(self, *args, env=None):
         if p.returncode != 0 and support.verbose:
             print(f"--- {cmd} failed ---")
             print(f"stdout:\n{out}")
-            print(f"stderr:\n{out}")
+            print(f"stderr:\n{err}")
             print(f"------")
 
         self.assertEqual(p.returncode, 0,
@@ -83,7 +83,7 @@ def run_repeated_init_and_subinterpreters(self):
         for line in out.splitlines():
             if line == "--- Pass {} ---".format(numloops):
                 self.assertEqual(len(current_run), 0)
-                if support.verbose:
+                if support.verbose > 1:
                     print(line)
                 numloops += 1
                 continue
@@ -96,7 +96,7 @@ def run_repeated_init_and_subinterpreters(self):
             # Parse the line from the loop.  The first line is the main
             # interpreter and the 3 afterward are subinterpreters.
             interp = Interp(*match.groups())
-            if support.verbose:
+            if support.verbose > 1:
                 print(interp)
             self.assertTrue(interp.interp)
             self.assertTrue(interp.tstate)
@@ -190,12 +190,33 @@ def test_forced_io_encoding(self):
 
     def test_pre_initialization_api(self):
         """
-        Checks the few parts of the C-API that work before the runtine
+        Checks some key parts of the C-API that need to work before the runtine
         is initialized (via Py_Initialize()).
         """
         env = dict(os.environ, PYTHONPATH=os.pathsep.join(sys.path))
         out, err = self.run_embedded_interpreter("pre_initialization_api", env=env)
-        self.assertEqual(out, '')
+        if sys.platform == "win32":
+            expected_path = self.test_exe
+        else:
+            expected_path = os.path.join(os.getcwd(), "spam")
+        expected_output = f"sys.executable: {expected_path}\n"
+        self.assertIn(expected_output, out)
+        self.assertEqual(err, '')
+
+    def test_pre_initialization_sys_options(self):
+        """
+        Checks that sys.warnoptions and sys._xoptions can be set before the
+        runtime is initialized (otherwise they won't be effective).
+        """
+        env = dict(PYTHONPATH=os.pathsep.join(sys.path))
+        out, err = self.run_embedded_interpreter(
+                        "pre_initialization_sys_options", env=env)
+        expected_output = (
+            "sys.warnoptions: ['once', 'module', 'default']\n"
+            "sys._xoptions: {'not_an_option': '1', 'also_not_an_option': '2'}\n"
+            "warnings.filters[:3]: ['default', 'module', 'once']\n"
+        )
+        self.assertIn(expected_output, out)
         self.assertEqual(err, '')
 
     def test_bpo20891(self):
diff --git a/Misc/NEWS.d/next/C API/2018-03-20-21-43-09.bpo-33042.FPFp64.rst b/Misc/NEWS.d/next/C API/2018-03-20-21-43-09.bpo-33042.FPFp64.rst
@@ -0,0 +1,2 @@
+Embedding applications may once again call PySys_ResetWarnOptions,
+PySys_AddWarnOption, and PySys_AddXOption prior to calling Py_Initialize.
diff --git a/Programs/_testembed.c b/Programs/_testembed.c
@@ -2,6 +2,7 @@
 #include "pythread.h"
 #include <inttypes.h>
 #include <stdio.h>
+#include <wchar.h>
 
 /*********************************************************
  * Embedded interpreter tests that need a custom exe
@@ -130,23 +131,89 @@ static int test_forced_io_encoding(void)
  * Test parts of the C-API that work before initialization
  *********************************************************/
 
+/* The pre-initialization tests tend to break by segfaulting, so explicitly
+ * flushed progress messages make the broken API easier to find when they fail.
+ */
+#define _Py_EMBED_PREINIT_CHECK(msg) \
+    do {printf(msg); fflush(stdout);} while (0);
+
 static int test_pre_initialization_api(void)
 {
     /* Leading "./" ensures getpath.c can still find the standard library */
+    _Py_EMBED_PREINIT_CHECK("Checking Py_DecodeLocale\n");
     wchar_t *program = Py_DecodeLocale("./spam", NULL);
     if (program == NULL) {
         fprintf(stderr, "Fatal error: cannot decode program name\n");
         return 1;
     }
+    _Py_EMBED_PREINIT_CHECK("Checking Py_SetProgramName\n");
     Py_SetProgramName(program);
 
+    _Py_EMBED_PREINIT_CHECK("Initializing interpreter\n");
     Py_Initialize();
+    _Py_EMBED_PREINIT_CHECK("Check sys module contents\n");
+    PyRun_SimpleString("import sys; "
+                       "print('sys.executable:', sys.executable)");
+    _Py_EMBED_PREINIT_CHECK("Finalizing interpreter\n");
     Py_Finalize();
 
+    _Py_EMBED_PREINIT_CHECK("Freeing memory allocated by Py_DecodeLocale\n");
     PyMem_RawFree(program);
     return 0;
 }
 
+
+/* bpo-33042: Ensure embedding apps can predefine sys module options */
+static int test_pre_initialization_sys_options(void)
+{
+    /* We allocate a couple of the option dynamically, and then delete
+     * them before calling Py_Initialize. This ensures the interpreter isn't
+     * relying on the caller to keep the passed in strings alive.
+     */
+    wchar_t *static_warnoption = L"once";
+    wchar_t *static_xoption = L"also_not_an_option=2";
+    size_t warnoption_len = wcslen(static_warnoption);
+    size_t xoption_len = wcslen(static_xoption);
+    wchar_t *dynamic_once_warnoption = calloc(warnoption_len+1, sizeof(wchar_t));
+    wchar_t *dynamic_xoption = calloc(xoption_len+1, sizeof(wchar_t));
+    wcsncpy(dynamic_once_warnoption, static_warnoption, warnoption_len+1);
+    wcsncpy(dynamic_xoption, static_xoption, xoption_len+1);
+
+    _Py_EMBED_PREINIT_CHECK("Checking PySys_AddWarnOption\n");
+    PySys_AddWarnOption(L"default");
+    _Py_EMBED_PREINIT_CHECK("Checking PySys_ResetWarnOptions\n");
+    PySys_ResetWarnOptions();
+    _Py_EMBED_PREINIT_CHECK("Checking PySys_AddWarnOption linked list\n");
+    PySys_AddWarnOption(dynamic_once_warnoption);
+    PySys_AddWarnOption(L"module");
+    PySys_AddWarnOption(L"default");
+    _Py_EMBED_PREINIT_CHECK("Checking PySys_AddXOption\n");
+    PySys_AddXOption(L"not_an_option=1");
+    PySys_AddXOption(dynamic_xoption);
+
+    /* Delete the dynamic options early */
+    free(dynamic_once_warnoption);
+    dynamic_once_warnoption = NULL;
+    free(dynamic_xoption);
+    dynamic_xoption = NULL;
+
+    _Py_EMBED_PREINIT_CHECK("Initializing interpreter\n");
+    _testembed_Py_Initialize();
+    _Py_EMBED_PREINIT_CHECK("Check sys module contents\n");
+    PyRun_SimpleString("import sys; "
+                       "print('sys.warnoptions:', sys.warnoptions); "
+                       "print('sys._xoptions:', sys._xoptions); "
+                       "warnings = sys.modules['warnings']; "
+                       "latest_filters = [f[0] for f in warnings.filters[:3]]; "
+                       "print('warnings.filters[:3]:', latest_filters)");
+    _Py_EMBED_PREINIT_CHECK("Finalizing interpreter\n");
+    Py_Finalize();
+
+    return 0;
+}
+
+
+/* bpo-20891: Avoid race condition when initialising the GIL */
 static void bpo20891_thread(void *lockp)
 {
     PyThread_type_lock lock = *((PyThread_type_lock*)lockp);
@@ -217,6 +284,7 @@ static struct TestCase TestCases[] = {
     { "forced_io_encoding", test_forced_io_encoding },
     { "repeated_init_and_subinterpreters", test_repeated_init_and_subinterpreters },
     { "pre_initialization_api", test_pre_initialization_api },
+    { "pre_initialization_sys_options", test_pre_initialization_sys_options },
     { "bpo20891", test_bpo20891 },
     { NULL, NULL }
 };
@@ -232,13 +300,13 @@ int main(int argc, char *argv[])
 
     /* No match found, or no test name provided, so display usage */
     printf("Python " PY_VERSION " _testembed executable for embedded interpreter tests\n"
-           "Normally executed via 'EmbeddingTests' in Lib/test/test_capi.py\n\n"
+           "Normally executed via 'EmbeddingTests' in Lib/test/test_embed.py\n\n"
            "Usage: %s TESTNAME\n\nAll available tests:\n", argv[0]);
     for (struct TestCase *tc = TestCases; tc && tc->name; tc++) {
         printf("  %s\n", tc->name);
     }
 
-    /* Non-zero exit code will cause test_capi.py tests to fail.
+    /* Non-zero exit code will cause test_embed.py tests to fail.
        This is intentional. */
     return -1;
 }
diff --git a/Python/sysmodule.c b/Python/sysmodule.c

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+Embedding applications may once again call PySys_ResetWarnOptions,`
	`2`	`+PySys_AddWarnOption, and PySys_AddXOption prior to calling Py_Initialize.`