Issue 12105: open() does not able to set flags, such as O_CLOEXEC (original) (raw)
Created on 2011-05-18 19:11 by socketpair, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (49)
Author: Марк Коренберг (socketpair) *
Date: 2011-05-18 19:11
In Python3 I'm not able to use open('xxx', 're') as it does not call fopen() anymore. What about extra flags like 'e'=O_CLOEXEC?
P.S. see for list of needed flags
P.P.S does 2to3 tool properly detect such cases?
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * 
Date: 2011-05-18 20:57
These "needed" flags are actually nonstandard extensions of the libc fopen() function on some platforms.
In python3, one can still use fcntl(f.fileno(), FD_SET, FD_CLOEXEC), but a "flags" parameter would be more convenient.
Author: Charles-François Natali (neologix) *
Date: 2011-05-18 21:53
In python3, one can still use fcntl(f.fileno(), FD_SET, FD_CLOEXEC)
Note that it's not atomic.
Author: Charles-François Natali (neologix) *
Date: 2011-05-19 18:38
Here's a patch adding O_CLOEXEC to the os module, with test. This patch makes it possible to open and set a FD CLOEXEC atomically. O_CLOEXEC is part of POSIX.1-2008, supported by the Linux kernel since 2.6.23 and has been committed recently to FreeBSD. Note that I'm not sure that adding this flag to built-in open() is necessarily a good idea, because it's not portable and low-level. The same functionality can be more or less achieved with: f = os.fdopen(os.open('/etc/fstab', os.O_RDONLY|os.O_CLOEXEC))
Author: STINNER Victor (vstinner) *
Date: 2011-05-19 19:15
Using spawn_python() to check that os.O_CLOEXEC flag is correctly set seems overkill. Why not just testing fcntl.fcntl(f.fileno(), fcntl.F_GETFL) & FD_CLOEXEC)? I don't think that there are OSes with O_CLOEXEC but without fcntl(F_GETFL).
Note that I'm not sure that adding this flag to built-in open() is necessarily a good idea
I agree.
open() documentation may explain the os.fdopen(os.open()) "trick" to use low-level options like O_SYNC or O_CLOEXEC.
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *
Date: 2011-05-19 19:18
+1 for the patch! Note that fdopen is now a simple call to open(), so theses lines are equivalent:
python2.7: open(filename, 're') python3.3: open(os.open(filename, os.O_RDONLY|os.O_CLOEXEC))
Author: Charles-François Natali (neologix) *
Date: 2011-05-19 20:34
Using spawn_python() to check that os.O_CLOEXEC flag is correctly set seems overkill. Why not just testing fcntl.fcntl(f.fileno(), fcntl.F_GETFL) & FD_CLOEXEC)?
Because I couldn't find a place where the CLOEXEC flag was fully tested (I mean, checking in the child process that the FD was correctly closed), so I took the opportunity to test it thoroughly here. But you're right it's maybe a little bit overkill, so here's a patch using just F_GETFL. Pick up whichever you like.
Note that I'm not sure that adding this flag to built-in open() is necessarily a good idea
I agree.
OK.
open() documentation may explain the os.fdopen(os.open()) "trick" to use low-level options like O_SYNC or O_CLOEXEC.
Why not, but I leave it to someone more comfortable with documentation than me :-)
Author: Марк Коренберг (socketpair) *
Date: 2011-05-20 04:55
Why not to implement 'e' letter in py3k ?
In systems where O_CLOEXEC is not supported in open(), flag should be set non-atomically using fcntl.
To exclude races (in concurrent threads), this two ops should be done under lock (GIL?)
Author: STINNER Victor (vstinner) *
Date: 2011-05-20 08:52
Why not to implement 'e' letter in py3k ?
In systems where O_CLOEXEC is not supported in open(), flag should be set non-atomically using fcntl.
Having an atomic or non-atomic behaviour depending on the OS is not a good idea.
Author: Charles-François Natali (neologix) *
Date: 2011-05-20 09:18
To exclude races (in concurrent threads), this two ops should be done under lock (GIL?)
That won't work, because open(), like other slow syscalls, is called without the GIL held. Furthermore, it wouldn't be atomic anyway (imagine a fork is done from a C extension without the GIL held). So we would end up tricking people into using a 'e' flag that, contrarily to GNU fopen(), would not be atomic. Since the fopen() 'e' flag is only available on platform supporting O_CLOEXEC, you're exactly as portable using the fdopen() trick. And you're sure of what's happening.
Author: STINNER Victor (vstinner) *
Date: 2011-05-22 13:56
@charles-francois.natali: Your patch is ok, you can commit it into 3.1, 3.2, 3.3. But you may wait after 3.2.1.
@Georg Brandl: Should we wait after Python 3.2.1 for this issue?
Author: STINNER Victor (vstinner) *
Date: 2011-05-22 14:00
open() documentation may explain the os.fdopen(os.open()) "trick" to use low-level options like O_SYNC or O_CLOEXEC.
Why not, but I leave it to someone more comfortable with documentation than me :-)
Issue #12103 should be fine for this idea.
Author: STINNER Victor (vstinner) *
Date: 2011-05-22 14:07
Oh, by the way: it would also be nice to add os.O_CLOEXEC to Python 2.7.
For example, tempfile._mkstemp_inner() uses: fd = _os.open(file, flags, 0600) _set_cloexec(fd) # fcntl(fd, F_SETFD, flags | FD_CLOEXEC) which is not atomic.
Author: Georg Brandl (georg.brandl) *
Date: 2011-05-22 15:07
One moment -- adding a new value to the os module looks like a new feature to me. Is there any convincing reason why this needs to go to 3.2? (And it most definitely shouldn't go to 3.1.)
Author: Antoine Pitrou (pitrou) *
Date: 2011-05-22 15:20
No reason. I think this is definitely 3.3 material.
Author: Roundup Robot (python-dev) 
Date: 2011-05-22 18:41
New changeset f1c544245eab by Charles-François Natali in branch 'default': Issue #12105: Add O_CLOEXEC to the os module. http://hg.python.org/cpython/rev/f1c544245eab
Author: Charles-François Natali (neologix) *
Date: 2011-05-22 18:45
I've committed the patch to 3.3. Since the documentation aspect is traced in Issue #12103, I'm closing this issue. Марк, thanks for reporting this!
Author: Antoine Pitrou (pitrou) *
Date: 2011-05-22 19:13
And apparently some buildbot doesn't like it: http://www.python.org/dev/buildbot/all/builders/x86%20Gentoo%20Non-Debug%203.x/builds/57/
====================================================================== FAIL: test_oscloexec (test.test_posix.PosixTester)
Traceback (most recent call last): File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/test/test_posix.py", line 315, in test_oscloexec self.assertTrue(fcntl.fcntl(fd, fcntl.F_GETFD) & fcntl.FD_CLOEXEC) AssertionError: 0 is not true
Author: STINNER Victor (vstinner) *
Date: 2011-05-22 20:19
One moment -- adding a new value to the os module looks like a new feature to me. Is there any convincing reason why this needs to go to 3.2? (And it most definitely shouldn't go to 3.1.)
Python doesn't suppose atomic open+CLOEXEC anymore, I consider this as a regression from Python 2 (which support open("re") with the GNU libc). Because the patch is simple, I think that it can go in 3.1 and 3.2. Am I wrong? But... it tooks some years until someone noticed this regression.
Can we add new features to old releases?
Author: Antoine Pitrou (pitrou) *
Date: 2011-05-22 20:25
Python doesn't suppose atomic open+CLOEXEC anymore, I consider this as a regression from Python 2 (which support open("re") with the GNU libc).
It has never been documented (nor supported) so, no, I wouldn't consider it a regression.
But... it tooks some years until someone noticed this regression.
Which means it's certainly unimportant.
Can we add new features to old releases?
Well, you already know the answer, don't you? :)
Author: Charles-François Natali (neologix) *
Date: 2011-05-22 20:37
And apparently some buildbot doesn't like it
Linux-2.6.22-vs2.2.0.7-gentoo-i686-Intel-R-_Xeon-TM-_CPU_2.80GHz-with-gentoo-2.0.1 little-endian
O_CLOEXEC support was added to Linux 2.6.23: this means that the libc defines it while the kernel doesn't support it (it's probably defined as 0). I could either add some voodoo configure checks to ensure that O_CLOEXEC is indeed supported, or just skip the test on Linux kernels older than 2.6.23. I prefer the later option, it's much simpler and less error-prone. Sure, it will make O_CLOEXEC available while the kernel doesn't support it on some configurations, but since it's a libc issue...
Author: Roundup Robot (python-dev)
Date: 2011-05-23 10:54
New changeset bff9265d677d by Victor Stinner in branch 'default': Issue #12105: test_posix, add the value of O_CLOEXEC in the error message http://hg.python.org/cpython/rev/bff9265d677d
Author: STINNER Victor (vstinner) *
Date: 2011-05-23 10:55
or just skip the test on Linux kernels older than 2.6.23
I like this solution, but I don't know how to test that the kernel doesn't support O_CLOEXEC. My commit bff9265d677d will tell use the value of O_CLOEXEC on the "Linux-2.6.22-vs2.2.0.7-gentoo-i686-Intel-R-_Xeon-TM-_CPU_2.80GHz-with-gentoo-2.0.1 little-endian" buildbot.
Author: Antoine Pitrou (pitrou) *
Date: 2011-05-23 11:00
I like this solution, but I don't know how to test that the kernel doesn't support O_CLOEXEC. My commit bff9265d677d will tell use the value of O_CLOEXEC on the "Linux-2.6.22-vs2.2.0.7-gentoo-i686-Intel-R-_Xeon-TM-_CPU_2.80GHz-with-gentoo-2.0.1 little-endian" buildbot.
If O_CLOEXEC is a #defined constant in the glibc, it can't be different from other kernels.
Author: STINNER Victor (vstinner) *
Date: 2011-05-23 11:19
Story of O_CLOEXEC in the GNU libc, by Ulrich Drepper: "Secure File Descriptor Handling" http://udrepper.livejournal.com/20407.html
--
I could either add some voodoo configure checks to ensure that O_CLOEXEC is indeed supported
Hum, if I patch tempfile._mkstemp_inner() to use os.O_CLOEXEC if available, whereas this flag has no effect, the function will be less secure on Linux 2.6.22 (if the GNU libc exposes O_CLOEXEC).
Check O_CLOEXEC is configure is not safe if the host compiling Python uses a different kernel that the host running Python (e.g. think of Linux distro: Python is compiled on a different computer).
We need maybe a flag to indicate that O_CLOEXEC is supported by the running kernel or not. Or maybe a function to check it at least?
--
O_CLOEXEC was added to Ruby and then removed because the flag is sometimes ignored: http://redmine.ruby-lang.org/issues/1291 http://redmine.ruby-lang.org/projects/ruby-19/repository/revisions/31643
"because boron chkbuild test result says, An old linux kernel ignore O_CLOEXEC silently instead of return an error. It may lead to bring new security risk. So, we have to be pending it until finish to implement proper fallback logic."
--
Read also the discussion about O_CLOEXEC on bugs-findutils mailing list: "it's not a guarantee that the O_CLOEXEC actually had an effect"
Their patch uses :
static int fd_is_cloexec (int fd) { const int flags = fcntl (fd, F_GETFD); return (flags & FD_CLOEXEC) || (fcntl (fd, F_GETFL) & O_CLOEXEC); }
static bool o_cloexec_works (void) { bool result = false; int fd = open ("/", O_RDONLY|O_CLOEXEC); if (fd >= 0) { result = fd_is_cloexec (fd); close (fd); } return result; }
--
Oh, there is also SOCK_CLOEXEC, which may be useful for #5715.
Author: STINNER Victor (vstinner) *
Date: 2011-05-23 11:50
Another idea is to write a best-effort function to open a file with CLOEXEC flag:
- use O_CLOEXEC if available
- use fcntl(fd, F_SETFD, flags | FD_CLOEXEC) if O_CLOEXEC is missing or was silently ignored by the kernel (by open)
Attached open_cloexec.py is an implementation.
--
Usage of "CLOEXEC" in the Python standard library:
- subprocess: create pipe. use pipe2() or pipe()+fcntl(FD_CLOEXEC)
- test_socket: create a socket. use SOCK_CLOEXEC. The test is skipped if the kernel is Linux < 2.6.28. It has a nice linux_version() which should be moved to the platform module.
- test_posix: check open(O_CLOEXEC).
- test_tempfile: test "cloexec" behaviour using os.spawnl()
- xmlrpclib: use FD_CLOEXEC on the socket
You may also add pipe_cloexec() to os, and socket_cloexec() to socket?
Author: STINNER Victor (vstinner) *
Date: 2011-05-23 11:52
My commit bff9265d677d will tell use the value of O_CLOEXEC on the "Linux-2.6.22-vs2.2.0.7-gentoo-..." buildbot.
Here you have: "AssertionError: 0 is not true : CLOEXEC flag not set (O_CLOEXEC=0x80000)"
It's the same value on my Debian Sid. So we cannot test O_CLOEXEC value, we have to check the kernel version.
Author: Charles-François Natali (neologix) *
Date: 2011-05-23 12:06
The real issue is that the libc defines O_CLOEXEC, but kernels prior to 2.6.23 don't support it: instead of returning EINVAL, the socket syscall silently ignores the flag (don't know why I made the comment about this flag being defined to 0...). IMHO this is really a distribution packaging bug (kernel/libc mismatch), and a quite serious one. I don't think we should be fixing this in Python, but that we should merely skip this test on kernels older than 2.6.23. People should complain to their distributions vendors instead (I tested this on RHEL4, RHEL6 and Debian Squeeze without problem).
I personally don't like the idea of a best-effort O_CLOEXEC implementation: providing a O_CLOEXEC flag which is not atomic feels really wrong to me.
Author: STINNER Victor (vstinner) *
Date: 2011-05-23 12:11
The real issue is that the libc defines O_CLOEXEC, but kernels prior to 2.6.23 don't support it: instead of returning EINVAL, the socket syscall silently ignores the flag (don't know why I made the comment about this flag being defined to 0...).
This is a kernel bug, not a bug in the GNU libc (ask Ulrich if you are not sure ;-)). An host can have multiple kernel versions (and choose at boot time using GRUB/LILO/...), but it has usually only one C library. You cannot change the version of the libc depending on the kernel.
If you really want to fix this problem, you will have to patch kernel < 2.6.23. Good luck!
Or we can workaround kernel bugs providing a documentation and/or functions for that.
Author: STINNER Victor (vstinner) *
Date: 2011-05-23 12:26
test_socket: ...has a nice linux_version() which should be moved to the platform module
I created the issue #12158 for that.
Author: Charles-François Natali (neologix) *
Date: 2011-05-23 12:27
This is a kernel bug, not a bug in the GNU libc (ask Ulrich if you are not sure ;-)).
Kernels prior to 2.6.23 didn't know about the O_CLOEXEC flag: to catch this kind of problem, every syscall would have to check every bit when it's passed a combination of flags. This would be clumsy, error-prone and slow. It's not a libc bug either. The problem is really a distribution issue: using a libc defining a flag unsupported by the kernel is really calling for trouble.
An host can have multiple kernel versions (and choose at boot time using GRUB/LILO/...)
It's possible, but it's definitely a bad idea, because of such API mismatch. For example nothing prevents a syscall from being removed/modified from one kernel version to another. If your libc doesn't follow, you're up for trouble. Try using futexes with a kernel not supporting them :-)
Author: Charles-François Natali (neologix) *
Date: 2011-05-23 21:16
@Victor Since linux_version() won't be added to the platform module, could you add it to test.support so that it can be used in the O_CLOEXEC test?
Author: Roundup Robot (python-dev)
Date: 2011-05-23 22:29
New changeset 9a16fa0c9548 by Victor Stinner in branch 'default': Issue #12105: test_posix skips test_oscloexec() on Linux < 2.6.23 http://hg.python.org/cpython/rev/9a16fa0c9548
Author: Charles-François Natali (neologix) *
Date: 2011-05-24 16:11
Checking the kernel version did the trick, the test now run fines on the buildbots. Thanks Victor. Re-closing.
Author: Марк Коренберг (socketpair) *
Date: 2011-09-06 08:42
O_CLOEXEC is not linux-only. Windows has the same flag. In file-opening functions there is lpSecurityAttributes argument. And there is bInheritHandle member of corresponding structure.
http://msdn.microsoft.com/en-us/library/aa379560(v=VS.85).aspx : bInheritHandle A Boolean value that specifies whether the returned handle is inherited when a new process is created. If this member is TRUE, the new process inherits the handle.
So, why not to implement 'e' letter in open()? it is true crossplatform. On platforms where inheritance does not work, flag should be ignored.
P.S. Moreover, I think that all file-descriptor-crete functions (open, socket, pipe, dup, ...) should set CLOEXEC atomically
Author: Alexey Smirnov (alexey-smirnov)
Date: 2011-09-06 10:09
FreeBSD 8+ also supports O_CLOEXEC in open().
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *
Date: 2011-09-06 11:29
O_CLOEXEC is not linux-only. Windows has the same flag. In file-opening functions there is lpSecurityAttributes argument
How do you suggest to use it? Even on Windows, python calls open(). And lpSecurityAttributes is an argument of CreateFile (which is the win32 kernel function that open() calls)
Author: STINNER Victor (vstinner) *
Date: 2011-09-06 11:37
Windows provides a _get_osfhandle() function. There is not the opposite function? :-)
Anyway, O_CLOEXEC is not available on all platforms. Even on FreeBSD and Linux, it depends on the OS/kernel version.
Author: Марк Коренберг (socketpair) *
Date: 2011-09-06 12:29
Some times ago, Python has used fopen() for open() implementation. Now, it uses OS-kernel native function to open files. AFAIK, open() in Windows is a wrapper around CreateFile, created to support some POSIX programs in Windows. Why not to use CreateFile() on Windows platform?
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *
Date: 2011-09-06 12:40
Why not to use CreateFile() on Windows platform? Good idea! Please open a separate issue for it.
Author: STINNER Victor (vstinner) *
Date: 2011-09-08 23:05
See also issue #12760 (Add create mode to open).
Author: STINNER Victor (vstinner) *
Date: 2011-09-08 23:22
Why not to use CreateFile() on Windows platform? Good idea! Please open a separate issue for it.
Done, issue #12939.
Author: Марк Коренберг (socketpair) *
Date: 2011-11-01 04:56
Why it is closed as duplicate? nothing said about CLOEXEC in
Author: Charles-François Natali (neologix) *
Date: 2011-11-01 07:27
Why it is closed as duplicate? nothing said about CLOEXEC in
See http://bugs.python.org/issue12760#msg146686
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *
Date: 2011-11-01 08:33
And to be explicit, you can now write:
def open_cloexex(filename, mode='r'): return open(filename, mode, opener=lambda path, mod: os.open(path, mod|os.O_CLOEXEC))
Author: Марк Коренберг (socketpair) *
Date: 2011-11-01 11:55
Well, I understand. So why not to add 'e' (and 'N', which is the same meaning) character, which:
- use O_CLOEXEC in modern Linux
- generate Exception if O_CLOEXEC is not supported (or does not work) on platform.
Also, implement O_CLOEXEC for open() in Windows using appropriate securityattributes with CreateFile()
?
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *
Date: 2011-11-01 12:07
So why not to add 'e' character You said it: because it can't be written consistently on all platforms. For example, python does not use CreateFile on Windows, see #12939.
Author: Richard Oudkerk (sbt) *
Date: 2013-01-03 12:45
Note that on Windows there is an O_NOINHERIT flag which almost corresponds to O_CLOEXEC on Linux.
I don't think there is a need to use the win32 api.
Author: STINNER Victor (vstinner) *
Date: 2013-01-03 15:24
Note that on Windows there is an O_NOINHERIT flag which almost corresponds to O_CLOEXEC on Linux. I don't think there is a need to use the win32 api.
Ah yes. Because this issue is closed, I created the issue #16850 which is more specific to open + close-and-exec.
History
Date
User
Action
Args
2022-04-11 14:57:17
admin
set
github: 56314
2013-01-03 15:24:48
vstinner
set
messages: +
2013-01-03 12:45:05
sbt
set
nosy: + sbt
messages: +
2011-11-01 12:07:17
amaury.forgeotdarc
set
messages: +
2011-11-01 11:55:57
socketpair
set
messages: +
2011-11-01 08:33:02
amaury.forgeotdarc
set
messages: +
2011-11-01 07:27:44
neologix
set
messages: +
2011-11-01 04:56:31
socketpair
set
messages: +
2011-10-30 21:15:54
vstinner
set
resolution: fixed -> duplicate
2011-10-29 15:57:50
neologix
set
status: open -> closed
superseder: io.FileIO and io.open should support openat
2011-09-08 23:22:23
vstinner
set
messages: +
2011-09-08 23:05:30
vstinner
set
messages: +
2011-09-06 12:40:47
amaury.forgeotdarc
set
messages: +
2011-09-06 12:29:11
socketpair
set
messages: +
2011-09-06 11:37:40
vstinner
set
messages: +
2011-09-06 11:29:33
amaury.forgeotdarc
set
messages: +
2011-09-06 10:09:43
alexey-smirnov
set
messages: +
2011-09-06 08:42:54
socketpair
set
status: closed -> open
messages: +
2011-05-24 16:11:21
neologix
set
status: open -> closed
messages: +
2011-05-23 22:29:48
python-dev
set
messages: +
2011-05-23 21:16:33
neologix
set
messages: +
2011-05-23 12:27:08
neologix
set
messages: +
2011-05-23 12:26:11
vstinner
set
messages: +
2011-05-23 12:11:27
vstinner
set
messages: +
2011-05-23 12:06:39
neologix
set
messages: +
2011-05-23 11:52:31
vstinner
set
messages: +
2011-05-23 11:50:56
vstinner
set
files: + open_cloexec.py
messages: +
2011-05-23 11:19:13
vstinner
set
messages: +
2011-05-23 11:00:54
pitrou
set
messages: +
2011-05-23 10:55:46
vstinner
set
messages: +
2011-05-23 10:54:59
python-dev
set
messages: +
2011-05-22 20:37:10
neologix
set
messages: +
2011-05-22 20:25:23
pitrou
set
messages: +
2011-05-22 20:19:06
vstinner
set
messages: +
2011-05-22 19:13:40
pitrou
set
status: closed -> open
assignee: neologix
messages: +
2011-05-22 18:45:09
neologix
set
status: open -> closed
resolution: accepted -> fixed
messages: +
stage: commit review -> resolved
2011-05-22 18:41:18
python-dev
set
nosy: + python-dev
messages: +
2011-05-22 15:20:53
pitrou
set
nosy: + pitrou
messages: +
versions: - Python 3.1, Python 3.2, Python 3.4
2011-05-22 15:07:18
georg.brandl
set
messages: +
2011-05-22 14:07:00
vstinner
set
messages: +
2011-05-22 14:00:29
vstinner
set
messages: +
2011-05-22 13:56:40
vstinner
set
nosy: + georg.brandl
messages: +
2011-05-20 09:33:26
petri.lehtinen
set
nosy: + petri.lehtinen
2011-05-20 09🔞10
neologix
set
messages: +
2011-05-20 08:52:25
vstinner
set
messages: +
2011-05-20 04:55:46
socketpair
set
messages: +
2011-05-19 20:34:19
neologix
set
files: + os_cloexec_1.diff
messages: +
2011-05-19 19🔞52
amaury.forgeotdarc
set
resolution: accepted
messages: +
stage: commit review
2011-05-19 19:15:32
vstinner
set
messages: +
2011-05-19 18:38:54
neologix
set
files: + os_cloexec.diff
nosy: + vstinner
messages: +
keywords: + patch
2011-05-19 05:53:26
alexey-smirnov
set
nosy: + alexey-smirnov
2011-05-18 21:53:13
neologix
set
nosy: + neologix
messages: +
2011-05-18 20:57:08
amaury.forgeotdarc
set
nosy: + amaury.forgeotdarc
messages: +
2011-05-18 19:11:54
socketpair
set
components: + Library (Lib)
2011-05-18 19:11:28
socketpair
create