classification
Title: Implement os.putenv() with setenv() if available
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eryksun, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2020-01-21 08:30 by vstinner, last changed 2020-01-22 22:00 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 18095 closed vstinner, 2020-01-21 08:56
PR 18106 merged vstinner, 2020-01-21 17:56
PR 18126 merged vstinner, 2020-01-22 20:14
PR 18128 merged vstinner, 2020-01-22 21:03
Messages (15)
msg360365 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-21 08:30
Currently, os.putenv() is always implemented with putenv(). The problem is that putenv(str) puts directly the string into the environment, the string is not copied. So Python has to keep track of this memory.

In Python 3.9, this string is now cleared at Python exit, without unsetting the environment variable which cause bpo-39395 crash.

I propose to implement os.putenv() with setenv() if available, which avoids bpo-39395 on platforms providing setenv().
msg360370 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-21 09:02
setenv() is available on:

* Linux: http://man7.org/linux/man-pages/man3/setenv.3.html
* macOS: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/setenv.3.html
* FreeBSD: https://www.freebsd.org/cgi/man.cgi?query=getenv&sektion=3&apropos=0&manpath=FreeBSD+12.1-RELEASE

It is not available on Windows.
msg360371 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-01-21 09:12
Maybe use SetEnvironmentVariable() on Windows?
msg360378 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-21 10:03
> Maybe use SetEnvironmentVariable() on Windows?

I didn't know this function. I updated PR 18095.
msg360386 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2020-01-21 10:57
> Maybe use SetEnvironmentVariable() on Windows? 

`_wputenv` keeps Python's copy of the environment block in sync with CRT's copy, which in turn calls `SetEnvironmentVariableW` to sync with process environment block. The CRT's copy of the environment excludes and disallows variable names that start with "=". Because the interpreter initializes `os.environ` from the CRT environment, Python has never included or allowed 'hidden' variables such as "=C:". 

Switching to calling `SetEnvironmentVariableW` directly won't affect `os.system` and `os.spawnv[e]`, since the CRT calls `CreateProcessW` without overriding the process environment. However, have you considered the consequences for extension modules and embedding applications that use the CRT environment (e.g. `getenv`, `_wgetenv`) if the interpreter stops syncing with it? 

The concern that `[_w]putenv` doesn't copy the string does not apply to Windows.
msg360388 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-01-21 11:19
This indeed a good argument to continue to use _wputenv. Thank you Eryk, you have saved us from making a mistake.
msg360419 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-21 18:08
I created bpo-39413 "Implement os.unsetenv() on Windows" to prepare work on this issue. But Eryk raised the same concern about CRT: https://bugs.python.org/issue39413#msg360404

I wasn't aware of that :-/

> `_wputenv` keeps Python's copy of the environment block in sync with CRT's copy, which in turn calls `SetEnvironmentVariableW` to sync with process environment block. The CRT's copy of the environment excludes and disallows variable names that start with "=". Because the interpreter initializes `os.environ` from the CRT environment, Python has never included or allowed 'hidden' variables such as "=C:". 

My main motivation to use SetEnvironmentVariableW() on Windows is to avoid bpo-39395. Do you mean that Windows _putenv() is not affected by bpo-39395: Python can safely removes the string to _putenv() just after the call?
msg360421 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-21 18:25
New changeset 623ed6171eae35af7fd2e804dfd9c832c05c5d48 by Victor Stinner in branch 'master':
bpo-39406: Add PY_PUTENV_DICT macro to posixmodule.c (GH-18106)
https://github.com/python/cpython/commit/623ed6171eae35af7fd2e804dfd9c832c05c5d48
msg360422 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-01-21 18:29
There is also _wputenv_s which affects _spawn, _exec and system.
msg360424 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-01-21 18:42
Why posix_putenv_garbage was renamed to putenv_dict?
msg360445 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2020-01-22 03:22
> Python can safely removes the string to _putenv() just after the call? 

Windows ucrt copies the buffer that's passed to _[w]putenv. This makes it non-compliant with POSIX but easier to use in this case. Here's an example using ctypes:

    >>> ucrt = ctypes.CDLL('ucrtbase')
    >>> buf = ctypes.create_unicode_buffer('spam=eggs')
    >>> ucrt._wputenv(buf)
    0

Directly modifying the buffer has no effect on the _wgetenv result:

    >>> buf[5] = 'a'
    >>> ucrt._wgetenv.restype = ctypes.POINTER(ctypes.c_wchar)
    >>> ucrt._wgetenv('spam')[:4]
    'eggs'

Linux putenv complies with POSIX, so changing "eggs" to "aggs" in the buffer does affect the getenv result in Linux. 

On the other hand, ucrt's [_w]getenv does not copy the environment string. For example:

    >>> p1 = ucrt._wgetenv('spam')
    >>> p1[0] = 'o'

    >>> p2 = ucrt._wgetenv('spam')
    >>> p2[:4]
    'oggs'

[_w]getenv is not thread safe. Even though all calls that access the ucrt environment are internally synchronized on a common lock, accessing the *result* from [_w]getenv still needs to be synchronized with concurrent _[w]putenv calls in a multithreaded process. Better still, use [_w]getenv_s or _[w]dupenv_s, which returns a copy.

> There is also _wputenv_s which affects _spawn, _exec and system.

The documentation is perhaps misleading. When a variable is set with _[w]putenv[_s], it also sets the variable in the process environment block to stay in sync. This indirectly affects _spawn*, _exec* and system(). common_spawnv (in ucrt\exec\spawnv.cpp), which implements the latter functions, calls CreateProcess with lpEnvironment as NULL (i.e.  inherit the current process environment), unless the caller passes a new environment (e.g. envp of spawnve).
msg360505 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-22 20:53
New changeset 0852c7dd52ac42e7843ddfef44571494e4c86070 by Victor Stinner in branch 'master':
bpo-39406: os.putenv() avoids putenv_dict on Windows (GH-18126)
https://github.com/python/cpython/commit/0852c7dd52ac42e7843ddfef44571494e4c86070
msg360509 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-22 21:48
New changeset b477d19a6b7751b0c933b239dae4fc96dbcde9c4 by Victor Stinner in branch 'master':
bpo-39406: Implement os.putenv() with setenv() if available (GH-18128)
https://github.com/python/cpython/commit/b477d19a6b7751b0c933b239dae4fc96dbcde9c4
msg360511 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-22 21:59
Serhiy:
> Maybe use SetEnvironmentVariable() on Windows?

In bpo-39413, I implemented os.unsetenv() on Windows using SetEnvironmentVariable(), but Eryk reported that SetEnvironmentVariable() does not update the CRT API (_environ, _wenviron, getenv, etc.). So I reverted my change. See bpo-39413 for the details.


Eryk:
> Windows ucrt copies the buffer that's passed to _[w]putenv.

Thank you for checking manually. I pushed commit 0852c7dd52ac42e7843ddfef44571494e4c86070 to avoid putenv_dict on Windows.
msg360513 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-22 22:00
The initial issue has been fixed, so I close the issue. Big thanks to Eryk Sun for your great help, thanks also Serhiy Storchaka.
History
Date User Action Args
2020-01-22 22:00:40vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg360513

stage: patch review -> resolved
2020-01-22 21:59:02vstinnersetmessages: + msg360511
2020-01-22 21:48:19vstinnersetmessages: + msg360509
2020-01-22 21:03:44vstinnersetpull_requests: + pull_request17515
2020-01-22 20:53:33vstinnersetmessages: + msg360505
2020-01-22 20:14:33vstinnersetpull_requests: + pull_request17513
2020-01-22 03:22:06eryksunsetmessages: + msg360445
2020-01-21 18:42:22serhiy.storchakasetmessages: + msg360424
2020-01-21 18:29:42serhiy.storchakasetmessages: + msg360422
2020-01-21 18:25:46vstinnersetmessages: + msg360421
2020-01-21 18:08:46vstinnersetmessages: + msg360419
2020-01-21 17:56:45vstinnersetpull_requests: + pull_request17495
2020-01-21 11:19:38serhiy.storchakasetmessages: + msg360388
2020-01-21 10:57:50eryksunsetnosy: + eryksun
messages: + msg360386
2020-01-21 10:03:49vstinnersetmessages: + msg360378
2020-01-21 09:12:23serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg360371
2020-01-21 09:02:01vstinnersetmessages: + msg360370
2020-01-21 08:56:59vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request17485
2020-01-21 08:30:02vstinnercreate