python / cpython

The Python programming language
https://www.python.org
Other
63.12k stars 30.22k forks source link

os.environ.clear() fails with empty keys (posix.unsetenv) #64857

Open 1f7c169c-a6ad-45ae-b614-0d40d962c776 opened 10 years ago

1f7c169c-a6ad-45ae-b614-0d40d962c776 commented 10 years ago
BPO 20658
Nosy @pefu, @vstinner, @blueyed, @ned-deily, @4kir4, @serhiy-storchaka, @iritkatriel
Files
  • test_empty_env_var.py
  • reject_empty_env_var.patch
  • set_unset_env.c
  • 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: ```python assignee = None closed_at = None created_at = labels = ['interpreter-core', '3.11'] title = 'os.environ.clear() fails with empty keys (posix.unsetenv)' updated_at = user = 'https://github.com/blueyed' ``` bugs.python.org fields: ```python activity = actor = 'vstinner' assignee = 'none' closed = False closed_date = None closer = None components = ['Interpreter Core'] creation = creator = 'blueyed' dependencies = [] files = ['34132', '34133', '50245'] hgrepos = [] issue_num = 20658 keywords = ['patch'] message_count = 21.0 messages = ['211415', '211450', '211451', '211508', '211509', '211612', '211613', '211615', '211617', '211619', '211622', '211624', '212405', '400467', '400609', '400610', '400611', '400613', '400616', '401037', '401041'] nosy_count = 8.0 nosy_names = ['pefu', 'vstinner', 'blueyed', 'ned.deily', 'Arfrever', 'akira', 'serhiy.storchaka', 'iritkatriel'] pr_nums = [] priority = 'normal' resolution = None stage = 'resolved' status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue20658' versions = ['Python 3.11'] ```

    1f7c169c-a6ad-45ae-b614-0d40d962c776 commented 10 years ago

    posix.unsetenv fails to clear the environment if there's an entry with an empty key.

    TEST CASE:

        Python 2.7.6 (default, Jan  6 2014, 17:05:19) 
        [GCC 4.8.1] on linux2
        Type "help", "copyright", "credits" or "license" for more information.
        >>> import os
        >>> os.environ['']='foo'
        >>> os.environ.clear()
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
          File "/path/to/python/2.7.6/lib/python2.7/os.py", line 499, in clear
            unsetenv(key)
        OSError: [Errno 22] Invalid argument

    I believe that os.environ.clear() via unsetenv should handle empty keys.

    ned-deily commented 10 years ago

    According to the Open Group Base Specification (Issue 7 2013 Issue):

    "The setenv() function shall fail if:

    [EINVAL] The envname argument points to an empty string or points to a string containing an '=' character."

    So it seems to me that the issue here is that os.environ[''] attempts to allow you to create a environment variable with a null key.

    ned-deily commented 10 years ago

    OTOH, the specification for putenv, which is what is actually used by posixmodule.c, does not contain that requirement.

    http://pubs.opengroup.org/onlinepubs/9699919799/functions/putenv.html http://pubs.opengroup.org/onlinepubs/9699919799/functions/setenv.html

    vstinner commented 10 years ago

    putenv("=value") does nothing: it doesn't create a variable with an empty name. You can test with the attach test_empty_env_var.py script (written for Linux).

    Attached reject_empty_env_var.patch patch modifies posix.putenv() to raise a ValueError if the name is empty.

    The patch is incomplete, the Windows part should also be modified. But I didn't test yet if Windows supports variables with empty name.

    vstinner commented 10 years ago

    The workaround of this bug is to avoid "os.environ['']=value".

    1f7c169c-a6ad-45ae-b614-0d40d962c776 commented 10 years ago

    Please note that I have noticed this not because of setting it via os.environ, but because a program using os.environ.clear() crashed: https://bugs.launchpad.net/ubuntu/+source/apport/+bug/1281086

    I cannot say how this unusual entry was added to the environment, but it showed up in env and therefore it is possible somehow.

    vstinner commented 10 years ago

    "Please note that I have noticed this not because of setting it via os.environ, but because a program using os.environ.clear() crashed: https://bugs.launchpad.net/ubuntu/+source/apport/+bug/1281086"

    The bug is not os.environ.clear(), but that os.environ contains an empty string.

    It would help to know if the key was set manually by apport, or if it comes from the real environment. The environment looks correct: https://launchpadlibrarian.net/166517334/ProcEnviron.txt

    1f7c169c-a6ad-45ae-b614-0d40d962c776 commented 10 years ago

    It would help to know if the key was set manually by apport, or if it comes from the real environment. The environment looks correct:

    It comes from the real environment.

    I wanted to use apport, but could not from the current shell, because of this bug.

    I had then looked at the output of env in this shell, and it contained just "=" (IIRC, but an empty name/key for sure). This was in a tmux shell/pane, which I have closed already.

    vstinner commented 10 years ago

    It comes from the real environment.

    Oh. It looks possible to define an environment variable with an empty key, but not to remove it:

    $ env -i =value python -c 'import pprint, os; pprint.pprint(os.environ); del os.environ[""]'
    environ({'': 'value'})
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "Lib/os.py", line 662, in __delitem__
        self.unsetenv(encodedkey)
    OSError: [Errno 22] Invalid argument
    vstinner commented 10 years ago

    By the way, Python allows also to set an environment with a name containing "=", whereas getenv/setenv/unsetenv doesn't.

    $ env -i python -c 'import pprint, posix, os; os.environ["a="]="1"; print(os.environ); posix.unsetenv("a=")'
    environ({'a=': '1'})
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    OSError: [Errno 22] Invalid argument
    serhiy-storchaka commented 10 years ago

    Are there supported platforms on which setenv() is not supported? When we will migrate to setenv(), an argument check will be not needed.

    In any case I think we should wrap unsetenv() in os.environ.clear() so that it should try to remove all environment variables even if some calls of unsetenv() fails.

    vstinner commented 10 years ago

    In fact, there is also a clearenv() function which could be used by os.environ.clear().

    "The clearenv() function clears the environment of all name-value pairs and sets the value of the external variable environ to NULL."

    It looks like supported names depends a lot on the platform and platform version. Extract of Linux manual pages:

    setenv: --- BUGS:

    POSIX.1-2001 specifies that if name contains an '=' character, then setenv() should fail with the error EINVAL; however, versions of glibc before 2.3.4 allowed an '=' sign in name. ---

    clearenv: --- CONFORMING TO

    Various UNIX variants (DG/UX, HP-UX, QNX, ...). POSIX.9 (bindings for FORTRAN77). POSIX.1-1996 did not accept clearenv() and putenv(3), but changed its mind and scheduled these functions for some later issue of this standard (cf. B.4.6.1). However, POSIX.1-2001 adds only putenv(3), and rejected clearenv(). ---

    In any case I think we should wrap unsetenv() in os.environ.clear() so that it should try to remove all environment variables even if some calls of unsetenv() fails.

    os.environ.clear() may tries to remove as much keys as possible, but keep keys for which unsetenv raised an error and raise a global error in this case.

    7fe5d93b-2a2c-46a0-b5cd-5602c591856a commented 10 years ago

    Related: bpo-4926 "putenv() accepts names containing '=', return value of unsetenv() not checked"

    os.environ.clear() also fails if the environment contains names with =:

      >>> import os
      >>> os.environ['a=b'] = 'c'
      >>> os.environ.clear()
      Traceback (most recent call last):
        File "<stdin>", line 1, in <module>
        File "/.../lib/python3.4/_collections_abc.py", line 558, in clear
          self.popitem()
        File "/.../lib/python3.4/_collections_abc.py", line 551, in popitem
          del self[key]
        File "/.../lib/python3.4/os.py", line 662, in __delitem__
          self.unsetenv(encodedkey)
      OSError: [Errno 22] Invalid argument
    iritkatriel commented 3 years ago

    They keys are checked now, so I think this is resolved. (Tested on windows and Mac).

    >>> os.environ[''] = 'X'
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/Users/iritkatriel/src/cpython/Lib/os.py", line 684, in __setitem__
        putenv(key, value)
        ^^^^^^^^^^^^^^^^^^
    OSError: [Errno 22] Invalid argument
    
    >>> os.environ['a=b'] = 'c'
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/Users/iritkatriel/src/cpython/Lib/os.py", line 684, in __setitem__
        putenv(key, value)
        ^^^^^^^^^^^^^^^^^^
    ValueError: illegal environment variable name
    vstinner commented 3 years ago

    The following command still fails on the Python main branch on Linux: ---

    $ env -i =value ./python -c 'import pprint, os; pprint.pprint(os.environ); del os.environ[""]'
    environ({'': 'value', 'LC_CTYPE': 'C.UTF-8'})
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/home/vstinner/python/main/Lib/os.py", line 689, in __delitem__
        unsetenv(encodedkey)
        ^^^^^^^^^^^^^^^^^^^^
    OSError: [Errno 22] Invalid argument

    'del os.environ[""]' calls unsetenv("") which fails with EINVAL.

    Python is a thin wrapper to C functions setenv() and unsetenv(), and raises an exception when a C function fails. It works as expected.

    Python exposes the variable with an empty name which is found in the environment variables: again, it works as expected.

    I don't see how Python could do better, since the glibc unsetenv() fails with EINVAL if the string is empty. It is even a documented behaviour, see the unsetenv() manual page: --- ERRORS

    EINVAL name is NULL, points to a string of length 0, or contains an '=' character. ---

    vstinner commented 3 years ago

    By the way: --- The env command from GNU coreutils supports setting the environment variable with an empty name but not unsetting it. That's a bug.

    $ env '=wibble' env |grep wibble                                 
    =wibble
    $ env '=wibble' env -u '' env
    env: cannot unset `': Invalid argument

    https://unix.stackexchange.com/questions/178522/unsetting-environment-variable-with-an-empty-name

    iritkatriel commented 3 years ago

    I see, so intercepting the assignment is not enough. Reopening.

    vstinner commented 3 years ago

    Attached set_unset_env.c program calls putenv("=hello world") and then unsetenv("").

    On my Fedora 34 with glibc-2.33-20.fc34.x86_64, putenv() succeed, but unsetenv() fails. ---

    $ gcc set_unset_env.c -g -o set_unset_env && ./set_unset_env
    putenv("=hello world") -> hello world
    ERROR: unsetenv("") failed: [error 22] Invalid argument

    By the way, getenv() fails to find an environment variable if its name is empty: I reimplemented getenv() using the 'environ' variable for my test.

    vstinner commented 3 years ago

    For the very specific case of os.environ.clear(), the C function clearenv() could be used if available.

    While clearenv() is available in the glibc, it's not a POSIX function.

    serhiy-storchaka commented 3 years ago

    For the very specific case of os.environ.clear(), the C function clearenv() could be used if available.

    It is bad in any way. The supposed example of using clear():

        old_environ = dict(os.environ)
        os.environ.clear()
        os.environ.update(new_environ)
        ...
        os.environ.clear()
        os.environ.update(old_environ)

    Even if clear() passed, it will fail on attempt to restore the old environment, with the empty key.

    You can have the same issue in a corresponding C code. I think the only way is to report this as a bug in glibc and wait on their reaction.

    vstinner commented 3 years ago

    POSIX says:

    (1) "The setenv() function shall fail if: [EINVAL] The name argument is a null pointer, points to an empty string, or points to a string containing an '=' character."

    https://pubs.opengroup.org/onlinepubs/009604499/functions/setenv.html

    (2) "The unsetenv() function shall fail if: [EINVAL] The name argument is a null pointer, points to an empty string, or points to a string containing an '=' character."

    https://pubs.opengroup.org/onlinepubs/009695399/functions/unsetenv.html

    But POSIX doesn't specify which keys are invalid for putenv. It only specifies a single error:

    (3) "The putenv() function may fail if: [ENOMEM] Insufficient memory was available."

    https://pubs.opengroup.org/onlinepubs/009696899/functions/putenv.html

    The GNU libc respects POSIX.

    IMO setenv() and unsetenv() are fine. The problem comes from putenv() which is under specified and allows keys which are causing a lot of subtle issues, not only in Python.