[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[STINNER Victor]

STINNER Victor  added the comment:

I close the issue as "not a bug". If you disagree, please comment the issue :-)

Steven Stewart-Gallus:
> In the code FD_DIR is "/proc/self/fd" on Linux. I'm not sure this code
> is correct. This seems as if it would have the same problems as
> iterating over a list and modifying it at the same time.

I don't understand the initial issue. What makes you think that the current 
code is not reliable? Python is using this code for years, and so far, nobody 
reported any crash on this code.

You didn't report a crash, you only wrote that you suspect that there is a bug. 
Can you prove that the current Python code is wrong? Did you look at the 
implementation of readdir() in the Linux glibc for example?

About close() vs marking file descriptor non inheritable, I'm not convinced 
that marking file descriptors is faster or more reliable. On many operating 
systems, _Py_set_inheritable() requires 2 syscalls (get flags, set flags), 
whereas close() is a single syscall: calling close() is faster.

---

Python now has a _Py_closerange() function which supports:

* Linux closerange() syscall, calling the glibc closerange() function
* FreeBSD closefrom() function
* Solaris fdwalk() function

On Linux, _posixsubprocess still iterates /proc/self/pid/ if this directory can 
be opened, even if the closerange() function is available. I'm not sure which 
option is the safest or the fastest. 

By the way, Steven Stewart-Gallus did mention his operating system.

--
resolution:  -> not a bug
stage:  -> resolved
status: open -> closed

___
Python tracker 

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[STINNER Victor]

STINNER Victor  added the comment:

Dan Snider: "On Android, if os.close_range closes the file descriptor of the 
scandir iterator, the interpreter immediately crashes"

I don't see anything wrong with Python. It's not different than calling 
os.close(3).

If it hurts, don't do that :-)

--

___
Python tracker 

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Dan Snider]

Dan Snider  added the comment:

On Android, if os.close_range closes the file descriptor of the scandir 
iterator, the interpreter immediately crashes eg:

>>> import os
>>> os.scandir()

>>> os.closerange(3,)
fdsan: attempted to close file descriptor 3, expected to be unowned, actually 
owned by DIR* 0x7263390290
Aborted
$

Sorry if this isn't an appropriate place to add this, but the title matched the 
concept I think and I don't know enough to judge whether this is something that 
needs to be fixed.

--
nosy: +bup

___
Python tracker 

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[STINNER Victor]

STINNER Victor  added the comment:

Linux 5.9 added close_range() syscall that we can use.

Linux 5.10 may get a new CLOSE_RANGE_CLOEXEC flag for close_range().
https://lwn.net/Articles/837816/

We may use close_range() rather than iterating on /proc/self/fd/ in user space. 
It may be faster and safer.

--

___
Python tracker 

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Gregory P. Smith]

Gregory P. Smith  added the comment:

The Linux kernel code is not sufficiently easy to follow to understand if it 
has this issue or if it pre-creates the dirent structures for all fds at 
opendir time for /proc/self/fd or if it is iterating through the list of fds in 
sorted order so an older closed fd will not interfere with its internal 
iteration.

Regardless, I've yet to knowingly witness a problem from this come up in 
practice.  knowingly and yet being key words. :)

But I like the general theme of your patch to set CLOEXEC on all of the fd's 
rather than explicitly call close(fd) in the directory reading loop.

--
assignee:  -> gregory.p.smith
components: +Library (Lib)
resolution: out of date -> 
type:  -> behavior
versions: +Python 3.10, Python 3.9 -Python 3.5

___
Python tracker 

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

It is Modules/_posixsubprocess.c.

We still do not have a reproducer, so I am not sure that the code has this 
problem.

--
nosy: +serhiy.storchaka
status: pending -> open

___
Python tracker 

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Irit Katriel]

Irit Katriel  added the comment:

I can't find _posixsubmodules.c or _close_open_fd_range_safe in the codebase. 
Is this issue out of date?

--
nosy: +iritkatriel
resolution:  -> out of date
status: open -> pending

___
Python tracker 

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Steven Stewart-Gallus]

Steven Stewart-Gallus added the comment:

Okay, I made a patch that I hoped dealt with all the criticisms and that fixed 
up a problem I noted myself.

--
Added file: http://bugs.python.org/file35599/fixed-setcloexec.patch

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Steven Stewart-Gallus]

Changes by Steven Stewart-Gallus sstewartgallu...@mylangara.bc.ca:


Removed file: http://bugs.python.org/file35599/fixed-setcloexec.patch

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Steven Stewart-Gallus]

Changes by Steven Stewart-Gallus sstewartgallu...@mylangara.bc.ca:


Added file: http://bugs.python.org/file35600/fixed-setcloexec.patch

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[STINNER Victor]

STINNER Victor added the comment:

The _posixsubprocess module is not compiled on Windows.

--

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Steven Stewart-Gallus]

Steven Stewart-Gallus added the comment:

Okay, now I'm confused. How would I conditionally compile and use the 
setcloexec object and header on POSIX platforms and not on Windows?

--

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Steven Stewart-Gallus]

Steven Stewart-Gallus added the comment:

Okay, so the Python directory seems to be where wrappers over low level or 
missing functionality is placed. So I guess I should make a _Py_setcloexec 
function in a Python/setcloexec.c and a definition in Include/setcloexec.h?

--

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Berker Peksag]

Changes by Berker Peksag berker.pek...@gmail.com:


--
nosy: +gregory.p.smith
versions: +Python 3.5

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[STINNER Victor]

Changes by STINNER Victor victor.stin...@gmail.com:


--
nosy: +neologix, pitrou

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[STINNER Victor]

STINNER Victor added the comment:

I have no opinon on close() vs setting CLOEXEC flag, but you should use 
_Py_set_inheritable(fd, 0, NULL). _Py_set_inheritable() is able to set the 
CLOEXEC flag in a single syscall, instead of 2 syscalls. 
_close_fds_by_brute_force() is already very slow when the maximum file 
descriptor is large:
http://legacy.python.org/dev/peps/pep-0446/#closing-all-open-file-descriptors

--
nosy: +haypo

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[STINNER Victor]

STINNER Victor added the comment:

Since Python 3.4, all file descriptors directly created by Python are marked as 
non-inheritable. It should now be safe to use subprocess with close_fds=False, 
but the default value was not changed because third party modules (especially 
code written in C) may not respect the PEP 446.

If you are interested to change the default value, you should audit the major 
Python modules on PyPI to check if they were adapted to respect the PEP 446 
(mark all file descriptors as non-inheritable).

--

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Steven Stewart-Gallus]

Steven Stewart-Gallus added the comment:

It occurred to me that the current patch I have is wrong and that using 
_Py_set_inheritable is wrong because EBADF can occur in the brute force version 
which in the case of _Py_set_inheritable raises an error which I am not sure is 
asynch signal safe. I could test ahead of time but that is a bit hacky. The 
most principled approach would be to extract either a common set_cloexec or 
set_inheritable function. If set_inheritable is done then I would have to 
report either a Windows error or an errno error which would be messy. I'm not 
sure where the best place to put the set_cloexec function would be.

--

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Steven Stewart-Gallus]

New submission from Steven Stewart-Gallus:

Hello,

I noticed some possible bad behaviour while working on Python issue
21618 (see http://bugs.python.org/issue21618). Python has the
following code in _posixsubmodules.c for closing extra files before
spawning a process:

static void
_close_open_fd_range_safe(int start_fd, int end_fd, PyObject* py_fds_to_keep)
{
int fd_dir_fd;
if (start_fd = end_fd)
return;

fd_dir_fd = _Py_open(FD_DIR, O_RDONLY);
if (fd_dir_fd == -1) {
/* No way to get a list of open fds. */
_close_fds_by_brute_force(start_fd, end_fd, py_fds_to_keep);
return;
} else {
char buffer[sizeof(struct linux_dirent64)];
int bytes;
while ((bytes = syscall(SYS_getdents64, fd_dir_fd,
(struct linux_dirent64 *)buffer,
sizeof(buffer)))  0) {
struct linux_dirent64 *entry;
int offset;
for (offset = 0; offset  bytes; offset += entry-d_reclen) {
int fd;
entry = (struct linux_dirent64 *)(buffer + offset);
if ((fd = _pos_int_from_ascii(entry-d_name))  0)
continue;  /* Not a number. */
if (fd != fd_dir_fd  fd = start_fd  fd  end_fd 
!_is_fd_in_sorted_fd_sequence(fd, py_fds_to_keep)) {
while (close(fd)  0  errno == EINTR);
}
}
}
close(fd_dir_fd);
}
}

In the code FD_DIR is /proc/self/fd on Linux. I'm not sure this code
is correct. This seems as if it would have the same problems as
iterating over a list and modifying it at the same time.

I can think of a few solutions but they all have problems.

One could allocate a list of open files once and then iterate through
that list and close the files but this is not signal safe so this
solution is incorrect. One possible workaround is to use mmap to
allocate the memory. This is a direct system call and I don't see a
reason for it not to be signal safe but I'm not sure. One neat hack
would be too mmap the memory ahead of time and then rely on lazy
paging to allocate the memory lazily. Another solution is to search
for the largest open file and then iterate over and close all the
possible file descriptors in between. So far most possible solutions
just seem really hacky to me though.

I feel the best solution is to let the OS close the files and to set
the O_CLOEXEC flag on the files that need to be closed.

--
messages: 219510
nosy: sstewartgallus
priority: normal
severity: normal
status: open
title: Concurrently closing files and iterating over the open files directory 
is not well specified

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Steven Stewart-Gallus]

Steven Stewart-Gallus added the comment:

Okay, I've made a simple proof of concept patch.

--

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

[issue21627] Concurrently closing files and iterating over the open files directory is not well specified

[Steven Stewart-Gallus]

Steven Stewart-Gallus added the comment:

Gah, I had trouble figuring out mecurial.

Close files after reading open files and not concurrently

This commit removes the old behaviour of closing files concurrently
with reading the system's list of open files and instead closes the
files after the list has been read.

Because no memory can be allocated in that specific context the
approach of setting the CLOEXEC flag and letting the following exec
close the files has been used.

For consistency, the brute force approach to closing the files has
also been modified to set the CLOEXEC flag.

--
keywords: +patch
Added file: http://bugs.python.org/file35441/cloexec.patch

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