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.

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

Implement os.putenv() with setenv() if available #83587

Closed
vstinner opened this issue Jan 21, 2020 · 15 comments
Closed

Implement os.putenv() with setenv() if available #83587

vstinner opened this issue Jan 21, 2020 · 15 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir

Comments

@vstinner
Copy link
Member

BPO 39406
Nosy @vstinner, @serhiy-storchaka, @eryksun
PRs
  • bpo-39406: Implement os.putenv() with setenv() if available #18095
  • bpo-39406: Add PY_PUTENV_DICT macro to posixmodule.c #18106
  • bpo-39406: os.putenv() avoids putenv_dict on Windows #18126
  • bpo-39406: Implement os.putenv() with setenv() if available #18128
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2020-01-22.22:00:40.936>
    created_at = <Date 2020-01-21.08:30:01.989>
    labels = ['library', '3.9']
    title = 'Implement os.putenv() with setenv() if available'
    updated_at = <Date 2020-01-22.22:00:40.935>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2020-01-22.22:00:40.935>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-01-22.22:00:40.936>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2020-01-21.08:30:01.989>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39406
    keywords = ['patch']
    message_count = 15.0
    messages = ['360365', '360370', '360371', '360378', '360386', '360388', '360419', '360421', '360422', '360424', '360445', '360505', '360509', '360511', '360513']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'serhiy.storchaka', 'eryksun']
    pr_nums = ['18095', '18106', '18126', '18128']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue39406'
    versions = ['Python 3.9']

    @vstinner
    Copy link
    Member Author

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

    @vstinner vstinner added 3.9 only security fixes stdlib Python modules in the Lib dir labels Jan 21, 2020
    @serhiy-storchaka
    Copy link
    Member

    Maybe use SetEnvironmentVariable() on Windows?

    @vstinner
    Copy link
    Member Author

    Maybe use SetEnvironmentVariable() on Windows?

    I didn't know this function. I updated PR 18095.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jan 21, 2020

    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.

    @serhiy-storchaka
    Copy link
    Member

    This indeed a good argument to continue to use _wputenv. Thank you Eryk, you have saved us from making a mistake.

    @vstinner
    Copy link
    Member Author

    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?

    @vstinner
    Copy link
    Member Author

    New changeset 623ed61 by Victor Stinner in branch 'master':
    bpo-39406: Add PY_PUTENV_DICT macro to posixmodule.c (GH-18106)
    623ed61

    @serhiy-storchaka
    Copy link
    Member

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

    @serhiy-storchaka
    Copy link
    Member

    Why posix_putenv_garbage was renamed to putenv_dict?

    @eryksun
    Copy link
    Contributor

    eryksun commented Jan 22, 2020

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

    @vstinner
    Copy link
    Member Author

    New changeset 0852c7d by Victor Stinner in branch 'master':
    bpo-39406: os.putenv() avoids putenv_dict on Windows (GH-18126)
    0852c7d

    @vstinner
    Copy link
    Member Author

    New changeset b477d19 by Victor Stinner in branch 'master':
    bpo-39406: Implement os.putenv() with setenv() if available (GH-18128)
    b477d19

    @vstinner
    Copy link
    Member Author

    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 0852c7d to avoid putenv_dict on Windows.

    @vstinner
    Copy link
    Member Author

    The initial issue has been fixed, so I close the issue. Big thanks to Eryk Sun for your great help, thanks also Serhiy Storchaka.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants