Fixes python#23564: Fix a partially broken sanity check in the _posixsubprocess

gpshead · gpshead · commit d0a5b1c34325 · 2015-11-15T21:15:26.000-08:00
internals regarding how fds_to_pass were passed to the child.  The bug
had no actual impact as subprocess.py already avoided it.
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
@@ -2312,8 +2312,6 @@ def test_fork_exec(self):
             func = lambda: None
             gc.enable()
 
-            executable_list = "exec"   # error: must be a sequence
-
             for args, exe_list, cwd, env_list in (
                 (123,      [b"exe"], None, [b"env"]),
                 ([b"arg"], 123,      None, [b"env"]),
@@ -2331,6 +2329,34 @@ def test_fork_exec(self):
             if not gc_enabled:
                 gc.disable()
 
+    @support.cpython_only
+    def test_fork_exec_sorted_fd_sanity_check(self):
+        # Issue #23564: sanity check the fork_exec() fds_to_keep sanity check.
+        import _posixsubprocess
+        gc_enabled = gc.isenabled()
+        try:
+            gc.enable()
+
+            for fds_to_keep in (
+                (-1, 2, 3, 4, 5),  # Negative number.
+                ('str', 4),  # Not an int.
+                (18, 23, 42, 2**63),  # Out of range.
+                (5, 4),  # Not sorted.
+                (6, 7, 7, 8),  # Duplicate.
+            ):
+                with self.assertRaises(
+                        ValueError,
+                        msg='fds_to_keep={}'.format(fds_to_keep)) as c:
+                    _posixsubprocess.fork_exec(
+                        [b"false"], [b"false"],
+                        True, fds_to_keep, None, [b"env"],
+                        -1, -1, -1, -1,
+                        1, 2, 3, 4,
+                        True, True, None)
+                self.assertIn('fds_to_keep', str(c.exception))
+        finally:
+            if not gc_enabled:
+                gc.disable()
 
 
 @unittest.skipUnless(mswindows, "Windows specific tests")
diff --git a/Misc/NEWS b/Misc/NEWS
@@ -11,6 +11,10 @@ Release date: TBA
 Core and Builtins
 -----------------
 
+- Issue #23564: Fixed a partially broken sanity check in the _posixsubprocess
+  internals regarding how fds_to_pass were passed to the child.  The bug had
+  no actual impact as subprocess.py already avoided it.
+
 - Issue #25388: Fixed tokenizer crash when processing undecodable source code
   with a null byte.
 
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
@@ -109,10 +109,11 @@ _sanity_check_python_fd_sequence(PyObject *fd_sequence)
     for (seq_idx = 0; seq_idx < seq_len; ++seq_idx) {
         PyObject* py_fd = PySequence_Fast_GET_ITEM(fd_sequence, seq_idx);
         long iter_fd = PyLong_AsLong(py_fd);
-        if (iter_fd < 0 || iter_fd < prev_fd || iter_fd > INT_MAX) {
+        if (iter_fd < 0 || iter_fd <= prev_fd || iter_fd > INT_MAX) {
             /* Negative, overflow, not a Long, unsorted, too big for a fd. */
             return 1;
         }
+        prev_fd = iter_fd;
     }
     return 0;
 }

Original file line number	Diff line number	Diff line change
`@@ -109,10 +109,11 @@ _sanity_check_python_fd_sequence(PyObject *fd_sequence)`
`109`	`109`	`for (seq_idx = 0; seq_idx < seq_len; ++seq_idx) {`
`110`	`110`	`PyObject* py_fd = PySequence_Fast_GET_ITEM(fd_sequence, seq_idx);`
`111`	`111`	`long iter_fd = PyLong_AsLong(py_fd);`
`112`		`- if (iter_fd < 0 \|\| iter_fd < prev_fd \|\| iter_fd > INT_MAX) {`
	`112`	`+ if (iter_fd < 0 \|\| iter_fd <= prev_fd \|\| iter_fd > INT_MAX) {`
`113`	`113`	`/* Negative, overflow, not a Long, unsorted, too big for a fd. */`
`114`	`114`	`return 1;`
`115`	`115`	`}`
	`116`	`+ prev_fd = iter_fd;`
`116`	`117`	`}`
`117`	`118`	`return 0;`
`118`	`119`	`}`