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

Conversation

@vstinner
Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 24, 2020

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

vstinner added 2 commits Jan 24, 2020
@vstinner vstinner merged commit 161e7b3 into python:master Jan 24, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200124.28 succeeded
Details
bedevere/issue-number Issue number 39413 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vstinner vstinner deleted the vstinner:win_unsetenv4 branch Jan 24, 2020
@eryksun

This comment has been minimized.

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

This comment has been minimized.

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;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.