classification
Title: [2.7] random: make the file descriptor non-inheritable (on POSIX)
Type: Stage: needs patch
Components: Interpreter Core Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: vstinner Nosy List: Pavel.Labath, alex, emaste, jcea, koobs, ned.deily, pitrou, python-dev, rhettinger, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-02-13 08:31 by vstinner, last changed 2015-04-06 21:41 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
pep446_random.patch vstinner, 2015-02-13 08:31 review
test_fd_status.patch vstinner, 2015-02-13 08:31 review
issue23458_tiger.patch ned.deily, 2015-03-17 08:41 review
Messages (19)
msg235880 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-13 08:31
Attached patch tries to make the private random file descriptor non-inheritable.

It should fix the following issue:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=197376

I tried to write an unit test, but since the PEP 446 is not implemented, unexpected file descriptors are inherited. The test should maybe be run in a subprocess to not inherit all file descriptors created by other unit tests.

Note: I removed the stat.S_ISDOOR(st.st_mode) check from Lib/test/subprocessdata/fd_status.py, because stat.S_ISDOOR is not defined in Python 2.7.
msg235894 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-02-13 15:51
I'm not sure what the point is - there are many child descriptors which may inherited, why care about this one?

The right way to avoid this on 2.7 is to call subprocess.Popen(..., close_fds=True).
msg235895 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-13 15:59
> I'm not sure what the point is - there are many child descriptors which may inherited, why care about this one?

The bug report comes from FreeBSD, https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=197376

They use os.system(), at least in the bug report.

The random file descriptor is a little suprising because it's not obvious than os.urandom() opens a file, nor that the file remains open.

It's also a change in a minor Python version.
msg236015 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-02-15 01:45
+0 This seems like a reasonable change with some upside and no obvious downside.
msg236317 - (view) Author: Ed Maste (emaste) Date: 2015-02-20 18:14
For reference, this fd leak was causing one of LLDB's tests to fail. It is now marked XFAIL pending a resolution of this issue: http://llvm.org/viewvc/llvm-project?view=revision&revision=229704

Linux is also affected, the Linux LLDB tests were previously running on an earlier Python version which did not demonstrate this problem.
msg236491 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-02-24 13:31
New changeset 88a99ea8a4cf by Victor Stinner in branch '2.7':
Issue #23458: On POSIX, the file descriptor kept open by os.urandom() is now
https://hg.python.org/cpython/rev/88a99ea8a4cf
msg236492 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-24 13:32
I simplified the patch on random.c to avoid O_CLOEXEC, there is no need to have an atomic operation.

I also fixed the test: it now uses two subprocesses, to avoid inheritable file descriptors created by the Python test suite.

Thanks for the report.
msg236495 - (view) Author: Pavel Labath (Pavel.Labath) Date: 2015-02-24 13:48
Thanks for fixing this.

Still, I would like to argue that an atomic operation is important. Python interpreter can run embedded in an multi-threaded application (as we do in the case of LLDB). If this is the case, then file descriptors opened by python in one thread are visible to other application threads even though they may have nothing to do with python -- they may not even know that python exists. Now if these other threads do a fork between the time you do open() and fcntl(), you've got yourself a race condition.

Granted, this is not very likely, but this is precisely the reason why O_CLOEXEC was created, and I think we should take advantage of that. And it is not even linux specific, the flag is now recent versions of mac and freebsd too. Therefore I think the original patch is better, even though it may be more complex.
msg236496 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-24 13:54
> Still, I would like to argue that an atomic operation is important.

Yes, and that's why I wrote the whole PEP 446 :-) But we are *not* going to backport the PEP. New features and bug fixes requiring large code change are only done in the development branch, not on stable branches.

Fixing os.urandom() to have atomic operation creating the file descriptor non-inheritable is useless, because they are a *lot* of other functions which create inheritable file descriptors, and a few others which create non-inheritable file descriptor but are not atomic.

You don't have to search too far: Popen.pipe_cloexec() is not atomic...

Reminder: subprocess is not thread-safe => issue #19809 (doc issue).

See my "Bugs already fixed in Python 3" and "Bugs that won’t be fixed in Python 2 anymore" lists:
http://haypo-notes.readthedocs.org/python.html#python-3-is-better-than-python-2

--

I reopen the issue because the test is failing on Windows and OpenIndiana buildbots.
msg236498 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-02-24 14:13
New changeset 8969c7f44d9e by Victor Stinner in branch '2.7':
Issue #23458: skip test_os.test_urandom_fd_non_inheritable() on Windows
https://hg.python.org/cpython/rev/8969c7f44d9e
msg236500 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-24 14:27
Hum, the test also fails on Mac OS X Tiger:

http://buildbot.python.org/all/builders/x86%20Tiger%202.7/builds/2944/steps/test/logs/stdio



======================================================================
FAIL: test_urandom_fd_non_inheritable (test.test_os.URandomTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/db3l/buildarea/2.7.bolen-tiger/build/Lib/test/test_os.py", line 597, in test_urandom_fd_non_inheritable
    self.assertEqual(open_fds - set(range(3)), set())
AssertionError: Items in the first set but not the second:
5


Why do we still maintain these old operating systems? :-p
msg238265 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2015-03-17 08:41
FD_CLOEXEC is first support on OS X 10.5.  Here's a patch to skip the test on earlier systems: tested on 10.4, 10.5, and 10.10.
msg238266 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 09:21
Ned Deily added the comment:
> FD_CLOEXEC is first support on OS X 10.5.  Here's a patch to skip the test on earlier systems: tested on 10.4, 10.5, and 10.10.

What do you mean by "first support"? Does it mean that fcntl(fd, F_SETFD, FD_CLOEXEC) is simply a no-op?

Does Python 3.4 with the PEP 446 works on OS X 10.4? Can you test the following example?

Python 3.4.1 (default, Nov  3 2014, 14:38:10)
[GCC 4.9.1 20140930 (Red Hat 4.9.1-11)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> fd=os.open('.', os.O_RDONLY)
>>> os.get_inheritable(fd)
False
>>> os.set_inheritable(fd, True)
>>> os.get_inheritable(fd)
True
msg238283 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2015-03-17 11:42
$ sw_vers
ProductName:	Mac OS X
ProductVersion:	10.4.11
BuildVersion:	8S165
$ ./python
Python 3.4.3+ (3.4:910a7a540a31, Mar 17 2015, 03:33:01)
[GCC 4.0.1 (Apple Computer, Inc. build 5370)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> fd=os.open('.', os.O_RDONLY)
>>> os.get_inheritable(fd)
False
>>> os.set_inheritable(fd, True)
>>> os.get_inheritable(fd)
True

But if I port test_urandom_fd_non_inheritable from 2.7, it fails on 3.4 as well.
msg238291 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 13:50
Ok, so Python 3.4 works as expected on Mac OS X 10.4. It's probably because Python 3.4 uses ioctl() if available. Maybe ioctl() works, but not fcntl().

os.urandom() only *tries* to make the file descriptor non-inheritable. It would be possible to backport some features of the PEP 446 but the code would be much more complex.

Please apply issue23458_tiger.patch, it's ok to skip the test.
msg238351 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-03-17 22:18
New changeset 730bbd1499ba by Ned Deily in branch '2.7':
Issue #23458: Skip test_urandom_fd_non_inheritable on OS X 10.4 since
https://hg.python.org/cpython/rev/730bbd1499ba
msg238371 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2015-03-18 01:01
The buildbot is now green -> closed.
msg239474 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-29 06:34
The test_urandom_fd_non_inheritable is constantly failed on AMD64 OpenIndiana 2.7 buildbot.

http://buildbot.python.org/all/builders/AMD64%20OpenIndiana%202.7/builds/2727/steps/test/logs/stdio
======================================================================
FAIL: test_urandom_fd_non_inheritable (test.test_os.URandomTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/export/home/buildbot/64bits/2.7.cea-indiana-amd64/build/Lib/test/test_support.py", line 387, in wrapper
    return func(*args, **kw)
  File "/export/home/buildbot/64bits/2.7.cea-indiana-amd64/build/Lib/test/test_os.py", line 601, in test_urandom_fd_non_inheritable
    self.assertEqual(open_fds - set(range(3)), set())
AssertionError: Items in the first set but not the second:
4

----------------------------------------------------------------------
msg240189 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-04-06 21:41
New changeset 1b509a7e3b99 by Victor Stinner in branch '2.7':
Issue #23458: Remove test_os.test_urandom_fd_non_inheritable()
https://hg.python.org/cpython/rev/1b509a7e3b99
History
Date User Action Args
2015-04-06 21:41:49vstinnersetstatus: open -> closed
resolution: fixed
2015-04-06 21:41:23python-devsetmessages: + msg240189
2015-03-31 00:00:58ned.deilysetstatus: closed -> open
resolution: fixed -> (no value)
stage: resolved -> needs patch
2015-03-29 06:34:17serhiy.storchakasetnosy: + jcea
messages: + msg239474
2015-03-18 01:01:04ned.deilysetstatus: open -> closed
resolution: fixed
messages: + msg238371

stage: commit review -> resolved
2015-03-17 22:18:55python-devsetmessages: + msg238351
2015-03-17 13:50:05vstinnersetmessages: + msg238291
2015-03-17 11:42:36ned.deilysetmessages: + msg238283
2015-03-17 09:21:12vstinnersetmessages: + msg238266
2015-03-17 08:41:39ned.deilysetfiles: + issue23458_tiger.patch

nosy: + ned.deily
messages: + msg238265

stage: commit review
2015-02-28 09:27:28serhiy.storchakasetassignee: vstinner
2015-02-24 14:27:35vstinnersetmessages: + msg236500
2015-02-24 14:13:25python-devsetmessages: + msg236498
2015-02-24 13:54:23vstinnersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg236496
2015-02-24 13:48:16Pavel.Labathsetmessages: + msg236495
2015-02-24 13:32:10vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg236492
2015-02-24 13:31:02python-devsetnosy: + python-dev
messages: + msg236491
2015-02-24 10:09:31Pavel.Labathsetnosy: + Pavel.Labath
2015-02-20 18:14:35emastesetmessages: + msg236317
2015-02-20 17:57:53serhiy.storchakasetnosy: + serhiy.storchaka
2015-02-20 17:39:46emastesetnosy: + emaste
2015-02-15 01:45:50rhettingersetnosy: + rhettinger
messages: + msg236015
2015-02-13 15:59:57vstinnersetmessages: + msg235895
2015-02-13 15:51:55pitrousetmessages: + msg235894
2015-02-13 09:27:45vstinnersetversions: + Python 2.7, - Python 3.4, Python 3.5
2015-02-13 08:31:58vstinnersetnosy: + koobs
2015-02-13 08:31:52vstinnersetfiles: + test_fd_status.patch
2015-02-13 08:31:46vstinnercreate