[issue16860] Use O_CLOEXEC in the tempfile module

[STINNER Victor]

New submission from STINNER Victor:

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.

--
components: Library (Lib)
messages: 179023
nosy: haypo, neologix
priority: normal
severity: normal
status: open
title: Use O_CLOEXEC in the tempfile module
versions: Python 3.3, Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16860] Use O_CLOEXEC in the tempfile module

[STINNER Victor]

STINNER Victor added the comment:

See also #16850 which proposes to expose O_CLOEXEC feature in the open() 
builtin function.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16860] Use O_CLOEXEC in the tempfile module

[Charles-François Natali]

Charles-François Natali added the comment:

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

--
keywords: +needs review, patch
type:  -> behavior
Added file: http://bugs.python.org/file28560/tempfile_cloexec.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16860] Use O_CLOEXEC in the tempfile module

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16860] Use O_CLOEXEC in the tempfile module

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16860] Use O_CLOEXEC in the tempfile module

[Charles-François Natali]

Charles-François Natali added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16860] Use O_CLOEXEC in the tempfile module

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16860] Use O_CLOEXEC in the tempfile module

[Charles-François Natali]

Charles-François Natali added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16860] Use O_CLOEXEC in the tempfile module

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16860] Use O_CLOEXEC in the tempfile module

[Charles-François Natali]

Charles-François Natali added the comment:

Closing.
Let's keep O_NOFOLLOW: it doesn't buy much, and might be useful for some arcane 
reason on some weird platform...

--
resolution:  -> fixed
stage:  -> committed/rejected
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com