Repository navigation
putenv() accepts names containing '=', return value of unsetenv() not checked #49176
Description
| BPO | 4926 |
|---|---|
| Nosy | @loewis, @pefu, @amauryfa, @serhiy-storchaka, @eryksun |
| PRs | |
| Files |
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:
assignee = None
closed_at = <Date 2020-03-22.09:27:46.810>
created_at = <Date 2009-01-12.23:30:21.260>
labels = ['extension-modules', 'type-bug']
title = "putenv() accepts names containing '=', return value of unsetenv() not checked"
updated_at = <Date 2020-03-22.09:27:46.810>
user = 'https://bugs.python.org/baikie'bugs.python.org fields:
activity = <Date 2020-03-22.09:27:46.810>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2020-03-22.09:27:46.810>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2009-01-12.23:30:21.260>
creator = 'baikie'
dependencies = []
files = ['12708', '12709', '12710', '18185', '18186', '18187', '18188', '18189', '18190']
hgrepos = []
issue_num = 4926
keywords = ['patch']
message_count = 10.0
messages = ['79708', '109614', '111036', '111503', '111535', '111549', '252363', '296815', '333144', '364791']
nosy_count = 7.0
nosy_names = ['loewis', 'pefu', 'amaury.forgeotdarc', 'ggenellina', 'baikie', 'serhiy.storchaka', 'eryksun']
pr_nums = ['2382']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue4926'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5', 'Python 3.6']
Activity
baikie commentedon Jan 12, 2009
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.
BreamoreBoy commentedon Jul 8, 2010
As patches were originally provided would someone kindly review them.
amauryfa commentedon Jul 21, 2010
The patch actually does 2 things:
Also, some unit tests would be appreciated
baikie commentedon Jul 24, 2010
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.
BreamoreBoy commentedon Jul 25, 2010
@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? :)
baikie commentedon Jul 25, 2010
You're having a bad day at the office :) Just use "patch -p1".
eryksun commentedon Oct 5, 2015
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=').
serhiy-storchaka commentedon Jun 25, 2017
The part of this issue ('=' in putenv()) is fixed in bpo-30746.
serhiy-storchaka commentedon Jan 7, 2019
The original issues seem fixed.
As for leaks in posix_putenv_garbage on Windows, it is better to open a new issue for this.
serhiy-storchaka commentedon Mar 22, 2020
posix_putenv_garbage no longer used on Windows.