Title: putenv() accepts names containing '=', return value of unsetenv() not checked
Type: behavior Stage: patch review
Components: Extension Modules Versions: Python 3.6, Python 3.5, Python 3.4, Python 2.7
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, baikie, eryksun, ggenellina, loewis, pefu, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2009-01-12 23:30 by baikie, last changed 2017-06-25 06:52 by serhiy.storchaka.

File name Uploaded Description Edit
visibility.diff baikie, 2009-01-12 23:30 Make bugs visible for testing
2.x.diff baikie, 2009-01-12 23:31 Fix for Python 2.x
3.x.diff baikie, 2009-01-12 23:31 Fix for Python 3.x
putenv-equals-2.x.diff baikie, 2010-07-24 19:20 Make posix.putenv() raise ValueError if name contains an '=' character
putenv-empty-2.x.diff baikie, 2010-07-24 19:21 Make posix.putenv() raise ValueError if name is empty
putenv-equals-3.x.diff baikie, 2010-07-24 19:21 Make posix.putenv() raise ValueError if name contains an '=' character
putenv-empty-3.x.diff baikie, 2010-07-24 19:22 Make posix.putenv() raise ValueError if name is empty
visibility-2.x.diff baikie, 2010-07-24 19:24 Make posix_putenv_garbage visible, print unsetenv() return value
visibility-3.x.diff baikie, 2010-07-24 19:24 Make posix_putenv_garbage visible, print unsetenv() return value
Pull Requests
URL Status Linked Edit
PR 2382 serhiy.storchaka, 2017-06-24 19:11
Messages (8)
msg79708 - (view) Author: David Watson (baikie) Date: 2009-01-12 23:30
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
msg109614 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-08 21:31
As patches were originally provided would someone kindly review them.
msg111036 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-07-21 10:52
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
msg111503 - (view) Author: David Watson (baikie) Date: 2010-07-24 19:20
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.
msg111535 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-25 11:40
@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? :)
msg111549 - (view) Author: David Watson (baikie) Date: 2010-07-25 18:23
You're having a bad day at the office :)  Just use "patch -p1".
msg252363 - (view) Author: Eryk Sun (eryksun) * Date: 2015-10-05 22:34
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=').
msg296815 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-25 06:52
The part of this issue ('=' in putenv()) is fixed in issue30746.
Date User Action Args
2017-06-25 06:52:29serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg296815
2017-06-24 19:11:40serhiy.storchakasetpull_requests: + pull_request2432
2015-10-05 22:34:54eryksunsetnosy: + eryksun

messages: + msg252363
versions: + Python 3.4, Python 3.5, Python 3.6, - Python 3.1, Python 3.2
2015-10-05 08:16:48pefusetnosy: + pefu
2014-02-03 19:19:48BreamoreBoysetnosy: - BreamoreBoy
2010-07-25 18:23:56baikiesetmessages: + msg111549
2010-07-25 11:40:57BreamoreBoysetmessages: + msg111535
2010-07-24 19:24:30baikiesetfiles: + visibility-3.x.diff
2010-07-24 19:24:11baikiesetfiles: + visibility-2.x.diff
2010-07-24 19:22:22baikiesetfiles: + putenv-empty-3.x.diff
2010-07-24 19:21:46baikiesetfiles: + putenv-equals-3.x.diff
2010-07-24 19:21:10baikiesetfiles: + putenv-empty-2.x.diff
2010-07-24 19:20:32baikiesetfiles: + putenv-equals-2.x.diff

messages: + msg111503
2010-07-21 10:52:00amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg111036
2010-07-08 21:31:56BreamoreBoysetversions: + Python 3.1, Python 2.7, Python 3.2
nosy: + loewis, BreamoreBoy

messages: + msg109614

stage: patch review
2009-01-13 04:36:49ggenellinasetnosy: + ggenellina
2009-01-12 23:31:49baikiesetfiles: + 3.x.diff
2009-01-12 23:31:27baikiesetfiles: + 2.x.diff
2009-01-12 23:30:21baikiecreate