Fix filedescriptor out of range in select() when running commands (#65058)

* Fix filedescriptor out of range in select() when running commands

* Simplify the run_command() code

Now that we're using selectors in run_command(), we can simplify some of
the code.

* Use fileobj.read() instead of os.read()
* No longer use get_buffer_size() as we can just slurp all of the data
  instead.

Also use a simpler conditional check of whether the selector map is
empty

Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>

11 files changed, 173 insertions, 74 deletions

diff --git a/changelogs/fragments/65058-fix-fd-out-of-range-in-select.yml b/changelogs/fragments/65058-fix-fd-out-of-range-in-select.yml
new file mode 100644
index 0000000000..9fcdab6f26
--- /dev/null
+++ b/changelogs/fragments/65058-fix-fd-out-of-range-in-select.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - Avoid bare select() for running commands to avoid too large file descriptor numbers failing tasks
diff --git a/lib/ansible/compat/selectors/__init__.py b/lib/ansible/compat/selectors/__init__.py
index e47984065b..6bbf6d8b57 100644
--- a/lib/ansible/compat/selectors/__init__.py
+++ b/lib/ansible/compat/selectors/__init__.py
@@ -22,35 +22,9 @@ __metaclass__ = type
 '''
 Compat selectors library.  Python-3.5 has this builtin.  The selectors2
 package exists on pypi to backport the functionality as far as python-2.6.
+Implementation previously resided here - maintaining this file after the
+move to ansible.module_utils for code backwards compatibility.
 '''
-# The following makes it easier for us to script updates of the bundled code
-_BUNDLED_METADATA = {"pypi_name": "selectors2", "version": "1.1.1", "version_constraints": ">1.0,<2.0"}
-
-# Added these bugfix commits from 2.1.0:
-# * https://github.com/SethMichaelLarson/selectors2/commit/3bd74f2033363b606e1e849528ccaa76f5067590
-#   Wrap kqueue.control so that timeout is a keyword arg
-# * https://github.com/SethMichaelLarson/selectors2/commit/6f6a26f42086d8aab273b30be492beecb373646b
-#   Fix formatting of the kqueue.control patch for pylint
-# * https://github.com/SethMichaelLarson/selectors2/commit/f0c2c6c66cfa7662bc52beaf4e2d65adfa25e189
-#   Fix use of OSError exception for py3 and use the wrapper of kqueue.control so retries of
-#   interrupted syscalls work with kqueue
-
-import os.path
 import sys
-
-try:
-    # Python 3.4+
-    import selectors as _system_selectors
-except ImportError:
-    try:
-        # backport package installed in the system
-        import selectors2 as _system_selectors
-    except ImportError:
-        _system_selectors = None
-
-if _system_selectors:
-    selectors = _system_selectors
-else:
-    # Our bundled copy
-    from . import _selectors2 as selectors
+from ansible.module_utils.compat import selectors
 sys.modules['ansible.compat.selectors'] = selectors
diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py
index 9c0cc308a2..391171548d 100644
--- a/lib/ansible/module_utils/basic.py
+++ b/lib/ansible/module_utils/basic.py
@@ -81,6 +81,8 @@ except ImportError:
 # Python2 & 3 way to get NoneType
 NoneType = type(None)
 
+from ansible.module_utils.compat import selectors
+
 from ._text import to_native, to_bytes, to_text
 from ansible.module_utils.common.text.converters import (
     jsonify,
@@ -2329,15 +2331,6 @@ class AnsibleModule(object):
             self.fail_json(msg='Could not write data to file (%s) from (%s): %s' % (dest, src, to_native(e)),
                            exception=traceback.format_exc())
 
-    def _read_from_pipes(self, rpipes, rfds, file_descriptor):
-        data = b('')
-        if file_descriptor in rfds:
-            data = os.read(file_descriptor.fileno(), self.get_buffer_size(file_descriptor))
-            if data == b(''):
-                rpipes.remove(file_descriptor)
-
-        return data
-
     def _clean_args(self, args):
 
         if not self._clean:
@@ -2567,9 +2560,14 @@ class AnsibleModule(object):
             # the communication logic here is essentially taken from that
             # of the _communicate() function in ssh.py
 
-            stdout = b('')
-            stderr = b('')
-            rpipes = [cmd.stdout, cmd.stderr]
+            stdout = b''
+            stderr = b''
+            selector = selectors.DefaultSelector()
+            selector.register(cmd.stdout, selectors.EVENT_READ)
+            selector.register(cmd.stderr, selectors.EVENT_READ)
+            if os.name == 'posix':
+                fcntl.fcntl(cmd.stdout.fileno(), fcntl.F_SETFL, fcntl.fcntl(cmd.stdout.fileno(), fcntl.F_GETFL) | os.O_NONBLOCK)
+                fcntl.fcntl(cmd.stderr.fileno(), fcntl.F_SETFL, fcntl.fcntl(cmd.stderr.fileno(), fcntl.F_GETFL) | os.O_NONBLOCK)
 
             if data:
                 if not binary_data:
@@ -2580,9 +2578,15 @@ class AnsibleModule(object):
                 cmd.stdin.close()
 
             while True:
-                rfds, wfds, efds = select.select(rpipes, [], rpipes, 1)
-                stdout += self._read_from_pipes(rpipes, rfds, cmd.stdout)
-                stderr += self._read_from_pipes(rpipes, rfds, cmd.stderr)
+                events = selector.select(1)
+                for key, event in events:
+                    b_chunk = key.fileobj.read()
+                    if b_chunk == b(''):
+                        selector.unregister(key.fileobj)
+                    if key.fileobj == cmd.stdout:
+                        stdout += b_chunk
+                    elif key.fileobj == cmd.stderr:
+                        stderr += b_chunk
                 # if we're checking for prompts, do it now
                 if prompt_re:
                     if prompt_re.search(stdout) and not data:
@@ -2592,12 +2596,12 @@ class AnsibleModule(object):
                 # only break out if no pipes are left to read or
                 # the pipes are completely read and
                 # the process is terminated
-                if (not rpipes or not rfds) and cmd.poll() is not None:
+                if (not events or not selector.get_map()) and cmd.poll() is not None:
                     break
                 # No pipes are left to read but process is not yet terminated
                 # Only then it is safe to wait for the process to be finished
-                # NOTE: Actually cmd.poll() is always None here if rpipes is empty
-                elif not rpipes and cmd.poll() is None:
+                # NOTE: Actually cmd.poll() is always None here if no selectors are left
+                elif not selector.get_map() and cmd.poll() is None:
                     cmd.wait()
                     # The process is terminated. Since no pipes to read from are
                     # left, there is no need to call select() again.
@@ -2605,6 +2609,7 @@ class AnsibleModule(object):
 
             cmd.stdout.close()
             cmd.stderr.close()
+            selector.close()
 
             rc = cmd.returncode
         except (OSError, IOError) as e:
diff --git a/lib/ansible/compat/selectors/_selectors2.py b/lib/ansible/module_utils/compat/_selectors2.py
index be44b4b36f..be44b4b36f 100644
--- a/lib/ansible/compat/selectors/_selectors2.py
+++ b/lib/ansible/module_utils/compat/_selectors2.py
diff --git a/lib/ansible/module_utils/compat/selectors.py b/lib/ansible/module_utils/compat/selectors.py
new file mode 100644
index 0000000000..53996d7e01
--- /dev/null
+++ b/lib/ansible/module_utils/compat/selectors.py
@@ -0,0 +1,56 @@
+# (c) 2014, 2017 Toshio Kuratomi <tkuratomi@ansible.com>
+#
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+
+# Make coding more python3-ish
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+'''
+Compat selectors library.  Python-3.5 has this builtin.  The selectors2
+package exists on pypi to backport the functionality as far as python-2.6.
+'''
+# The following makes it easier for us to script updates of the bundled code
+_BUNDLED_METADATA = {"pypi_name": "selectors2", "version": "1.1.1", "version_constraints": ">1.0,<2.0"}
+
+# Added these bugfix commits from 2.1.0:
+# * https://github.com/SethMichaelLarson/selectors2/commit/3bd74f2033363b606e1e849528ccaa76f5067590
+#   Wrap kqueue.control so that timeout is a keyword arg
+# * https://github.com/SethMichaelLarson/selectors2/commit/6f6a26f42086d8aab273b30be492beecb373646b
+#   Fix formatting of the kqueue.control patch for pylint
+# * https://github.com/SethMichaelLarson/selectors2/commit/f0c2c6c66cfa7662bc52beaf4e2d65adfa25e189
+#   Fix use of OSError exception for py3 and use the wrapper of kqueue.control so retries of
+#   interrupted syscalls work with kqueue
+
+import os.path
+import sys
+
+try:
+    # Python 3.4+
+    import selectors as _system_selectors
+except ImportError:
+    try:
+        # backport package installed in the system
+        import selectors2 as _system_selectors
+    except ImportError:
+        _system_selectors = None
+
+if _system_selectors:
+    selectors = _system_selectors
+else:
+    # Our bundled copy
+    from ansible.module_utils.compat import _selectors2 as selectors
+sys.modules['ansible.module_utils.compat.selectors'] = selectors
diff --git a/lib/ansible/plugins/connection/local.py b/lib/ansible/plugins/connection/local.py
index e37661030f..297e3d3204 100644
--- a/lib/ansible/plugins/connection/local.py
+++ b/lib/ansible/plugins/connection/local.py
@@ -23,8 +23,8 @@ import fcntl
 import getpass
 
 import ansible.constants as C
-from ansible.compat import selectors
 from ansible.errors import AnsibleError, AnsibleFileNotFound
+from ansible.module_utils.compat import selectors
 from ansible.module_utils.six import text_type, binary_type
 from ansible.module_utils._text import to_bytes, to_native, to_text
 from ansible.plugins.connection import ConnectionBase
diff --git a/lib/ansible/plugins/connection/ssh.py b/lib/ansible/plugins/connection/ssh.py
index 5e4bb5c190..479e1919ce 100644
--- a/lib/ansible/plugins/connection/ssh.py
+++ b/lib/ansible/plugins/connection/ssh.py
@@ -289,7 +289,7 @@ from ansible.errors import (
     AnsibleFileNotFound,
 )
 from ansible.errors import AnsibleOptionsError
-from ansible.compat import selectors
+from ansible.module_utils.compat import selectors
 from ansible.module_utils.six import PY3, text_type, binary_type
 from ansible.module_utils.six.moves import shlex_quote
 from ansible.module_utils._text import to_bytes, to_native, to_text
diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt
index 29b5726d5e..113f32fcd6 100644
--- a/test/sanity/ignore.txt
+++ b/test/sanity/ignore.txt
@@ -43,9 +43,6 @@ hacking/tests/gen_distribution_version_testcase.py metaclass-boilerplate
 lib/ansible/cli/console.py pylint:blacklisted-name
 lib/ansible/cli/scripts/ansible_cli_stub.py shebang
 lib/ansible/cli/scripts/ansible_connection_cli_stub.py shebang
-lib/ansible/compat/selectors/_selectors2.py future-import-boilerplate # ignore bundled
-lib/ansible/compat/selectors/_selectors2.py metaclass-boilerplate # ignore bundled
-lib/ansible/compat/selectors/_selectors2.py pylint:blacklisted-name
 lib/ansible/config/base.yml no-unwanted-files
 lib/ansible/config/module_defaults.yml no-unwanted-files
 lib/ansible/executor/playbook_executor.py pylint:blacklisted-name
@@ -60,6 +57,9 @@ lib/ansible/module_utils/api.py metaclass-boilerplate
 lib/ansible/module_utils/basic.py metaclass-boilerplate
 lib/ansible/module_utils/common/network.py future-import-boilerplate
 lib/ansible/module_utils/common/network.py metaclass-boilerplate
+lib/ansible/module_utils/compat/_selectors2.py future-import-boilerplate # ignore bundled
+lib/ansible/module_utils/compat/_selectors2.py metaclass-boilerplate # ignore bundled
+lib/ansible/module_utils/compat/_selectors2.py pylint:blacklisted-name
 lib/ansible/module_utils/connection.py future-import-boilerplate
 lib/ansible/module_utils/connection.py metaclass-boilerplate
 lib/ansible/module_utils/distro/__init__.py empty-init # breaks namespacing, bundled, do not override
diff --git a/test/units/executor/module_common/test_recursive_finder.py b/test/units/executor/module_common/test_recursive_finder.py
index b57de77acb..8ae6dc9eaa 100644
--- a/test/units/executor/module_common/test_recursive_finder.py
+++ b/test/units/executor/module_common/test_recursive_finder.py
@@ -54,6 +54,9 @@ MODULE_UTILS_BASIC_IMPORTS = frozenset((('ansible', '__init__'),
                                         ('ansible', 'module_utils', 'common', 'text', 'formatters'),
                                         ('ansible', 'module_utils', 'common', 'validation'),
                                         ('ansible', 'module_utils', 'common', '_utils'),
+                                        ('ansible', 'module_utils', 'compat', '__init__'),
+                                        ('ansible', 'module_utils', 'compat', '_selectors2'),
+                                        ('ansible', 'module_utils', 'compat', 'selectors'),
                                         ('ansible', 'module_utils', 'distro', '__init__'),
                                         ('ansible', 'module_utils', 'distro', '_distro'),
                                         ('ansible', 'module_utils', 'parsing', '__init__'),
@@ -81,6 +84,9 @@ MODULE_UTILS_BASIC_FILES = frozenset(('ansible/module_utils/_text.py',
                                       'ansible/module_utils/common/text/formatters.py',
                                       'ansible/module_utils/common/validation.py',
                                       'ansible/module_utils/common/_utils.py',
+                                      'ansible/module_utils/compat/__init__.py',
+                                      'ansible/module_utils/compat/_selectors2.py',
+                                      'ansible/module_utils/compat/selectors.py',
                                       'ansible/module_utils/distro/__init__.py',
                                       'ansible/module_utils/distro/_distro.py',
                                       'ansible/module_utils/parsing/__init__.py',
diff --git a/test/units/module_utils/basic/test_run_command.py b/test/units/module_utils/basic/test_run_command.py
index 3e5e874ad6..f28f3175f5 100644
--- a/test/units/module_utils/basic/test_run_command.py
+++ b/test/units/module_utils/basic/test_run_command.py
@@ -14,6 +14,7 @@ import pytest
 
 from ansible.module_utils._text import to_native
 from ansible.module_utils.six import PY2
+from ansible.module_utils.compat import selectors
 
 
 class OpenBytesIO(BytesIO):
@@ -28,9 +29,6 @@ class OpenBytesIO(BytesIO):
 
 @pytest.fixture
 def mock_os(mocker):
-    def mock_os_read(fd, nbytes):
-        return os._cmd_out[fd].read(nbytes)
-
     def mock_os_chdir(path):
         if path == '/inaccessible':
             raise OSError(errno.EPERM, "Permission denied: '/inaccessible'")
@@ -42,11 +40,6 @@ def mock_os(mocker):
             return os.getcwd.return_value + '/' + path
 
     os = mocker.patch('ansible.module_utils.basic.os')
-    os._cmd_out = {
-        # os.read() is returning 'bytes', not strings
-        mocker.sentinel.stdout: BytesIO(),
-        mocker.sentinel.stderr: BytesIO(),
-    }
 
     os.path.expandvars.side_effect = lambda x: x
     os.path.expanduser.side_effect = lambda x: x
@@ -54,26 +47,79 @@ def mock_os(mocker):
     os.getcwd.return_value = '/home/foo'
     os.path.isdir.return_value = True
     os.chdir.side_effect = mock_os_chdir
-    os.read.side_effect = mock_os_read
     os.path.abspath.side_effect = mock_os_abspath
 
     yield os
 
 
+class DummyFileObj():
+    def __init__(self, fileobj):
+        self.fileobj = fileobj
+
+
+class SpecialBytesIO(BytesIO):
+    def __init__(self, *args, **kwargs):
+        fh = kwargs.pop('fh', None)
+        super(SpecialBytesIO, self).__init__(*args, **kwargs)
+        self.fh = fh
+
+    def fileno(self):
+        return self.fh
+
+    # We need to do this because some of our tests create a new value for stdout and stderr
+    # The new value is able to affect the string that is returned by the subprocess stdout and
+    # stderr but by the time the test gets it, it is too late to change the SpecialBytesIO that
+    # subprocess.Popen returns for stdout and stderr.  If we could figure out how to change those as
+    # well, then we wouldn't need this.
+    def __eq__(self, other):
+        if id(self) == id(other) or self.fh == other.fileno():
+            return True
+        return False
+
+
+class DummyKey:
+    def __init__(self, fileobj):
+        self.fileobj = fileobj
+
+
 @pytest.fixture
 def mock_subprocess(mocker):
-    def mock_select(rlist, wlist, xlist, timeout=1):
-        return (rlist, [], [])
 
-    fake_select = mocker.patch('ansible.module_utils.basic.select')
-    fake_select.select.side_effect = mock_select
+    class MockSelector(selectors.BaseSelector):
+        def __init__(self):
+            super(MockSelector, self).__init__()
+            self._file_objs = []
+
+        def register(self, fileobj, events, data=None):
+            self._file_objs.append(fileobj)
+
+        def unregister(self, fileobj):
+            self._file_objs.remove(fileobj)
+
+        def select(self, timeout=None):
+            ready = []
+            for file_obj in self._file_objs:
+                ready.append((DummyKey(subprocess._output[file_obj.fileno()]), selectors.EVENT_READ))
+            return ready
+
+        def get_map(self):
+            return self._file_objs
+
+        def close(self):
+            super(MockSelector, self).close()
+            self._file_objs = []
+
+    selectors.DefaultSelector = MockSelector
 
     subprocess = mocker.patch('ansible.module_utils.basic.subprocess')
+    subprocess._output = {mocker.sentinel.stdout: SpecialBytesIO(b'', fh=mocker.sentinel.stdout),
+                          mocker.sentinel.stderr: SpecialBytesIO(b'', fh=mocker.sentinel.stderr)}
+
     cmd = mocker.MagicMock()
     cmd.returncode = 0
     cmd.stdin = OpenBytesIO()
-    cmd.stdout.fileno.return_value = mocker.sentinel.stdout
-    cmd.stderr.fileno.return_value = mocker.sentinel.stderr
+    cmd.stdout = subprocess._output[mocker.sentinel.stdout]
+    cmd.stderr = subprocess._output[mocker.sentinel.stderr]
     subprocess.Popen.return_value = cmd
 
     yield subprocess
@@ -84,7 +130,6 @@ def rc_am(mocker, am, mock_os, mock_subprocess):
     am.fail_json = mocker.MagicMock(side_effect=SystemExit)
     am._os = mock_os
     am._subprocess = mock_subprocess
-    am.get_buffer_size = mocker.MagicMock(return_value=900)
     yield am
 
 
@@ -151,7 +196,11 @@ class TestRunCommandPrompt:
 
     @pytest.mark.parametrize('stdin', [{}], indirect=['stdin'])
     def test_prompt_match_wo_data(self, mocker, rc_am):
-        rc_am._os._cmd_out[mocker.sentinel.stdout] = BytesIO(b'Authentication required!\nEnter password: ')
+        rc_am._subprocess._output = {mocker.sentinel.stdout:
+                                     SpecialBytesIO(b'Authentication required!\nEnter password: ',
+                                                    fh=mocker.sentinel.stdout),
+                                     mocker.sentinel.stderr:
+                                     SpecialBytesIO(b'', fh=mocker.sentinel.stderr)}
         (rc, _, _) = rc_am.run_command('foo', prompt_regex=r'[pP]assword:', data=None)
         assert rc == 257
 
@@ -181,7 +230,10 @@ class TestRunCommandOutput:
 
     @pytest.mark.parametrize('stdin', [{}], indirect=['stdin'])
     def test_ascii_stdout(self, mocker, rc_am):
-        rc_am._os._cmd_out[mocker.sentinel.stdout] = BytesIO(b'hello')
+        rc_am._subprocess._output = {mocker.sentinel.stdout:
+                                     SpecialBytesIO(b'hello', fh=mocker.sentinel.stdout),
+                                     mocker.sentinel.stderr:
+                                     SpecialBytesIO(b'', fh=mocker.sentinel.stderr)}
         (rc, stdout, stderr) = rc_am.run_command('/bin/cat hello.txt')
         assert rc == 0
         # module_utils function.  On py3 it returns text and py2 it returns
@@ -190,8 +242,12 @@ class TestRunCommandOutput:
 
     @pytest.mark.parametrize('stdin', [{}], indirect=['stdin'])
     def test_utf8_output(self, mocker, rc_am):
-        rc_am._os._cmd_out[mocker.sentinel.stdout] = BytesIO(u'Žarn§'.encode('utf-8'))
-        rc_am._os._cmd_out[mocker.sentinel.stderr] = BytesIO(u'لرئيسية'.encode('utf-8'))
+        rc_am._subprocess._output = {mocker.sentinel.stdout:
+                                     SpecialBytesIO(u'Žarn§'.encode('utf-8'),
+                                                    fh=mocker.sentinel.stdout),
+                                     mocker.sentinel.stderr:
+                                     SpecialBytesIO(u'لرئيسية'.encode('utf-8'),
+                                                    fh=mocker.sentinel.stderr)}
         (rc, stdout, stderr) = rc_am.run_command('/bin/something_ugly')
         assert rc == 0
         # module_utils function.  On py3 it returns text and py2 it returns
diff --git a/test/units/plugins/connection/test_ssh.py b/test/units/plugins/connection/test_ssh.py
index 2a8f7b90cc..cfe7fcb6b8 100644
--- a/test/units/plugins/connection/test_ssh.py
+++ b/test/units/plugins/connection/test_ssh.py
@@ -26,10 +26,10 @@ import pytest
 
 from ansible import constants as C
 from ansible.errors import AnsibleAuthenticationFailure
-from ansible.compat.selectors import SelectorKey, EVENT_READ
 from units.compat import unittest
 from units.compat.mock import patch, MagicMock, PropertyMock
 from ansible.errors import AnsibleError, AnsibleConnectionFailure, AnsibleFileNotFound
+from ansible.module_utils.compat.selectors import SelectorKey, EVENT_READ
 from ansible.module_utils.six.moves import shlex_quote
 from ansible.module_utils._text import to_bytes
 from ansible.playbook.play_context import PlayContext
@@ -380,7 +380,7 @@ def mock_run_env(request, mocker):
     request.cls.mock_popen = mock_popen
 
     request.cls.mock_selector = MockSelector()
-    mocker.patch('ansible.compat.selectors.DefaultSelector', lambda: request.cls.mock_selector)
+    mocker.patch('ansible.module_utils.compat.selectors.DefaultSelector', lambda: request.cls.mock_selector)
 
     request.cls.mock_openpty = mocker.patch('pty.openpty')