From 255872a49bc4e60ab4978dd4f473695904bb0cac Mon Sep 17 00:00:00 2001 From: AN Long Date: Tue, 9 Jan 2024 23:58:26 +0800 Subject: [PATCH 1/6] 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 b7b16e22fa6419..b5f2fbf2386b42 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -789,6 +789,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', @@ -820,6 +833,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' @@ -860,6 +884,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 7fb1f2f561b0f9..e84a486bba1a05 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -781,12 +781,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)) { @@ -795,11 +940,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; } @@ -891,6 +1041,7 @@ getenvironment(PyObject* environment) cleanup: error: + Py_XDECREF(normalized_environment); Py_XDECREF(keys); Py_XDECREF(values); return buffer; From 03022e41d1a214067cca5e383ee80c6dc05ede10 Mon Sep 17 00:00:00 2001 From: AN Long Date: Thu, 11 Jan 2024 07:17:05 +0800 Subject: [PATCH 2/6] 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 b5f2fbf2386b42..f71f953fbd1797 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -833,6 +833,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', @@ -840,9 +845,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 From 97438d6f4d8933f2495dbc1375d986e99cc8f11f Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 11 Jan 2024 21:38:03 +0000 Subject: [PATCH 3/6] Update _winapi.c --- Modules/_winapi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index e84a486bba1a05..6a85c2a3a4bf41 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -35,6 +35,7 @@ /* See https://www.python.org/2.4/license for licensing details. */ #include "Python.h" +#include "pycore_global_strings.h" // _Py_ID(NAME) #include "pycore_moduleobject.h" // _PyModule_GetState() #include "structmember.h" // PyMemberDef From 3e664984b2554b2a8e9134dd56177870c1dd4f38 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 11 Jan 2024 21:47:24 +0000 Subject: [PATCH 4/6] Update _winapi.c --- Modules/_winapi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 6a85c2a3a4bf41..d3dba48d1e075a 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -35,7 +35,8 @@ /* See https://www.python.org/2.4/license for licensing details. */ #include "Python.h" -#include "pycore_global_strings.h" // _Py_ID(NAME) +#include "pycore_global_objects.h" // _Py_SINGLETON +#include "pycore_global_strings.h" // _Py_ID #include "pycore_moduleobject.h" // _PyModule_GetState() #include "structmember.h" // PyMemberDef From 3f85fa9e517a9293aeed05526306b38460a67d32 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 11 Jan 2024 21:52:35 +0000 Subject: [PATCH 5/6] Update _winapi.c --- Modules/_winapi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index d3dba48d1e075a..5195f4f69c2418 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -35,9 +35,8 @@ /* See https://www.python.org/2.4/license for licensing details. */ #include "Python.h" -#include "pycore_global_objects.h" // _Py_SINGLETON -#include "pycore_global_strings.h" // _Py_ID #include "pycore_moduleobject.h" // _PyModule_GetState() +#include "pycore_runtime.h" // _Py_ID #include "structmember.h" // PyMemberDef From 15a3a52fc30ac01989b31e6108801aea41ecb400 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 11 Jan 2024 22:13:14 +0000 Subject: [PATCH 6/6] Fix sortenvironmentkey --- Modules/_winapi.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 5195f4f69c2418..55b37188d24e45 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -36,15 +36,15 @@ #include "Python.h" #include "pycore_moduleobject.h" // _PyModule_GetState() -#include "pycore_runtime.h" // _Py_ID #include "structmember.h" // PyMemberDef -#define WINDOWS_LEAN_AND_MEAN #include "windows.h" #include #include "winreparse.h" +#include "pycore_runtime.h" // _Py_ID + #if defined(MS_WIN32) && !defined(MS_WIN64) #define HANDLE_TO_PYNUM(handle) \ PyLong_FromUnsignedLong((unsigned long) handle) @@ -785,8 +785,13 @@ gethandle(PyObject* obj, const char* name) static PyObject * sortenvironmentkey(PyObject *module, PyObject *item) { - return _winapi_LCMapStringEx_impl(NULL, LOCALE_NAME_INVARIANT, - LCMAP_UPPERCASE, item); + PyObject *result = NULL; + PyObject *locale = PyUnicode_FromWideChar(LOCALE_NAME_INVARIANT, -1); + if (locale) { + result = _winapi_LCMapStringEx_impl(NULL, locale, LCMAP_UPPERCASE, item); + Py_DECREF(locale); + } + return result; } static PyMethodDef sortenvironmentkey_def = {