From 9eef029611844b4ba92d9f614d8ea5e15cf428f2 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 24 Jan 2020 10:09:13 +0100 Subject: [PATCH 1/3] bpo-39413: Implement os.unsetenv() on Windows The os.unsetenv() function is now also available on Windows. --- Doc/library/os.rst | 3 + Doc/whatsnew/3.9.rst | 3 + Lib/test/test_os.py | 42 +++++-- .../2020-01-24-10-10-25.bpo-39413.7XYDM8.rst | 1 + Modules/clinic/posixmodule.c.h | 42 ++++++- Modules/posixmodule.c | 107 +++++++++++------- 6 files changed, 149 insertions(+), 49 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2020-01-24-10-10-25.bpo-39413.7XYDM8.rst diff --git a/Doc/library/os.rst b/Doc/library/os.rst index 4fec647828e250..de3e5603e109fe 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -645,6 +645,9 @@ process and user. .. availability:: most flavors of Unix, Windows. + .. versionchanged:: 3.9 + The function is now also available on Windows. + .. _os-newstreams: diff --git a/Doc/whatsnew/3.9.rst b/Doc/whatsnew/3.9.rst index a6e938faa991e0..33922859c9ecbb 100644 --- a/Doc/whatsnew/3.9.rst +++ b/Doc/whatsnew/3.9.rst @@ -223,6 +223,9 @@ Exposed the Linux-specific :func:`os.pidfd_open` (:issue:`38692`) and :data:`os.P_PIDFD` (:issue:`38713`) for process management with file descriptors. +The :func:`os.unsetenv` function is now also available on Windows. +(Contributed by Victor Stinner in :issue:`39413`.) + poplib ------ diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 82c441c204835a..60134591926c98 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -953,17 +953,43 @@ def test_environb(self): value_str = value.decode(sys.getfilesystemencoding(), 'surrogateescape') self.assertEqual(os.environ['bytes'], value_str) + @unittest.skipUnless(hasattr(os, 'putenv'), "Test needs os.putenv()") + @unittest.skipUnless(hasattr(os, 'unsetenv'), "Test needs os.unsetenv()") + def test_putenv_unsetenv(self): + name = "PYTHONTESTVAR" + value = "testvalue" + code = f'import os; print(repr(os.environ.get({name!r})))' + + with support.EnvironmentVarGuard() as env: + env.pop(name, None) + + os.putenv(name, value) + proc = subprocess.run([sys.executable, '-c', code], check=True, + stdout=subprocess.PIPE, text=True) + self.assertEqual(proc.stdout.rstrip(), repr(value)) + + os.unsetenv(name) + proc = subprocess.run([sys.executable, '-c', code], check=True, + stdout=subprocess.PIPE, text=True) + self.assertEqual(proc.stdout.rstrip(), repr(None)) + # On OS X < 10.6, unsetenv() doesn't return a value (bpo-13415). @support.requires_mac_ver(10, 6) - def test_unset_error(self): + @unittest.skipUnless(hasattr(os, 'putenv'), "Test needs os.putenv()") + @unittest.skipUnless(hasattr(os, 'unsetenv'), "Test needs os.unsetenv()") + def test_putenv_unsetenv_error(self): + # "=" and null character are not allowed in a variable name + for name in ('=name', 'na=me', 'name=', 'name\0', 'na\0me'): + self.assertRaises((OSError, ValueError), os.putenv, name, "value") + self.assertRaises((OSError, ValueError), os.unsetenv, name) + if sys.platform == "win32": - # an environment variable is limited to 32,767 characters - key = 'x' * 50000 - self.assertRaises(ValueError, os.environ.__delitem__, key) - else: - # "=" is not allowed in a variable name - key = 'key=' - self.assertRaises(OSError, os.environ.__delitem__, key) + # On Windows, an environment variable string ("name=value" string) + # is limited to 32,767 characters + longstr = 'x' * 32_768 + self.assertRaises(ValueError, os.putenv, longstr, "1") + self.assertRaises(ValueError, os.putenv, "X", longstr) + self.assertRaises(ValueError, os.unsetenv, longstr) def test_key_type(self): missing = 'missingkey' diff --git a/Misc/NEWS.d/next/Library/2020-01-24-10-10-25.bpo-39413.7XYDM8.rst b/Misc/NEWS.d/next/Library/2020-01-24-10-10-25.bpo-39413.7XYDM8.rst new file mode 100644 index 00000000000000..a185ab5efe2edd --- /dev/null +++ b/Misc/NEWS.d/next/Library/2020-01-24-10-10-25.bpo-39413.7XYDM8.rst @@ -0,0 +1 @@ +The :func:`os.unsetenv` function is now also available on Windows. diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index 13a69cd5f0e4dc..0f5995ec6400c0 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -6125,7 +6125,43 @@ os_putenv(PyObject *module, PyObject *const *args, Py_ssize_t nargs) #endif /* ((defined(HAVE_SETENV) || defined(HAVE_PUTENV)) && !defined(MS_WINDOWS)) */ -#if defined(HAVE_UNSETENV) +#if defined(MS_WINDOWS) + +PyDoc_STRVAR(os_unsetenv__doc__, +"unsetenv($module, name, /)\n" +"--\n" +"\n" +"Delete an environment variable."); + +#define OS_UNSETENV_METHODDEF \ + {"unsetenv", (PyCFunction)os_unsetenv, METH_O, os_unsetenv__doc__}, + +static PyObject * +os_unsetenv_impl(PyObject *module, PyObject *name); + +static PyObject * +os_unsetenv(PyObject *module, PyObject *arg) +{ + PyObject *return_value = NULL; + PyObject *name; + + if (!PyUnicode_Check(arg)) { + _PyArg_BadArgument("unsetenv", "argument", "str", arg); + goto exit; + } + if (PyUnicode_READY(arg) == -1) { + goto exit; + } + name = arg; + return_value = os_unsetenv_impl(module, name); + +exit: + return return_value; +} + +#endif /* defined(MS_WINDOWS) */ + +#if (defined(HAVE_UNSETENV) && !defined(MS_WINDOWS)) PyDoc_STRVAR(os_unsetenv__doc__, "unsetenv($module, name, /)\n" @@ -6157,7 +6193,7 @@ os_unsetenv(PyObject *module, PyObject *arg) return return_value; } -#endif /* defined(HAVE_UNSETENV) */ +#endif /* (defined(HAVE_UNSETENV) && !defined(MS_WINDOWS)) */ PyDoc_STRVAR(os_strerror__doc__, "strerror($module, code, /)\n" @@ -8773,4 +8809,4 @@ os__remove_dll_directory(PyObject *module, PyObject *const *args, Py_ssize_t nar #ifndef OS__REMOVE_DLL_DIRECTORY_METHODDEF #define OS__REMOVE_DLL_DIRECTORY_METHODDEF #endif /* !defined(OS__REMOVE_DLL_DIRECTORY_METHODDEF) */ -/*[clinic end generated code: output=6f42d8be634f5942 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=0348cbdff48691e3 input=a9049054013a1b77]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 6a687529df0c08..4a752809f1e185 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -10083,23 +10083,9 @@ posix_putenv_dict_setitem(PyObject *name, PyObject *value) #ifdef MS_WINDOWS -/*[clinic input] -os.putenv - - name: unicode - value: unicode - / - -Change or add an environment variable. -[clinic start generated code]*/ - -static PyObject * -os_putenv_impl(PyObject *module, PyObject *name, PyObject *value) -/*[clinic end generated code: output=d29a567d6b2327d2 input=ba586581c2e6105f]*/ +static PyObject* +win32_putenv(PyObject *name, PyObject *value) { - const wchar_t *env; - Py_ssize_t size; - /* Search from index 1 because on Windows starting '=' is allowed for defining hidden environment variables. */ if (PyUnicode_GET_LENGTH(name) == 0 || @@ -10108,36 +10094,68 @@ os_putenv_impl(PyObject *module, PyObject *name, PyObject *value) PyErr_SetString(PyExc_ValueError, "illegal environment variable name"); return NULL; } - PyObject *unicode = PyUnicode_FromFormat("%U=%U", name, value); + PyObject *unicode; + if (value != NULL) { + unicode = PyUnicode_FromFormat("%U=%U", name, value); + } + else { + unicode = PyUnicode_FromFormat("%U=", name); + } if (unicode == NULL) { return NULL; } - env = PyUnicode_AsUnicodeAndSize(unicode, &size); - if (env == NULL) - goto error; + Py_ssize_t size; + /* PyUnicode_AsWideCharString() rejects embedded null characters */ + wchar_t *env = PyUnicode_AsWideCharString(unicode, &size); + Py_DECREF(unicode); + + if (env == NULL) { + return NULL; + } if (size > _MAX_ENV) { PyErr_Format(PyExc_ValueError, "the environment variable is longer than %u characters", _MAX_ENV); - goto error; - } - if (wcslen(env) != (size_t)size) { - PyErr_SetString(PyExc_ValueError, "embedded null character"); - goto error; + PyMem_Free(env); + return NULL; } - if (_wputenv(env)) { + /* _wputenv() and SetEnvironmentVariableW() update the environment in the + Process Environment Block (PEB). _wputenv() also updates CRT 'environ' + and '_wenviron' variables, whereas SetEnvironmentVariableW() does not. + + Prefer _wputenv() to be compatible with C libraries using CRT + variables. */ + int err = _wputenv(env); + PyMem_Free(env); + + if (err) { posix_error(); - goto error; + return NULL; } - Py_DECREF(unicode); Py_RETURN_NONE; +} +#endif -error: - Py_DECREF(unicode); - return NULL; + +#ifdef MS_WINDOWS +/*[clinic input] +os.putenv + + name: unicode + value: unicode + / + +Change or add an environment variable. +[clinic start generated code]*/ + +static PyObject * +os_putenv_impl(PyObject *module, PyObject *name, PyObject *value) +/*[clinic end generated code: output=d29a567d6b2327d2 input=ba586581c2e6105f]*/ +{ + return win32_putenv(name, value); } /* repeat !defined(MS_WINDOWS) to workaround an Argument Clinic issue */ #elif (defined(HAVE_SETENV) || defined(HAVE_PUTENV)) && !defined(MS_WINDOWS) @@ -10186,7 +10204,23 @@ os_putenv_impl(PyObject *module, PyObject *name, PyObject *value) #endif /* defined(HAVE_SETENV) || defined(HAVE_PUTENV) */ -#ifdef HAVE_UNSETENV +#ifdef MS_WINDOWS +/*[clinic input] +os.unsetenv + name: unicode + / + +Delete an environment variable. +[clinic start generated code]*/ + +static PyObject * +os_unsetenv_impl(PyObject *module, PyObject *name) +/*[clinic end generated code: output=54c4137ab1834f02 input=4d6a1747cc526d2f]*/ +{ + return win32_putenv(name, NULL); +} +/* repeat !defined(MS_WINDOWS) to workaround an Argument Clinic issue */ +#elif defined(HAVE_UNSETENV) && !defined(MS_WINDOWS) /*[clinic input] os.unsetenv name: FSConverter @@ -10199,16 +10233,13 @@ static PyObject * os_unsetenv_impl(PyObject *module, PyObject *name) /*[clinic end generated code: output=54c4137ab1834f02 input=2bb5288a599c7107]*/ { -#ifndef HAVE_BROKEN_UNSETENV - int err; -#endif - #ifdef HAVE_BROKEN_UNSETENV unsetenv(PyBytes_AS_STRING(name)); #else - err = unsetenv(PyBytes_AS_STRING(name)); - if (err) + int err = unsetenv(PyBytes_AS_STRING(name)); + if (err) { return posix_error(); + } #endif #ifdef PY_PUTENV_DICT From 9145d18e8f987efa6cbda510162ba4bcf1a257af Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 24 Jan 2020 11:15:40 +0100 Subject: [PATCH 2/3] Complete the comment --- Modules/posixmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 4a752809f1e185..3a8e6aacb2afd7 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -10126,7 +10126,7 @@ win32_putenv(PyObject *name, PyObject *value) and '_wenviron' variables, whereas SetEnvironmentVariableW() does not. Prefer _wputenv() to be compatible with C libraries using CRT - variables. */ + variables and CRT functions using these variables (ex: getenv()). */ int err = _wputenv(env); PyMem_Free(env); From f6c43580a180aac39390d9f2f5d4aaf906ae85cd Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 24 Jan 2020 11:16:55 +0100 Subject: [PATCH 3/3] empty variable is invalid --- Lib/test/test_os.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 60134591926c98..dbdc00c2fea229 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -978,8 +978,9 @@ def test_putenv_unsetenv(self): @unittest.skipUnless(hasattr(os, 'putenv'), "Test needs os.putenv()") @unittest.skipUnless(hasattr(os, 'unsetenv'), "Test needs os.unsetenv()") def test_putenv_unsetenv_error(self): - # "=" and null character are not allowed in a variable name - for name in ('=name', 'na=me', 'name=', 'name\0', 'na\0me'): + # Empty variable name is invalid. + # "=" and null character are not allowed in a variable name. + for name in ('', '=name', 'na=me', 'name=', 'name\0', 'na\0me'): self.assertRaises((OSError, ValueError), os.putenv, name, "value") self.assertRaises((OSError, ValueError), os.unsetenv, name)