One of these problems interacts with the other, and can cause
os.unsetenv() to free memory that's still in use.  Firstly,
calling os.putenv("FOO=BAR", "value") causes putenv(3) to be
called with the string "FOO=BAR=value", which sets a variable
called FOO, not FOO=BAR; hence, os.putenv() should not accept a
name with an "=" character in it.

The way this interacts with os.unsetenv() is that the string
(FOO=BAR=value) is stored in the internal dictionary object
posix_putenv_garbage under the key "FOO=BAR".  The reference in
this dict is supposed to prevent the bytes object (str in 3.x on
Windows) that contains the string from being garbage collected
and freed until unsetenv() is called, since putenv(3) makes the
char **environ array point to the actual string, not a copy.

The problem with os.unsetenv() is that it does not check the
return value from unsetenv(3) at all and will unconditionally
remove the corresponding string from posix_putenv_garbage.
Following the example above, when os.unsetenv("FOO=BAR") is
called, unsetenv(3) will fail because the name contains an "="
character, but the object containing the string will be garbage
collected even though char **environ still has a pointer to it.

This is a bit tricky to give a visible demonstration of, but the
attached visibility.diff adds posix_putenv_garbage to the module
namespace and prints the return value from unsetenv() so you can
see what's going on.

The other two attached diffs fix the problems (for 2.x and 3.x
separately) by making os.unsetenv() raise OSError on failure in
the usual way, and os.putenv() raise ValueError when its first
argument contains "=".  They also add test cases and docs.  In
the patch for 3.x, some of the relevant code differs between Unix
and Windows; I changed both but I've only tested the Unix
version.

As patches were originally provided would someone kindly review them.

The patch actually does 2 things:
- it prevents the usage of '=' in putenv, which is is good because the putenv() system call handles this badly.
- it will raise an error when unsetting a nonexistent variable. This is a change in behaviour which is not needed. (in a shell, the "unset" command will silently ignore missing variables)

Also, some unit tests would be appreciated

Unit tests were in the patch!  However, none of the patches
applied any more, so I've updated them and also improved the
tests a bit.  Again, I haven't tried them on Windows.

Unsetting a nonexistent variable isn't supposed to be an error
(see POSIX), but I did find a different problem with checking
unsetenv()'s return value, which is that older systems declare it
as void.  I've removed the check from the patch, mainly because I
don't know how to write the appropriate autoconf test, but it
isn't strictly necessary as long as putenv() can't set a name
that unsetenv() can fail to remove.

I did however find one more case where that can happen, which is
with an environment variable that has an empty name.  Linux at
least allows such variables to be set and passed to new
processes, but its unsetenv() will not remove them - the latter
behaviour is required by POSIX.

To avoid a use-after-free problem similar to the embedded-'='
one, I've made a separate patch to make putenv() raise ValueError
for empty names as well, but it's a more awkward case as Python
may receive such a variable on startup, which it would then be
unable to change (although even without the patch, it's already
unable to remove it - posix.unsetenv() just silently fails).

Checking unsetenv()'s return value would avoid the use-after-free
without having to change putenv(), but it would, for example,
make os.environ.clear() fail if an empty-named variable was
present - which would be correct, since the variable was not
removed, but rather surprising.  To really delete such a variable
would require editing **environ directly, AFAIK.

@David: I couldn't apply the patches directly with tortoisesvn cos of the git format so tried to do them manually but failed.  E.g. in test_os I couldn't find PYTHONTESTVAROS to insert the two new lines after and in test_posix couldn't find PYTHONTESTVARB.  Am I having a bad day at the office or did you have one yesterday? :)

You're having a bad day at the office :)  Just use "patch -p1".

AFAICT, on Windows using the posix_putenv_garbage dict is unnecessary. The Windows C runtime creates a private copy of the string, so there's no need to keep a reference. Moreover, since there's no unsetenv, deleting a variable is accomplished by calling putenv with an empty value, e.g. putenv('foo', ''). This leaks an item in posix_putenv_garbage, which is left set as ('foo', 'foo=').

The part of this issue ('=' in putenv()) is fixed in issue30746.

The original issues seem fixed.

As for leaks in posix_putenv_garbage on Windows, it is better to open a new issue for this.

posix_putenv_garbage no longer used on Windows.

stage: patch review -> resolved

messages: + msg333144

messages: + msg252363
versions: + Python 3.4, Python 3.5, Python 3.6, - Python 3.1, Python 3.2

messages: + msg111503

messages: + msg109614

stage: patch review