Skip to content

gh-77043: Make os.dup2(fd, fd) a no-op for valid fd #5713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
izbyshev wants to merge 3 commits into
base: main
Choose a base branch
from
+21 −4
Open

gh-77043: Make os.dup2(fd, fd) a no-op for valid fd #5713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1ae4d1
bpo-32862: Make os.dup2(fd, fd) a no-op for valid fd
Feb 17, 2018
e792f05
Fix the test for old Android
Feb 28, 2018
a55d7a7
Add NEWS entry
Feb 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Doc/library/os.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,9 @@ as internal buffering of data.
<fd_inheritance>` by default or non-inheritable if *inheritable*
is ``False``.

If *fd* is valid and equal to *fd2*, this function has no effect

@ZackerySpytz ZackerySpytz Sep 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a versionchanged directive.

@vstinner vstinner Jan 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"this function has no effect": that's wrong, it does validate the FD which is a legit usage of this function. I would rather remove this sentence.

regardless of the value of *inheritable*.

.. versionchanged:: 3.4
Add the optional *inheritable* parameter.

Expand Down
12 changes: 12 additions & 0 deletions Lib/test/test_os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3102,6 +3102,18 @@ 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

@vstinner vstinner Jan 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this test into a separated method and emit somehow a SkipTest exception to record that the test is skipped on old Android versions.

# Issue #26935: Avoid failure due to a bionic bug

@ZackerySpytz ZackerySpytz Sep 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Issue #26935: Avoid failure due to a bionic bug
# bpo-26935: Avoid failure due to a bionic bug

I believe it's preferred to use bpo-n in code comments.

# in old Android.
if (not hasattr(sys, 'getandroidapilevel') or

@ZackerySpytz ZackerySpytz Sep 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this new test code should be placed in a new method. The Android checks should be put in a @unittest.skip* decorator.

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):
master_fd, slave_fd = os.openpty()
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2018-02-28-12-36-16.bpo-32862.CUwrAr.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make os.dup2(fd, fd) have no effect apart from validating fd.
9 changes: 5 additions & 4 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8041,7 +8041,7 @@ os_dup2_impl(PyObject *module, int fd, int fd2, int inheritable)
res = fd2; // msvcrt dup2 returns 0 on success.

@vstinner vstinner Jan 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document somewhere the rationale for "fd != fd2" tests with a reference to "bpo-32862".

/* 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;
}
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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;
}
Expand Down