Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] bpo-39413: os.unsetenv() uses _wputenv() on Windows #18115

Closed
wants to merge 1 commit into from
Closed

[WIP] bpo-39413: os.unsetenv() uses _wputenv() on Windows #18115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions Lib/os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,15 +715,19 @@ def setdefault(self, key, value):
try:
_putenv = putenv
except NameError:
_putenv = lambda key, value: None
def _putenv(key, value):
# do nothing
return
else:
if "putenv" not in __all__:
__all__.append("putenv")

try:
_unsetenv = unsetenv
except NameError:
_unsetenv = lambda key: _putenv(key, "")
def _unsetenv(key):
# set the environment variable to an empty string
_putenv(key, "")
else:
if "unsetenv" not in __all__:
__all__.append("unsetenv")
Expand Down
10 changes: 6 additions & 4 deletions Lib/test/test_os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,10 +955,12 @@ def test_environb(self):

# On OS X < 10.6, unsetenv() doesn't return a value (bpo-13415).
@support.requires_mac_ver(10, 6)
def test_unset_error(self):
# "=" is not allowed in a variable name
key = 'key='
self.assertRaises(OSError, os.environ.__delitem__, key)
def test_unsetenv_error(self):
self.assertRaises(OSError, os.unsetenv, '=key')
# On Windows, only names starting with "=" are invalid
if sys.platform != "win32":
self.assertRaises(OSError, os.unsetenv, 'ke=y')
self.assertRaises(OSError, os.unsetenv, 'key=')

def test_key_type(self):
missing = 'missingkey'
Expand Down
10 changes: 5 additions & 5 deletions Modules/clinic/posixmodule.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

108 changes: 48 additions & 60 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10079,53 +10079,31 @@ posix_putenv_dict_setitem(PyObject *name, PyObject *value)
#endif /* PY_PUTENV_DICT */


#ifdef HAVE_PUTENV

#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*
py_win_putenv(PyObject *name, PyObject *value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is py_win_ the naming convention from now on -- as opposed to win32_?

{
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 ||
PyUnicode_FindChar(name, '=', 1, PyUnicode_GET_LENGTH(name), 1) != -1)
{
PyErr_SetString(PyExc_ValueError, "illegal environment variable name");
return NULL;
}
PyObject *unicode = PyUnicode_FromFormat("%U=%U", name, value);
if (unicode == NULL) {
return NULL;
}

env = PyUnicode_AsUnicodeAndSize(unicode, &size);
if (env == NULL)
const wchar_t *env;
Py_ssize_t size;
/* PyUnicode_AsWideCharString() rejects embedded null characters */
env = PyUnicode_AsWideCharString(unicode, &size);
if (env == NULL) {
goto error;
}
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;
}

/* Use _wputenv() rather than SetEnvironmentVariableW(): _wputenv()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The search for "=" (line 10102) should begin at index 0, not index 1. Windows allows variable names that begin with 0, but ucrt does not allow it. For example:

>>> os.putenv('spam=', 'eggs')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: illegal environment variable name

>>> os.putenv('=spam', 'eggs')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 22] Invalid argument

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this surprising difference and I'm trying to understand what should be the correct behavior...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with '=C'?

Copy link
Contributor

@eryksun eryksun Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python initializes nt.environ from C _wenviron, so it follows the long-standing convention of MSVC to exclude hidden variables that begin with "=" and to disallow setting them. We still set them for per-drive working directories (e.g. "=Z:"), but we set them in the process environment only, with SetEnvironmentVariableW. For example, see win32_wchdir in this module. The drive variables are supposed to be a hidden implementation detail. CMD's set doesn't show them, except due to a parsing bug if we run set "" or set ".

Copy link
Contributor

@eryksun eryksun Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to allow setting them, then Python should initialize nt.environ from WINAPI GetEnvironmentStringsW instead of _wenviron (or at least also from GetEnvironmentStringsW , just for the hidden variables). Then if a name begins with "=", call SetEnvironmentVariableW instead of _wputenv.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about os.unsetenv("key=value")?

By the way, why does os.putenv() explicitly rejects "key=" variable name?

Because in "key==value" what is a key and what is a value? key=="key" and value=="=value" or key=="key=" and value=="value"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows is so weird. The official documentation says:

The name of an environment variable cannot include an equal sign (=).

I tested: I can set a variable with name "zlong=" (the name contains "="). It is shown by the set command for example:

>>> import os
>>> os.putenv("zlong=", "1")
>>> os.putenv("zlon=g", "2")
>>> os.system("set")
(...)
zlon=g=2
zlong==1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The posix_putenv_dict_setitem call below is unnecessary with _wputenv. It copies the string before adding it to _wenviron, i.e. it does not comply with POSIX putenv.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked back to even Python 2.7 w/ msvcr90.dll, and even that old version of the CRT copies the string that's passed to putenv (via calls to _calloc_crt and strcpy_s), which I verified by noting the result from _calloc_crt and cross-checking with the address returned by getenv. The CRT didn't used to copy the string back in the 1990s, but that's ancient history.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The posix_putenv_dict_setitem call below is unnecessary with _wputenv. It copies the string before adding it to _wenviron, i.e. it does not comply with POSIX putenv.

Fixed in commit 0852c7d.

updates the CRT, whereas SetEnvironmentVariableW() does not. */
if (_wputenv(env)) {
posix_error();
goto error;
Expand All @@ -10138,7 +10116,37 @@ os_putenv_impl(PyObject *module, PyObject *name, PyObject *value)
Py_DECREF(unicode);
return NULL;
}
#else /* MS_WINDOWS */
#endif


#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]*/
{
/* Search from index 1 because on Windows starting '=' is allowed for
defining hidden environment variables. */
if (PyUnicode_GET_LENGTH(name) == 0 ||
PyUnicode_FindChar(name, '=', 1, PyUnicode_GET_LENGTH(name), 1) != -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this search should start at index 0, so that it consistently raises ValueError, instead of OSError if a name begins with "=".

{
PyErr_SetString(PyExc_ValueError, "illegal environment variable name");
return NULL;
}

return py_win_putenv(name, value);
}
/* repeat !defined(MS_WINDOWS) to workaround an Argument Clinic issue */
#elif defined(HAVE_PUTENV) && !defined(MS_WINDOWS)
/*[clinic input]
os.putenv

Expand Down Expand Up @@ -10176,8 +10184,7 @@ os_putenv_impl(PyObject *module, PyObject *name, PyObject *value)
posix_putenv_dict_setitem(name, bytes);
Py_RETURN_NONE;
}
#endif /* MS_WINDOWS */
#endif /* HAVE_PUTENV */
#endif /* defined(HAVE_PUTENV) && !defined(MS_WINDOWS) */


#ifdef MS_WINDOWS
Expand All @@ -10193,35 +10200,16 @@ static PyObject *
os_unsetenv_impl(PyObject *module, PyObject *name)
/*[clinic end generated code: output=54c4137ab1834f02 input=4d6a1747cc526d2f]*/
{
/* PyUnicode_AsWideCharString() rejects embedded null characters */
wchar_t *name_str = PyUnicode_AsWideCharString(name, NULL);
if (name_str == NULL) {
PyObject *empty = PyUnicode_New(0, 0);
if (empty == NULL) {
return NULL;
}

BOOL ok = SetEnvironmentVariableW(name_str, NULL);
PyMem_Free(name_str);

if (!ok) {
return PyErr_SetFromWindowsErr(0);
}
/* _wputenv("name=") removes the environment variable "name" */
PyObject *res = py_win_putenv(name, empty);
Py_DECREF(empty);

#ifdef PY_PUTENV_DICT
/* Remove the key from putenv_dict;
* this will cause it to be collected. This has to
* happen after the real unsetenv() call because the
* old value was still accessible until then.
*/
if (PyDict_DelItem(_posixstate(module)->putenv_dict, name)) {
/* really not much we can do; just leak */
if (!PyErr_ExceptionMatches(PyExc_KeyError)) {
return NULL;
}
PyErr_Clear();
}
#endif

Py_RETURN_NONE;
return res;
}
/* repeat !defined(MS_WINDOWS) to workaround an Argument Clinic issue */
#elif defined(HAVE_UNSETENV) && !defined(MS_WINDOWS)
Expand Down