From 235fd4b5faa5dec14beb8f8293faad052f940a7c Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Wed, 12 Oct 2022 16:01:59 +1000 Subject: [PATCH 1/5] gh-96853: Restore test coverage for `Py_Initialize(Ex)` * As most of `test_embed` now uses `Py_InitializeFromConfig`, add a specific test case to cover `Py_Initialize(Ex)` * Rename `_testembed` init helper to clarify the API used * Add a `PyConfig_Clear` call in `Py_InitializeEx` to make the code more obviously correct (it already didn't leak as none of the dynamically allocated config fields were being populated, but it's clearer if the wrappers follow the documented API usage guidelines) --- Lib/test/test_embed.py | 6 ++ ...2-10-12-14-57-06.gh-issue-96853.ANe-bw.rst | 3 + Programs/_testembed.c | 62 +++++++++++++------ Python/pylifecycle.c | 1 + 4 files changed, 53 insertions(+), 19 deletions(-) create mode 100644 Misc/NEWS.d/next/Tests/2022-10-12-14-57-06.gh-issue-96853.ANe-bw.rst diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index c5aeb9459848e4..13260cf5550710 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -340,6 +340,12 @@ def test_finalize_structseq(self): out, err = self.run_embedded_interpreter("test_repeated_init_exec", code) self.assertEqual(out, 'Tests passed\n' * INIT_LOOPS) + def test_simple_initialization_api(self): + # _testembed now uses Py_InitializeFromConfig by default + # This case specifically checks Py_Initialize(Ex) still works + out, err = self.run_embedded_interpreter("test_repeated_Py_Initialize") + self.assertEqual(out, 'Finalized\n' * INIT_LOOPS) + def test_quickened_static_code_gets_unquickened_at_Py_FINALIZE(self): # https://github.com/python/cpython/issues/92031 diff --git a/Misc/NEWS.d/next/Tests/2022-10-12-14-57-06.gh-issue-96853.ANe-bw.rst b/Misc/NEWS.d/next/Tests/2022-10-12-14-57-06.gh-issue-96853.ANe-bw.rst new file mode 100644 index 00000000000000..89958c57721913 --- /dev/null +++ b/Misc/NEWS.d/next/Tests/2022-10-12-14-57-06.gh-issue-96853.ANe-bw.rst @@ -0,0 +1,3 @@ +Added explicit coverage of ``Py_Initialize`` (and hence ``Py_InitializeEx``) +back to the embedding tests (all other embedding tests migrated to +``Py_InitializeFromConfig`` in Python 3.11) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index d635c5a4abe34a..4728f33e19c955 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -22,7 +22,7 @@ char **main_argv; /********************************************************* * Embedded interpreter tests that need a custom exe * - * Executed via 'EmbeddingTests' in Lib/test/test_capi.py + * Executed via Lib/test/test_embed.py *********************************************************/ // Use to display the usage @@ -73,7 +73,7 @@ static void init_from_config_clear(PyConfig *config) } -static void _testembed_Py_Initialize(void) +static void _testembed_Py_InitializeFromConfig(void) { PyConfig config; _PyConfig_InitCompatConfig(&config); @@ -81,6 +81,12 @@ static void _testembed_Py_Initialize(void) init_from_config_clear(&config); } +static void _testembed_Py_Initialize(void) +{ + Py_SetProgramName(PROGRAM_NAME); + Py_Initialize(); +} + /***************************************************** * Test repeated initialisation and subinterpreters @@ -110,7 +116,7 @@ static int test_repeated_init_and_subinterpreters(void) for (int i=1; i <= INIT_LOOPS; i++) { printf("--- Pass %d ---\n", i); - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); mainstate = PyThreadState_Get(); PyEval_ReleaseThread(mainstate); @@ -168,7 +174,7 @@ static int test_repeated_init_exec(void) fprintf(stderr, "--- Loop #%d ---\n", i); fflush(stderr); - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); int err = PyRun_SimpleString(code); Py_Finalize(); if (err) { @@ -178,6 +184,23 @@ static int test_repeated_init_exec(void) return 0; } +/**************************************************************************** + * Test the Py_Initialize(Ex) convenience/compatibility wrappers + ***************************************************************************/ +// This is here to ensure there are no wrapper resource leaks (gh-96853) +static int test_repeated_Py_Initialize(void) +{ + for (int i=1; i <= INIT_LOOPS; i++) { + fprintf(stderr, "--- Loop #%d ---\n", i); + fflush(stderr); + + _testembed_Py_Initialize(); + Py_Finalize(); + printf("Finalized\n"); // Give test_embed some output to check + } + return 0; +} + /***************************************************** * Test forcing a particular IO encoding @@ -199,7 +222,7 @@ static void check_stdio_details(const char *encoding, const char * errors) fflush(stdout); /* Force the given IO encoding */ Py_SetStandardStreamEncoding(encoding, errors); - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); PyRun_SimpleString( "import sys;" "print('stdin: {0.encoding}:{0.errors}'.format(sys.stdin));" @@ -308,7 +331,7 @@ static int test_pre_initialization_sys_options(void) dynamic_xoption = NULL; _Py_EMBED_PREINIT_CHECK("Initializing interpreter\n"); - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); _Py_EMBED_PREINIT_CHECK("Check sys module contents\n"); PyRun_SimpleString("import sys; " "print('sys.warnoptions:', sys.warnoptions); " @@ -352,7 +375,7 @@ static int test_bpo20891(void) return 1; } - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); unsigned long thrd = PyThread_start_new_thread(bpo20891_thread, &lock); if (thrd == PYTHREAD_INVALID_THREAD_ID) { @@ -375,7 +398,7 @@ static int test_bpo20891(void) static int test_initialize_twice(void) { - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); /* bpo-33932: Calling Py_Initialize() twice should do nothing * (and not crash!). */ @@ -393,7 +416,7 @@ static int test_initialize_pymain(void) L"print(f'Py_Main() after Py_Initialize: " L"sys.argv={sys.argv}')"), L"arg2"}; - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); /* bpo-34008: Calling Py_Main() after Py_Initialize() must not crash */ Py_Main(Py_ARRAY_LENGTH(argv), argv); @@ -416,7 +439,7 @@ dump_config(void) static int test_init_initialize_config(void) { - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); dump_config(); Py_Finalize(); return 0; @@ -769,7 +792,7 @@ static int test_init_compat_env(void) /* Test initialization from environment variables */ Py_IgnoreEnvironmentFlag = 0; set_all_env_vars(); - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); dump_config(); Py_Finalize(); return 0; @@ -805,7 +828,7 @@ static int test_init_env_dev_mode(void) /* Test initialization from environment variables */ Py_IgnoreEnvironmentFlag = 0; set_all_env_vars_dev_mode(); - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); dump_config(); Py_Finalize(); return 0; @@ -818,7 +841,7 @@ static int test_init_env_dev_mode_alloc(void) Py_IgnoreEnvironmentFlag = 0; set_all_env_vars_dev_mode(); putenv("PYTHONMALLOC=malloc"); - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); dump_config(); Py_Finalize(); return 0; @@ -1158,7 +1181,7 @@ static int test_open_code_hook(void) } Py_IgnoreEnvironmentFlag = 0; - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); result = 0; PyObject *r = PyFile_OpenCode("$$test-filename"); @@ -1222,7 +1245,7 @@ static int _test_audit(Py_ssize_t setValue) Py_IgnoreEnvironmentFlag = 0; PySys_AddAuditHook(_audit_hook, &sawSet); - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); if (PySys_Audit("_testembed.raise", NULL) == 0) { printf("No error raised"); @@ -1278,7 +1301,7 @@ static int test_audit_subinterpreter(void) { Py_IgnoreEnvironmentFlag = 0; PySys_AddAuditHook(_audit_subinterpreter_hook, NULL); - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); Py_NewInterpreter(); Py_NewInterpreter(); @@ -1873,13 +1896,13 @@ static int test_unicode_id_init(void) _Py_IDENTIFIER(test_unicode_id_init); // Initialize Python once without using the identifier - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); Py_Finalize(); // Now initialize Python multiple times and use the identifier. // The first _PyUnicode_FromId() call initializes the identifier index. for (int i=0; i<3; i++) { - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); PyObject *str1, *str2; @@ -2011,7 +2034,7 @@ unwrap_allocator(PyMemAllocatorEx *allocator) static int test_get_incomplete_frame(void) { - _testembed_Py_Initialize(); + _testembed_Py_InitializeFromConfig(); PyMemAllocatorEx allocator; wrap_allocator(&allocator); // Force an allocation with an incomplete (generator) frame: @@ -2043,6 +2066,7 @@ struct TestCase static struct TestCase TestCases[] = { // Python initialization {"test_repeated_init_exec", test_repeated_init_exec}, + {"test_repeated_Py_Initialize", test_repeated_Py_Initialize}, {"test_forced_io_encoding", test_forced_io_encoding}, {"test_repeated_init_and_subinterpreters", test_repeated_init_and_subinterpreters}, {"test_repeated_init_and_inittab", test_repeated_init_and_inittab}, diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index c550f13bc14820..c911be836d1d69 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1292,6 +1292,7 @@ Py_InitializeEx(int install_sigs) config.install_signal_handlers = install_sigs; status = Py_InitializeFromConfig(&config); + PyConfig_Clear(&config); if (_PyStatus_EXCEPTION(status)) { Py_ExitStatusException(status); } From d1f703011239ac243734de2562d042090b637270 Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Wed, 12 Oct 2022 16:08:46 +1000 Subject: [PATCH 2/5] Tweak comment --- Programs/_testembed.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 4728f33e19c955..815eaa48aa8897 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -187,7 +187,7 @@ static int test_repeated_init_exec(void) /**************************************************************************** * Test the Py_Initialize(Ex) convenience/compatibility wrappers ***************************************************************************/ -// This is here to ensure there are no wrapper resource leaks (gh-96853) +// This is here to help ensure there are no wrapper resource leaks (gh-96853) static int test_repeated_Py_Initialize(void) { for (int i=1; i <= INIT_LOOPS; i++) { From 709908f78666f4f0be6f059c2f367390ea98bf34 Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sun, 16 Oct 2022 15:00:52 +1000 Subject: [PATCH 3/5] Add release note for the C API fix --- .../next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst diff --git a/Misc/NEWS.d/next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst b/Misc/NEWS.d/next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst new file mode 100644 index 00000000000000..8c63f9d893bb60 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst @@ -0,0 +1,4 @@ +`Py_InitializeEx` now correctly calls `PyConfig_Clear` after initializing +the interpreter (the omission didn't cause a memory leak only because none +of the dynamically allocated config fields are populated by the wrapper +function) From e4d88c0b3f4620faabdbc3d4cfe04997e9391b5f Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sun, 16 Oct 2022 15:11:27 +1000 Subject: [PATCH 4/5] Keep consistent naming style for embedding test helpers --- Lib/test/test_embed.py | 2 +- Programs/_testembed.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 13260cf5550710..7e19ce2e29f415 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -343,7 +343,7 @@ def test_finalize_structseq(self): def test_simple_initialization_api(self): # _testembed now uses Py_InitializeFromConfig by default # This case specifically checks Py_Initialize(Ex) still works - out, err = self.run_embedded_interpreter("test_repeated_Py_Initialize") + out, err = self.run_embedded_interpreter("test_repeated_simple_init") self.assertEqual(out, 'Finalized\n' * INIT_LOOPS) def test_quickened_static_code_gets_unquickened_at_Py_FINALIZE(self): diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 815eaa48aa8897..362570b2f76fa8 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -188,7 +188,7 @@ static int test_repeated_init_exec(void) * Test the Py_Initialize(Ex) convenience/compatibility wrappers ***************************************************************************/ // This is here to help ensure there are no wrapper resource leaks (gh-96853) -static int test_repeated_Py_Initialize(void) +static int test_repeated_simple_init(void) { for (int i=1; i <= INIT_LOOPS; i++) { fprintf(stderr, "--- Loop #%d ---\n", i); @@ -2066,7 +2066,7 @@ struct TestCase static struct TestCase TestCases[] = { // Python initialization {"test_repeated_init_exec", test_repeated_init_exec}, - {"test_repeated_Py_Initialize", test_repeated_Py_Initialize}, + {"test_repeated_simple_init", test_repeated_simple_init}, {"test_forced_io_encoding", test_forced_io_encoding}, {"test_repeated_init_and_subinterpreters", test_repeated_init_and_subinterpreters}, {"test_repeated_init_and_inittab", test_repeated_init_and_inittab}, From 12e10d45f01996526ba9afd8b2dcb46ef4d4bcee Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sun, 16 Oct 2022 15:18:59 +1000 Subject: [PATCH 5/5] Fix markup --- .../next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst b/Misc/NEWS.d/next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst index 8c63f9d893bb60..d7e3cc423ac895 100644 --- a/Misc/NEWS.d/next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst +++ b/Misc/NEWS.d/next/C API/2022-10-16-15-00-25.gh-issue-96853.V0wiXP.rst @@ -1,4 +1,4 @@ -`Py_InitializeEx` now correctly calls `PyConfig_Clear` after initializing +``Py_InitializeEx`` now correctly calls ``PyConfig_Clear`` after initializing the interpreter (the omission didn't cause a memory leak only because none of the dynamically allocated config fields are populated by the wrapper function)