From 9442ad83f6e32b8e472ea2ba4d8853837ba19c1c Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Sat, 17 Feb 2018 06:05:29 +0300 Subject: [PATCH 1/2] gh-77043: Make os.dup2(fd, fd, False) always remove inheritability Currently, os.dup2(fd, fd, False) behaves differently depending on the platform: * On Windows, Linux before 2.6.27, macOS, and FreeBSD it validates the fd and makes it non-inheritable. * On Solaris it validates the fd, but doesn't change its inheritability. * On modern Linux and Illumos it fails with OSError. Fix this inconsistency by making it always behave as in the first point. --- Doc/library/os.rst | 3 + Lib/test/test_os.py | 22 ++++- ...3-02-22-15-20-55.gh-issue-77043.fXztgY.rst | 1 + Modules/posixmodule.c | 97 ++++++++----------- 4 files changed, 64 insertions(+), 59 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-02-22-15-20-55.gh-issue-77043.fXztgY.rst diff --git a/Doc/library/os.rst b/Doc/library/os.rst index 23ce98785bedfc6..75503165d8452b3 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -973,6 +973,9 @@ as internal buffering of data. ` by default or non-inheritable if *inheritable* is ``False``. + If *fd* is equal to *fd2*, the file descriptor is validated, but its + inheritability is not changed unless *inheritable* is ``False``. + .. availability:: not WASI. .. versionchanged:: 3.4 diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 792794ca1094892..1cd5b78f115f304 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2235,7 +2235,7 @@ def test_dup2_negative_fd(self): -2**31, ] for fd, fd2 in itertools.product(fds, repeat=2): - if fd != fd2: + if fd != valid_fd or fd2 != valid_fd: with self.subTest(fd=fd, fd2=fd2): with self.assertRaises(OSError) as ctx: os.dup2(fd, fd2) @@ -4041,6 +4041,26 @@ def test_dup2(self): self.assertEqual(os.dup2(fd, fd3, inheritable=False), fd3) self.assertFalse(os.get_inheritable(fd3)) + @unittest.skipUnless(hasattr(os, 'dup2'), "need os.dup2()") + @unittest.skipIf(hasattr(sys, 'getandroidapilevel') and + sys.getandroidapilevel() < 23, + "bpo-26935: os.dup2() fails for equal fds on old Android") + def test_dup2_eq_fd(self): + fd = os.open(__file__, os.O_RDONLY) + self.addCleanup(os.close, fd) + + # Non-inheritability is preserved by default and even with explicit + # inheritable=True. + os.dup2(fd, fd) + self.assertFalse(os.get_inheritable(fd)) + os.dup2(fd, fd, inheritable=True) + self.assertFalse(os.get_inheritable(fd)) + + # gh-77043: Inheritability is removed with inheritable=False. + os.set_inheritable(fd, True) + os.dup2(fd, fd, inheritable=False) + self.assertFalse(os.get_inheritable(fd)) + @unittest.skipUnless(hasattr(os, 'openpty'), "need os.openpty()") def test_openpty(self): master_fd, slave_fd = os.openpty() diff --git a/Misc/NEWS.d/next/Library/2023-02-22-15-20-55.gh-issue-77043.fXztgY.rst b/Misc/NEWS.d/next/Library/2023-02-22-15-20-55.gh-issue-77043.fXztgY.rst new file mode 100644 index 000000000000000..3c84de4fc78f635 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-02-22-15-20-55.gh-issue-77043.fXztgY.rst @@ -0,0 +1 @@ +Make :func:`os.dup2(fd, fd, False)` always remove fd inheritability. diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 7beb2cee64a05c6..7cd2c021fbb4186 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -9788,84 +9788,65 @@ static int os_dup2_impl(PyObject *module, int fd, int fd2, int inheritable) /*[clinic end generated code: output=bc059d34a73404d1 input=c3cddda8922b038d]*/ { - int res = 0; -#if defined(HAVE_DUP3) && \ - !(defined(HAVE_FCNTL_H) && defined(F_DUP2FD_CLOEXEC)) - /* dup3() is available on Linux 2.6.27+ and glibc 2.9 */ - static int dup3_works = -1; -#endif - - /* dup2() can fail with EINTR if the target FD is already open, because it - * then has to be closed. See os_close_impl() for why we don't handle EINTR - * upon close(), and therefore below. - */ -#ifdef MS_WINDOWS - Py_BEGIN_ALLOW_THREADS - _Py_BEGIN_SUPPRESS_IPH - res = dup2(fd, fd2); - _Py_END_SUPPRESS_IPH - Py_END_ALLOW_THREADS - if (res < 0) { - posix_error(); - return -1; - } - res = fd2; // msvcrt dup2 returns 0 on success. - - /* Character files like console cannot be make non-inheritable */ - if (!inheritable && _Py_set_inheritable(fd2, 0, NULL) < 0) { - close(fd2); - return -1; - } + int res = -1; -#elif defined(HAVE_FCNTL_H) && defined(F_DUP2FD_CLOEXEC) - Py_BEGIN_ALLOW_THREADS - if (!inheritable) - res = fcntl(fd, F_DUP2FD_CLOEXEC, fd2); - else - res = dup2(fd, fd2); - Py_END_ALLOW_THREADS - if (res < 0) { - posix_error(); - return -1; - } - -#else - -#ifdef HAVE_DUP3 - if (!inheritable && dup3_works != 0) { + // gh-77043: Always fallback to dup2() if fd == fd2 because other + // functions may behave differently in this case. + if (!inheritable && fd != fd2) { +#if defined(HAVE_FCNTL_H) && defined(F_DUP2FD_CLOEXEC) Py_BEGIN_ALLOW_THREADS - res = dup3(fd, fd2, O_CLOEXEC); + res = fcntl(fd, F_DUP2FD_CLOEXEC, fd2); Py_END_ALLOW_THREADS if (res < 0) { - if (dup3_works == -1) - dup3_works = (errno != ENOSYS); - if (dup3_works) { - posix_error(); - return -1; + posix_error(); + return -1; + } +#elif defined(HAVE_DUP3) + // dup3() is available on Linux 2.6.27+ and glibc 2.9. + static int dup3_works = -1; + if (dup3_works) { + Py_BEGIN_ALLOW_THREADS + res = dup3(fd, fd2, O_CLOEXEC); + Py_END_ALLOW_THREADS + if (res < 0) { + if (dup3_works == -1) { + dup3_works = (errno != ENOSYS); + } + if (dup3_works) { + posix_error(); + return -1; + } } } +#endif } - if (inheritable || dup3_works == 0) - { -#endif + if (res < 0) { + // dup2() can fail with EINTR if the target FD is already open, because + // it then has to be closed. See os_close_impl() for why we don't + // handle EINTR upon close(), and therefore below. Py_BEGIN_ALLOW_THREADS + _Py_BEGIN_SUPPRESS_IPH res = dup2(fd, fd2); + _Py_END_SUPPRESS_IPH Py_END_ALLOW_THREADS if (res < 0) { posix_error(); return -1; } +#ifdef MS_WINDOWS + res = fd2; // msvcrt dup2 returns 0 on success. +#endif + // On Windows, character files like console cannot be made + // non-inheritable. if (!inheritable && _Py_set_inheritable(fd2, 0, NULL) < 0) { - close(fd2); + if (fd != fd2) { + close(fd2); + } return -1; } -#ifdef HAVE_DUP3 } -#endif - -#endif return res; } From 5f2f2e12e642c5f94ae824c49fd65b932d33d3bb Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Wed, 5 Apr 2023 21:22:59 +0300 Subject: [PATCH 2/2] Improve phrasing in docs --- Doc/library/os.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Doc/library/os.rst b/Doc/library/os.rst index 75503165d8452b3..9a33cdb021134d1 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -973,8 +973,8 @@ as internal buffering of data. ` by default or non-inheritable if *inheritable* is ``False``. - If *fd* is equal to *fd2*, the file descriptor is validated, but its - inheritability is not changed unless *inheritable* is ``False``. + If *fd* is equal to *fd2* and *inheritable* is ``True``, the "inheritable" + flag of the specified file descriptor is not changed. .. availability:: not WASI.