classification
Title: Use O_CLOEXEC in the tempfile module
Type: behavior Stage: committed/rejected
Components: Library (Lib) Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, neologix, python-dev
Priority: normal Keywords: needs review, patch

Created on 2013-01-04 14:22 by haypo, last changed 2013-03-03 17:55 by neologix. This issue is now closed.

Files
File name Uploaded Description Edit
tempfile_cloexec.diff neologix, 2013-01-04 14:29 review
Messages (10)
msg179023 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-01-04 14:22
os.O_CLOEXEC has been added to Python 3.3. This flag solves a race condition if the process is forked between open() and a call to fcntl() to set the FD_CLOEXEC flag.

The following patch written by neologix should fix this issue:
"""
diff --git a/Lib/tempfile.py b/Lib/tempfile.py
--- a/Lib/tempfile.py
+++ b/Lib/tempfile.py
@@ -57,6 +57,8 @@
 _allocate_lock = _thread.allocate_lock

 _text_openflags = _os.O_RDWR | _os.O_CREAT | _os.O_EXCL
+if hasattr(_os, 'O_CLOEXEC'):
+    _text_openflags |= _os.O_CLOEXEC
 if hasattr(_os, 'O_NOINHERIT'):
     _text_openflags |= _os.O_NOINHERIT
 if hasattr(_os, 'O_NOFOLLOW'):
"""

The patch can be applied to Python 3.3 and 3.4.
msg179025 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-01-04 14:23
See also #16850 which proposes to expose O_CLOEXEC feature in the open() builtin function.
msg179026 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-01-04 14:29
Here's the patch.
It also removes O_NOFOLLOW, which is basically useless (if the file is created with O_CREAT|O_EXCL, then by definition it's not a symlink).
msg179029 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-01-04 14:35
> Here's the patch.

_set_cloexec() is still called whereas it is useless if the OS supports O_CLOEXEC... But the call must be kept because Linux < 2.6.23 just ignores O_CLOEXEC: we would have to check _fcntl.fcntl(fd, _fcntl.F_GETFD, 0) & _fcntl.FD_CLOEXEC to check if the kernel does really support O_CLOEXEC, which is overkill. The possibly useless syscall doesn't hurt.

> (if the file is created with O_CREAT|O_EXCL, then by definition it's not a symlink).

Ah yes, because of O_EXCL.

The patch looks good to me!
msg179053 - (view) Author: Roundup Robot (python-dev) Date: 2013-01-04 17:33
New changeset 64883c614c88 by Charles-François Natali in branch 'default':
Issue #16860: In tempfile, use O_CLOEXEC when available to set the
http://hg.python.org/cpython/rev/64883c614c88
msg179054 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-01-04 17:36
I've committed it only to default, since it's not really a bug, but rather an improvement (if we did consider this a "security" bug then it should also be backported to 2.7, 3.1, etc).

I'll wait a little before removing O_NOFOLLOW: I'm 99% sure it's useless, but just in case someone finds a really subtle reason to keep it...
msg179837 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-01-12 23:29
> I'll wait a little before removing O_NOFOLLOW

I don't know this flag. What is its effect of the directory part of the path? Does it change anything if the directory is a symbolic link?
msg179840 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-01-12 23:46
>> I'll wait a little before removing O_NOFOLLOW
>
> I don't know this flag. What is its effect of the directory part of the path? Does it change anything if the directory is a symbolic link?

No, it only has an effect if the target file is a symlink.
FWIW, glibc's mkstemp() implementation doesn't use it.
msg179841 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-01-12 23:48
> No, it only has an effect if the target file is a symlink.

Oh ok, so O_NOFOLLOW is useless when O_EXCL is used. It is safe to remove it, but please only modify Python 3.4 (just in case...).
msg183390 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-03-03 17:55
Closing.
Let's keep O_NOFOLLOW: it doesn't buy much, and might be useful for some arcane reason on some weird platform...
History
Date User Action Args
2013-03-03 17:55:40neologixsetstatus: open -> closed
resolution: fixed
messages: + msg183390

stage: committed/rejected
2013-01-12 23:48:11hayposetmessages: + msg179841
2013-01-12 23:46:16neologixsetmessages: + msg179840
2013-01-12 23:30:13hayposetmessages: + msg179837
2013-01-04 17:36:30neologixsetmessages: + msg179054
2013-01-04 17:33:34python-devsetnosy: + python-dev
messages: + msg179053
2013-01-04 14:35:46hayposetmessages: + msg179029
2013-01-04 14:29:22neologixsetkeywords: + patch, needs review
files: + tempfile_cloexec.diff
type: behavior
messages: + msg179026
2013-01-04 14:23:58hayposetmessages: + msg179025
2013-01-04 14:22:29haypocreate