From 12db2cd7f6bf05961625d656fc45d22ccaa1fda6 Mon Sep 17 00:00:00 2001 From: AN Long Date: Tue, 9 Jan 2024 23:58:26 +0800 Subject: [PATCH 1/2] gh-87868: Sort and remove duplicates in getenvironment() (GH-102731) (cherry picked from commit c31be58da8577ef140e83d4e46502c7bb1eb9abf) Co-authored-by: AN Long Co-authored-by: Alex Waygood Co-authored-by: Pieter Eendebak Co-authored-by: Erlend E. Aasland --- Lib/test/test_subprocess.py | 37 ++++ ...3-03-15-23-53-45.gh-issue-87868.4C36oQ.rst | 2 + Modules/_winapi.c | 159 +++++++++++++++++- 3 files changed, 194 insertions(+), 4 deletions(-) create mode 100644 Misc/NEWS.d/next/Windows/2023-03-15-23-53-45.gh-issue-87868.4C36oQ.rst diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 79497f9301557e..3dd6caadfc9560 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -791,6 +791,19 @@ def test_env(self): stdout, stderr = p.communicate() self.assertEqual(stdout, b"orange") + @unittest.skipUnless(sys.platform == "win32", "Windows only issue") + def test_win32_duplicate_envs(self): + newenv = os.environ.copy() + newenv["fRUit"] = "cherry" + newenv["fruit"] = "lemon" + newenv["FRUIT"] = "orange" + newenv["frUit"] = "banana" + with subprocess.Popen(["CMD", "/c", "SET", "fruit"], + stdout=subprocess.PIPE, + env=newenv) as p: + stdout, _ = p.communicate() + self.assertEqual(stdout.strip(), b"frUit=banana") + # Windows requires at least the SYSTEMROOT environment variable to start # Python @unittest.skipIf(sys.platform == 'win32', @@ -822,6 +835,17 @@ def is_env_var_to_ignore(n): if not is_env_var_to_ignore(k)] self.assertEqual(child_env_names, []) + def test_one_environment_variable(self): + newenv = {'fruit': 'orange'} + cmd = [sys.executable, '-c', + 'import sys,os;' + 'sys.stdout.write("fruit="+os.getenv("fruit"))'] + if sys.platform == "win32": + cmd = ["CMD", "/c", "SET", "fruit"] + with subprocess.Popen(cmd, stdout=subprocess.PIPE, env=newenv) as p: + stdout, _ = p.communicate() + self.assertTrue(stdout.startswith(b"fruit=orange")) + def test_invalid_cmd(self): # null character in the command name cmd = sys.executable + '\0' @@ -862,6 +886,19 @@ def test_invalid_env(self): stdout, stderr = p.communicate() self.assertEqual(stdout, b"orange=lemon") + @unittest.skipUnless(sys.platform == "win32", "Windows only issue") + def test_win32_invalid_env(self): + # '=' in the environment variable name + newenv = os.environ.copy() + newenv["FRUIT=VEGETABLE"] = "cabbage" + with self.assertRaises(ValueError): + subprocess.Popen(ZERO_RETURN_CMD, env=newenv) + + newenv = os.environ.copy() + newenv["==FRUIT"] = "cabbage" + with self.assertRaises(ValueError): + subprocess.Popen(ZERO_RETURN_CMD, env=newenv) + def test_communicate_stdin(self): p = subprocess.Popen([sys.executable, "-c", 'import sys;' diff --git a/Misc/NEWS.d/next/Windows/2023-03-15-23-53-45.gh-issue-87868.4C36oQ.rst b/Misc/NEWS.d/next/Windows/2023-03-15-23-53-45.gh-issue-87868.4C36oQ.rst new file mode 100644 index 00000000000000..37e8103c9ec34b --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2023-03-15-23-53-45.gh-issue-87868.4C36oQ.rst @@ -0,0 +1,2 @@ +Correctly sort and remove duplicate environment variables in +:py:func:`!_winapi.CreateProcess`. diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 77275408aed868..fdbe3f626fd2a7 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -772,12 +772,157 @@ gethandle(PyObject* obj, const char* name) return ret; } +static PyObject * +sortenvironmentkey(PyObject *module, PyObject *item) +{ + return _winapi_LCMapStringEx_impl(NULL, LOCALE_NAME_INVARIANT, + LCMAP_UPPERCASE, item); +} + +static PyMethodDef sortenvironmentkey_def = { + "sortenvironmentkey", _PyCFunction_CAST(sortenvironmentkey), METH_O, "", +}; + +static int +sort_environment_keys(PyObject *keys) +{ + PyObject *keyfunc = PyCFunction_New(&sortenvironmentkey_def, NULL); + if (keyfunc == NULL) { + return -1; + } + PyObject *kwnames = Py_BuildValue("(s)", "key"); + if (kwnames == NULL) { + Py_DECREF(keyfunc); + return -1; + } + PyObject *args[] = { keys, keyfunc }; + PyObject *ret = PyObject_VectorcallMethod(&_Py_ID(sort), args, 1, kwnames); + Py_DECREF(keyfunc); + Py_DECREF(kwnames); + if (ret == NULL) { + return -1; + } + Py_DECREF(ret); + + return 0; +} + +static int +compare_string_ordinal(PyObject *str1, PyObject *str2, int *result) +{ + wchar_t *s1 = PyUnicode_AsWideCharString(str1, NULL); + if (s1 == NULL) { + return -1; + } + wchar_t *s2 = PyUnicode_AsWideCharString(str2, NULL); + if (s2 == NULL) { + PyMem_Free(s1); + return -1; + } + *result = CompareStringOrdinal(s1, -1, s2, -1, TRUE); + PyMem_Free(s1); + PyMem_Free(s2); + return 0; +} + +static PyObject * +dedup_environment_keys(PyObject *keys) +{ + PyObject *result = PyList_New(0); + if (result == NULL) { + return NULL; + } + + // Iterate over the pre-ordered keys, check whether the current key is equal + // to the next key (ignoring case), if different, insert the current value + // into the result list. If they are equal, do nothing because we always + // want to keep the last inserted one. + for (Py_ssize_t i = 0; i < PyList_GET_SIZE(keys); i++) { + PyObject *key = PyList_GET_ITEM(keys, i); + + // The last key will always be kept. + if (i + 1 == PyList_GET_SIZE(keys)) { + if (PyList_Append(result, key) < 0) { + Py_DECREF(result); + return NULL; + } + continue; + } + + PyObject *next_key = PyList_GET_ITEM(keys, i + 1); + int compare_result; + if (compare_string_ordinal(key, next_key, &compare_result) < 0) { + Py_DECREF(result); + return NULL; + } + if (compare_result == CSTR_EQUAL) { + continue; + } + if (PyList_Append(result, key) < 0) { + Py_DECREF(result); + return NULL; + } + } + + return result; +} + +static PyObject * +normalize_environment(PyObject *environment) +{ + PyObject *keys = PyMapping_Keys(environment); + if (keys == NULL) { + return NULL; + } + + if (sort_environment_keys(keys) < 0) { + Py_DECREF(keys); + return NULL; + } + + PyObject *normalized_keys = dedup_environment_keys(keys); + Py_DECREF(keys); + if (normalized_keys == NULL) { + return NULL; + } + + PyObject *result = PyDict_New(); + if (result == NULL) { + Py_DECREF(normalized_keys); + return NULL; + } + + for (int i = 0; i < PyList_GET_SIZE(normalized_keys); i++) { + PyObject *key = PyList_GET_ITEM(normalized_keys, i); + PyObject *value = PyObject_GetItem(environment, key); + if (value == NULL) { + Py_DECREF(normalized_keys); + Py_DECREF(result); + return NULL; + } + + int ret = PyObject_SetItem(result, key, value); + Py_DECREF(value); + if (ret < 0) { + Py_DECREF(normalized_keys); + Py_DECREF(result); + return NULL; + } + } + + Py_DECREF(normalized_keys); + + return result; +} + static wchar_t * getenvironment(PyObject* environment) { Py_ssize_t i, envsize, totalsize; wchar_t *buffer = NULL, *p, *end; - PyObject *keys, *values; + PyObject *normalized_environment = NULL; + PyObject *keys = NULL; + PyObject *values = NULL; /* convert environment dictionary to windows environment string */ if (! PyMapping_Check(environment)) { @@ -786,11 +931,16 @@ getenvironment(PyObject* environment) return NULL; } - keys = PyMapping_Keys(environment); - if (!keys) { + normalized_environment = normalize_environment(environment); + if (normalize_environment == NULL) { return NULL; } - values = PyMapping_Values(environment); + + keys = PyMapping_Keys(normalized_environment); + if (!keys) { + goto error; + } + values = PyMapping_Values(normalized_environment); if (!values) { goto error; } @@ -882,6 +1032,7 @@ getenvironment(PyObject* environment) cleanup: error: + Py_XDECREF(normalized_environment); Py_XDECREF(keys); Py_XDECREF(values); return buffer; From 84a5a59c154c44c1a097ed5a80718f21d3ebba71 Mon Sep 17 00:00:00 2001 From: AN Long Date: Thu, 11 Jan 2024 07:17:05 +0800 Subject: [PATCH 2/2] gh-87868: Skip `test_one_environment_variable` in `test_subprocess` when the platform or build cannot do that (#113867) * improve the assert for test_one_environment_variable * skip some test in test_subprocess when python is configured with shared * also skip the test if AddressSanitizer is enabled --------- Co-authored-by: Steve Dower --- Lib/test/test_subprocess.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 3dd6caadfc9560..9ade90f741bc6f 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -835,6 +835,11 @@ def is_env_var_to_ignore(n): if not is_env_var_to_ignore(k)] self.assertEqual(child_env_names, []) + @unittest.skipIf(sysconfig.get_config_var('Py_ENABLE_SHARED') == 1, + 'The Python shared library cannot be loaded ' + 'without some system environments.') + @unittest.skipIf(check_sanitizer(address=True), + 'AddressSanitizer adds to the environment.') def test_one_environment_variable(self): newenv = {'fruit': 'orange'} cmd = [sys.executable, '-c', @@ -842,9 +847,13 @@ def test_one_environment_variable(self): 'sys.stdout.write("fruit="+os.getenv("fruit"))'] if sys.platform == "win32": cmd = ["CMD", "/c", "SET", "fruit"] - with subprocess.Popen(cmd, stdout=subprocess.PIPE, env=newenv) as p: - stdout, _ = p.communicate() - self.assertTrue(stdout.startswith(b"fruit=orange")) + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=newenv) as p: + stdout, stderr = p.communicate() + if p.returncode and support.verbose: + print("STDOUT:", stdout.decode("ascii", "replace")) + print("STDERR:", stderr.decode("ascii", "replace")) + self.assertEqual(p.returncode, 0) + self.assertEqual(stdout.strip(), b"fruit=orange") def test_invalid_cmd(self): # null character in the command name