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 00000000000..9fcdab6f26a --- /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 e47984065b1..6bbf6d8b574 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 9c0cc308a28..391171548de 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 similarity index 100% rename from lib/ansible/compat/selectors/_selectors2.py rename to 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 00000000000..53996d7e017 --- /dev/null +++ b/lib/ansible/module_utils/compat/selectors.py @@ -0,0 +1,56 @@ +# (c) 2014, 2017 Toshio Kuratomi +# +# 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 . + +# 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 e37661030f7..297e3d32041 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 5e4bb5c1903..479e1919cee 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 29b5726d5ec..113f32fcd6b 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 b57de77acbc..8ae6dc9eaaa 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 3e5e874ad63..f28f3175f59 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 2a8f7b90cce..cfe7fcb6b81 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')