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

bpo-39413: Implement os.unsetenv() on Windows #18163

Merged
merged 3 commits into from
Jan 24, 2020
Merged

bpo-39413: Implement os.unsetenv() on Windows #18163

merged 3 commits into from
Jan 24, 2020

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 24, 2020

The os.unsetenv() function is now also available on Windows.

https://bugs.python.org/issue39413

The os.unsetenv() function is now also available on Windows.
@vstinner
Copy link
Member Author

@eryksun @serhiy-storchaka: Here is a simpler implementation of os.unsetenv() for Windows.

  • Reuse os.putenv() code to check name
  • Add many tests for invalid names
  • Add tests for os.putenv()

In practice,os.unsetenv() rejects "=" anywhere in the name on all platforms: it's now validated by unit tests. I chose to not change raised exceptions (ValueError vs OSError) for invalid variable name in this PR. I plan to propose a separated PR to unify the raised exception on all platforms.

I also chose to not remove the Windows maximum variable length limit in this PR, again to make it easier to review. I also plan to propose a separated PR to limit this outdated limit.

@eryksun
Copy link
Contributor

eryksun commented Jan 24, 2020

I also chose to not remove the Windows maximum variable length limit in this PR, again to make it easier to review. I also plan to propose a separated PR to limit this outdated limit.

_MAX_ENV (32767) itself isn't outdated. However, the limit of less than 32767 characters (i.e. 32766 or less) is enforced individually on the name and value, not on the "name=value" entry. It either has to check them individually and raise ValueError or suppress the invalid parameter handler around the call via _Py_BEGIN_SUPPRESS_IPH / _Py_END_SUPPRESS_IPH and fail with a generic EINVAL error.

@eryksun
Copy link
Contributor

eryksun commented Jan 24, 2020

Consider calling _wputenv_s, which has separate parameters for the name and value. Also, other win32_ functions in this module take C data types. I think it would be simpler to change the clinic definition of os.putenv and os.unsetenv to use Py_UNICODE. This is argument type "u", which is based on PyUnicode_AsUnicodeAndSize and checks for embedded null characters. The code for os_unsetenv_impl would change to calling win32_putenv(name, L""). Here's what this might look like -- off the top my head, so excuse any errors:

static PyObject *
win32_putenv(wchar_t *name, wchar_t *value)
{
    size_t name_length = wcslen(name);
    
    if (!name_length || wcschr(name, L'=')) {
        PyErr_SetString(PyExc_ValueError, "illegal environment variable name");
        return NULL;
    }
    
    if (name_length >= _MAX_ENV) {
        PyErr_Format(PyExc_ValueError, "the environment variable name "
            "is longer than %u characters", _MAX_ENV - 1);
        return NULL;
    }
    
    if (wcslen(value) >= _MAX_ENV) {
        PyErr_Format(PyExc_ValueError, "the environment variable value "
            "is longer than %u characters", _MAX_ENV - 1);
        return NULL;
    }

    /* Both _wputenv_s() and SetEnvironmentVariableW() update the environment
       in the Process Environment Block (PEB), but _wputenv_s() also updates
       the CRT environ and _wenviron variables. Use _wputenv_s() in order to
       be compatible with C libraries that use the CRT variables and the CRT
       functions that use these variables, such as getenv(). */

    errno_t err;
    _Py_BEGIN_SUPPRESS_IPH
    err = _wputenv_s(name, value);
    _Py_END_SUPPRESS_IPH

    if (err) {
        posix_error();
        return NULL;
    }

    Py_RETURN_NONE;
}

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
The os.unsetenv() function is now also available on Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants