diff options
| -rw-r--r-- | Lib/test/test_subprocess.py | 53 | ||||
| -rw-r--r-- | Misc/NEWS | 5 | ||||
| -rw-r--r-- | Modules/_posixsubprocess.c | 87 |
3 files changed, 102 insertions, 43 deletions
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 32ffb5f..b1588e0 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1926,6 +1926,59 @@ class POSIXProcessTestCase(BaseTestCase): "Some fds not in pass_fds were left open") self.assertIn(1, remaining_fds, "Subprocess failed") + + def test_close_fds_when_max_fd_is_lowered(self): + """Confirm that issue21618 is fixed (may fail under valgrind).""" + fd_status = support.findfile("fd_status.py", subdir="subprocessdata") + + open_fds = set() + # Add a bunch more fds to pass down. + for _ in range(10): + fd = os.open("/dev/null", os.O_RDONLY) + open_fds.add(fd) + + # Leave a two pairs of low ones available for use by the + # internal child error pipe and the stdout pipe. + for fd in sorted(open_fds)[:4]: + os.close(fd) + open_fds.remove(fd) + + for fd in open_fds: + self.addCleanup(os.close, fd) + os.set_inheritable(fd, True) + + max_fd_open = max(open_fds) + + import resource + rlim_cur, rlim_max = resource.getrlimit(resource.RLIMIT_NOFILE) + try: + # 9 is lower than the highest fds we are leaving open. + resource.setrlimit(resource.RLIMIT_NOFILE, (9, rlim_max)) + # Launch a new Python interpreter with our low fd rlim_cur that + # inherits open fds above that limit. It then uses subprocess + # with close_fds=True to get a report of open fds in the child. + # An explicit list of fds to check is passed to fd_status.py as + # letting fd_status rely on its default logic would miss the + # fds above rlim_cur as it normally only checks up to that limit. + p = subprocess.Popen( + [sys.executable, '-c', + textwrap.dedent(""" + import subprocess, sys + subprocess.Popen([sys.executable, {fd_status!r}] + + [str(x) for x in range({max_fd})], + close_fds=True) + """.format(fd_status=fd_status, max_fd=max_fd_open+1))], + stdout=subprocess.PIPE, close_fds=False) + finally: + resource.setrlimit(resource.RLIMIT_NOFILE, (rlim_cur, rlim_max)) + + output, unused_stderr = p.communicate() + remaining_fds = set(map(int, output.strip().split(b','))) + + self.assertFalse(remaining_fds & open_fds, + msg="Some fds were left open.") + + # Mac OS X Tiger (10.4) has a kernel bug: sometimes, the file # descriptor of a pipe closed in the parent process is valid in the # child process according to fstat(), but the mode of the file @@ -89,6 +89,11 @@ Core and Builtins Library ------- +- Issue #21618: The subprocess module could fail to close open fds that were + inherited by the calling process and already higher than POSIX resource + limits would otherwise allow. On systems with a functioning /proc/self/fd + or /dev/fd interface the max is now ignored and all fds are closed. + - Issue #20383: Introduce importlib.util.module_from_spec() as the preferred way to create a new module. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 648a569..8f5ce04 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -44,10 +44,6 @@ #define POSIX_CALL(call) do { if ((call) == -1) goto error; } while (0) -/* Maximum file descriptor, initialized on module load. */ -static long max_fd; - - /* Given the gc module call gc.enable() and return 0 on success. */ static int _enable_gc(PyObject *gc_module) @@ -166,14 +162,39 @@ make_inheritable(PyObject *py_fds_to_keep, int errpipe_write) } -/* Close all file descriptors in the range start_fd inclusive to - * end_fd exclusive except for those in py_fds_to_keep. If the - * range defined by [start_fd, end_fd) is large this will take a - * long time as it calls close() on EVERY possible fd. +/* Get the maximum file descriptor that could be opened by this process. + * This function is async signal safe for use between fork() and exec(). + */ +static long +safe_get_max_fd(void) +{ + long local_max_fd; +#if defined(__NetBSD__) + local_max_fd = fcntl(0, F_MAXFD); + if (local_max_fd >= 0) + return local_max_fd; +#endif +#ifdef _SC_OPEN_MAX + local_max_fd = sysconf(_SC_OPEN_MAX); + if (local_max_fd == -1) +#endif + local_max_fd = 256; /* Matches legacy Lib/subprocess.py behavior. */ + return local_max_fd; +} + + +/* Close all file descriptors in the range from start_fd and higher + * except for those in py_fds_to_keep. If the range defined by + * [start_fd, safe_get_max_fd()) is large this will take a long + * time as it calls close() on EVERY possible fd. + * + * It isn't possible to know for sure what the max fd to go up to + * is for processes with the capability of raising their maximum. */ static void -_close_fds_by_brute_force(int start_fd, int end_fd, PyObject *py_fds_to_keep) +_close_fds_by_brute_force(long start_fd, PyObject *py_fds_to_keep) { + long end_fd = safe_get_max_fd(); Py_ssize_t num_fds_to_keep = PySequence_Length(py_fds_to_keep); Py_ssize_t keep_seq_idx; int fd_num; @@ -229,16 +250,14 @@ struct linux_dirent64 { * it with some cpp #define magic to work on other OSes as well if you want. */ static void -_close_open_fd_range_safe(int start_fd, int end_fd, PyObject* py_fds_to_keep) +_close_open_fds_safe(int start_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); + _close_fds_by_brute_force(start_fd, py_fds_to_keep); return; } else { char buffer[sizeof(struct linux_dirent64)]; @@ -253,23 +272,23 @@ _close_open_fd_range_safe(int start_fd, int end_fd, PyObject* py_fds_to_keep) 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 && + if (fd != fd_dir_fd && fd >= start_fd && !_is_fd_in_sorted_fd_sequence(fd, py_fds_to_keep)) { while (close(fd) < 0 && errno == EINTR); } } } - close(fd_dir_fd); + while (close(fd_dir_fd) < 0 && errno == EINTR); } } -#define _close_open_fd_range _close_open_fd_range_safe +#define _close_open_fds _close_open_fds_safe #else /* NOT (defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)) */ -/* Close all open file descriptors in the range start_fd inclusive to end_fd - * exclusive. Do not close any in the sorted py_fds_to_keep list. +/* Close all open file descriptors from start_fd and higher. + * Do not close any in the sorted py_fds_to_keep list. * * This function violates the strict use of async signal safe functions. :( * It calls opendir(), readdir() and closedir(). Of these, the one most @@ -282,17 +301,13 @@ _close_open_fd_range_safe(int start_fd, int end_fd, PyObject* py_fds_to_keep) * http://womble.decadent.org.uk/readdir_r-advisory.html */ static void -_close_open_fd_range_maybe_unsafe(int start_fd, int end_fd, - PyObject* py_fds_to_keep) +_close_open_fds_maybe_unsafe(long start_fd, PyObject* py_fds_to_keep) { DIR *proc_fd_dir; #ifndef HAVE_DIRFD - while (_is_fd_in_sorted_fd_sequence(start_fd, py_fds_to_keep) && - (start_fd < end_fd)) { + while (_is_fd_in_sorted_fd_sequence(start_fd, py_fds_to_keep)) { ++start_fd; } - if (start_fd >= end_fd) - return; /* Close our lowest fd before we call opendir so that it is likely to * reuse that fd otherwise we might close opendir's file descriptor in * our loop. This trick assumes that fd's are allocated on a lowest @@ -300,8 +315,6 @@ _close_open_fd_range_maybe_unsafe(int start_fd, int end_fd, while (close(start_fd) < 0 && errno == EINTR); ++start_fd; #endif - if (start_fd >= end_fd) - return; #if defined(__FreeBSD__) if (!_is_fdescfs_mounted_on_dev_fd()) @@ -311,7 +324,7 @@ _close_open_fd_range_maybe_unsafe(int start_fd, int end_fd, proc_fd_dir = opendir(FD_DIR); if (!proc_fd_dir) { /* No way to get a list of open fds. */ - _close_fds_by_brute_force(start_fd, end_fd, py_fds_to_keep); + _close_fds_by_brute_force(start_fd, py_fds_to_keep); } else { struct dirent *dir_entry; #ifdef HAVE_DIRFD @@ -324,7 +337,7 @@ _close_open_fd_range_maybe_unsafe(int start_fd, int end_fd, int fd; if ((fd = _pos_int_from_ascii(dir_entry->d_name)) < 0) continue; /* Not a number. */ - if (fd != fd_used_by_opendir && fd >= start_fd && fd < end_fd && + if (fd != fd_used_by_opendir && fd >= start_fd && !_is_fd_in_sorted_fd_sequence(fd, py_fds_to_keep)) { while (close(fd) < 0 && errno == EINTR); } @@ -332,13 +345,13 @@ _close_open_fd_range_maybe_unsafe(int start_fd, int end_fd, } if (errno) { /* readdir error, revert behavior. Highly Unlikely. */ - _close_fds_by_brute_force(start_fd, end_fd, py_fds_to_keep); + _close_fds_by_brute_force(start_fd, py_fds_to_keep); } closedir(proc_fd_dir); } } -#define _close_open_fd_range _close_open_fd_range_maybe_unsafe +#define _close_open_fds _close_open_fds_maybe_unsafe #endif /* else NOT (defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)) */ @@ -457,14 +470,8 @@ child_exec(char *const exec_array[], /* close FDs after executing preexec_fn, which might open FDs */ if (close_fds) { - int local_max_fd = max_fd; -#if defined(__NetBSD__) - local_max_fd = fcntl(0, F_MAXFD); - if (local_max_fd < 0) - local_max_fd = max_fd; -#endif /* TODO HP-UX could use pstat_getproc() if anyone cares about it. */ - _close_open_fd_range(3, local_max_fd, py_fds_to_keep); + _close_open_fds(3, py_fds_to_keep); } /* This loop matches the Lib/os.py _execvpe()'s PATH search when */ @@ -759,11 +766,5 @@ static struct PyModuleDef _posixsubprocessmodule = { PyMODINIT_FUNC PyInit__posixsubprocess(void) { -#ifdef _SC_OPEN_MAX - max_fd = sysconf(_SC_OPEN_MAX); - if (max_fd == -1) -#endif - max_fd = 256; /* Matches Lib/subprocess.py */ - return PyModule_Create(&_posixsubprocessmodule); } |