From f1ae4d1925c606740557bcbbbae15b6a1a139132 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Sat, 17 Feb 2018 06:05:29 +0300 Subject: [PATCH 1/3] bpo-32862: Make os.dup2(fd, fd) a no-op for valid fd os.dup2(fd, fd, inheritable=True) never changed fd inheritability, but with inheritable=False the function might fail or change it in an inconsistent manner depending on the platform. --- Doc/library/os.rst | 3 +++ Lib/test/test_os.py | 8 ++++++++ Modules/posixmodule.c | 9 +++++---- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/Doc/library/os.rst b/Doc/library/os.rst index bae432d33b0bd44..b71e210934ef003 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -741,6 +741,9 @@ as internal buffering of data. ` by default or non-inheritable if *inheritable* is ``False``. + If *fd* is valid and equal to *fd2*, this function has no effect + regardless of the value of *inheritable*. + .. versionchanged:: 3.4 Add the optional *inheritable* parameter. diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 77e4a008ae61e8a..fc5840d8c2fda0f 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3102,6 +3102,14 @@ def test_dup2(self): self.assertEqual(os.dup2(fd, fd3, inheritable=False), fd3) self.assertFalse(os.get_inheritable(fd3)) + # dup2(fd, fd) must have no effect for a valid fd + os.set_inheritable(fd, True) + os.dup2(fd, fd, inheritable=False) + self.assertTrue(os.get_inheritable(fd)) + os.set_inheritable(fd, False) + os.dup2(fd, fd, inheritable=True) + 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/Modules/posixmodule.c b/Modules/posixmodule.c index d242df0b6f3106d..ee581edac5d0b91 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -8041,7 +8041,7 @@ os_dup2_impl(PyObject *module, int fd, int fd2, int inheritable) 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) { + if (!inheritable && fd != fd2 && _Py_set_inheritable(fd2, 0, NULL) < 0) { close(fd2); return -1; } @@ -8061,7 +8061,7 @@ os_dup2_impl(PyObject *module, int fd, int fd2, int inheritable) #else #ifdef HAVE_DUP3 - if (!inheritable && dup3_works != 0) { + if (!inheritable && fd != fd2 && dup3_works != 0) { Py_BEGIN_ALLOW_THREADS res = dup3(fd, fd2, O_CLOEXEC); Py_END_ALLOW_THREADS @@ -8075,7 +8075,7 @@ os_dup2_impl(PyObject *module, int fd, int fd2, int inheritable) } } - if (inheritable || dup3_works == 0) + if (inheritable || fd == fd2 || dup3_works == 0) { #endif Py_BEGIN_ALLOW_THREADS @@ -8086,7 +8086,8 @@ os_dup2_impl(PyObject *module, int fd, int fd2, int inheritable) return -1; } - if (!inheritable && _Py_set_inheritable(fd2, 0, NULL) < 0) { + if (!inheritable && fd != fd2 && _Py_set_inheritable(fd2, 0, NULL) < 0) + { close(fd2); return -1; } From e792f05e2ceb3993f82cbdedb5efd21097179731 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Wed, 28 Feb 2018 12:37:32 +0300 Subject: [PATCH 2/3] Fix the test for old Android --- Lib/test/test_os.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index fc5840d8c2fda0f..81d58ced0681809 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3103,12 +3103,16 @@ def test_dup2(self): self.assertFalse(os.get_inheritable(fd3)) # dup2(fd, fd) must have no effect for a valid fd - os.set_inheritable(fd, True) - os.dup2(fd, fd, inheritable=False) - self.assertTrue(os.get_inheritable(fd)) - os.set_inheritable(fd, False) - os.dup2(fd, fd, inheritable=True) - self.assertFalse(os.get_inheritable(fd)) + # Issue #26935: Avoid failure due to a bionic bug + # in old Android. + if (not hasattr(sys, 'getandroidapilevel') or + sys.getandroidapilevel() >= 23): + os.set_inheritable(fd, True) + os.dup2(fd, fd, inheritable=False) + self.assertTrue(os.get_inheritable(fd)) + os.set_inheritable(fd, False) + os.dup2(fd, fd, inheritable=True) + self.assertFalse(os.get_inheritable(fd)) @unittest.skipUnless(hasattr(os, 'openpty'), "need os.openpty()") def test_openpty(self): From a55d7a745b4b33aed4420b6cfbf634c8fb209b25 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Wed, 28 Feb 2018 12:37:48 +0300 Subject: [PATCH 3/3] Add NEWS entry --- .../NEWS.d/next/Library/2018-02-28-12-36-16.bpo-32862.CUwrAr.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2018-02-28-12-36-16.bpo-32862.CUwrAr.rst diff --git a/Misc/NEWS.d/next/Library/2018-02-28-12-36-16.bpo-32862.CUwrAr.rst b/Misc/NEWS.d/next/Library/2018-02-28-12-36-16.bpo-32862.CUwrAr.rst new file mode 100644 index 000000000000000..31b807c58916988 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-02-28-12-36-16.bpo-32862.CUwrAr.rst @@ -0,0 +1 @@ +Make os.dup2(fd, fd) have no effect apart from validating fd.