FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[WIP] bpo-39413: os.unsetenv() uses _wputenv() on Windows by vstinner · Pull Request #18115 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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

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

Closed
vstinner wants to merge 1 commit into from
Conversation 29 Commits 1 Checks 0 Files changed
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 .c  (1) .h  (1) .py  (2) All 3 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
8 changes: 6 additions & 2 deletions Lib/os.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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)

eryksun Jan 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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()

eryksun Jan 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

vstinner Jan 22, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

serhiy-storchaka Jan 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does it work with '=C'?

eryksun Jan 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 ".

eryksun Jan 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

serhiy-storchaka Jan 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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"?

vstinner Jan 22, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

eryksun Jan 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

eryksun Jan 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

vstinner Jan 24, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

eryksun Jan 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Back | FazBrowse Home | New Git URL