bpo-32844: subprocess: Fix a potential misredirection of a low fd to stderr #5689
Conversation
…stderr When redirecting, subprocess attempts to achieve the following state: each fd to be redirected to is less than or equal to the fd it is redirected from, which is necessary because redirection occurs in the ascending order of destination descriptors. It fails to do so in a couple of corner cases, for example, if 1 is redirected to 2 and 0 is closed in the parent.
Lib/test/test_subprocess.py
Outdated
| os.lseek(from_fd, 0, os.SEEK_SET) | ||
| exp_bytes = str(to_fd).encode('ascii') | ||
| read_bytes = os.read(from_fd, 1024) | ||
| self.assertEqual(read_bytes, exp_bytes) |
There was a problem hiding this comment.
To make debugging failures easier, add a msg= parameter to this assertion that contains the an explanation of the from_fds, to_fds, and skipped_fd configuration that was being tested.
Lib/test/test_subprocess.py
Outdated
| os.write(fd, str(fd).encode('ascii')) | ||
| ''') | ||
|
|
||
| skipped_fd = next(fd for fd in range(3) if fd not in to_fds) |
There was a problem hiding this comment.
I believe this works, but it feels like something using sets might be easier to understand?
({0, 1, 2} - set(to_fds)).pop() perhaps?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Lib/test/test_subprocess.py
Outdated
| @@ -2195,7 +2193,7 @@ def _check_swap_std_fds_with_one_closed(self, from_fds, to_fds): | |||
| os.write(fd, str(fd).encode('ascii')) | |||
| ''') | |||
|
|
|||
| skipped_fd = next(fd for fd in range(3) if fd not in to_fds) | |||
| skipped_fd = (set(range(3)) - set(to_fds)).pop() | |||
There was a problem hiding this comment.
I've used the suggested construct but with range(3) instead of {0, 1, 2} for consistency with the surrounding code.
| parent descriptor {from_fd} got redirected | ||
| to descriptor(s) {read_fds} instead of descriptor {to_fd}. | ||
| """) | ||
| self.assertEqual([to_fd], read_fds, msg) |
There was a problem hiding this comment.
I thought that the assertion comparing bytes would be not very friendly even with a message, so I've decided it's not too dangerous to decode them.
|
Thank you for the review! I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @gpshead: please review the changes made to this pull request. |
|
GH-6262 is a backport of this pull request to the 3.7 branch. |
|
GH-6263 is a backport of this pull request to the 3.6 branch. |
When redirecting, subprocess attempts to achieve the following state:
each fd to be redirected to is less than or equal to the fd
it is redirected from, which is necessary because redirection
occurs in the ascending order of destination descriptors.
It fails to do so in a couple of corner cases,
for example, if 1 is redirected to 2 and 0 is closed in the parent.
https://bugs.python.org/issue32844