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.

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

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

+0 This seems like a reasonable change with some upside and no obvious downside.

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.

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

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.

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.

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

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

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

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.

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

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

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.

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

The buildbot is now green -> closed.

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

----------------------------------------------------------------------

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