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>
This commit is contained in:
Bob Weinand 2020-05-14 17:46:34 +02:00 committed by GitHub
parent 87d9b49de2
commit f200487414
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 173 additions and 74 deletions

View file

@ -0,0 +1,2 @@
bugfixes:
- Avoid bare select() for running commands to avoid too large file descriptor numbers failing tasks

View file

@ -22,35 +22,9 @@ __metaclass__ = type
''' '''
Compat selectors library. Python-3.5 has this builtin. The selectors2 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. 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 import sys
from ansible.module_utils.compat import selectors
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
sys.modules['ansible.compat.selectors'] = selectors sys.modules['ansible.compat.selectors'] = selectors

View file

@ -81,6 +81,8 @@ except ImportError:
# Python2 & 3 way to get NoneType # Python2 & 3 way to get NoneType
NoneType = type(None) NoneType = type(None)
from ansible.module_utils.compat import selectors
from ._text import to_native, to_bytes, to_text from ._text import to_native, to_bytes, to_text
from ansible.module_utils.common.text.converters import ( from ansible.module_utils.common.text.converters import (
jsonify, 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)), self.fail_json(msg='Could not write data to file (%s) from (%s): %s' % (dest, src, to_native(e)),
exception=traceback.format_exc()) 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): def _clean_args(self, args):
if not self._clean: if not self._clean:
@ -2567,9 +2560,14 @@ class AnsibleModule(object):
# the communication logic here is essentially taken from that # the communication logic here is essentially taken from that
# of the _communicate() function in ssh.py # of the _communicate() function in ssh.py
stdout = b('') stdout = b''
stderr = b('') stderr = b''
rpipes = [cmd.stdout, cmd.stderr] 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 data:
if not binary_data: if not binary_data:
@ -2580,9 +2578,15 @@ class AnsibleModule(object):
cmd.stdin.close() cmd.stdin.close()
while True: while True:
rfds, wfds, efds = select.select(rpipes, [], rpipes, 1) events = selector.select(1)
stdout += self._read_from_pipes(rpipes, rfds, cmd.stdout) for key, event in events:
stderr += self._read_from_pipes(rpipes, rfds, cmd.stderr) 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 we're checking for prompts, do it now
if prompt_re: if prompt_re:
if prompt_re.search(stdout) and not data: 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 # only break out if no pipes are left to read or
# the pipes are completely read and # the pipes are completely read and
# the process is terminated # 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 break
# No pipes are left to read but process is not yet terminated # 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 # 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 # NOTE: Actually cmd.poll() is always None here if no selectors are left
elif not rpipes and cmd.poll() is None: elif not selector.get_map() and cmd.poll() is None:
cmd.wait() cmd.wait()
# The process is terminated. Since no pipes to read from are # The process is terminated. Since no pipes to read from are
# left, there is no need to call select() again. # left, there is no need to call select() again.
@ -2605,6 +2609,7 @@ class AnsibleModule(object):
cmd.stdout.close() cmd.stdout.close()
cmd.stderr.close() cmd.stderr.close()
selector.close()
rc = cmd.returncode rc = cmd.returncode
except (OSError, IOError) as e: except (OSError, IOError) as e:

View file

@ -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

View file

@ -23,8 +23,8 @@ import fcntl
import getpass import getpass
import ansible.constants as C import ansible.constants as C
from ansible.compat import selectors
from ansible.errors import AnsibleError, AnsibleFileNotFound 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.six import text_type, binary_type
from ansible.module_utils._text import to_bytes, to_native, to_text from ansible.module_utils._text import to_bytes, to_native, to_text
from ansible.plugins.connection import ConnectionBase from ansible.plugins.connection import ConnectionBase

View file

@ -289,7 +289,7 @@ from ansible.errors import (
AnsibleFileNotFound, AnsibleFileNotFound,
) )
from ansible.errors import AnsibleOptionsError 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 import PY3, text_type, binary_type
from ansible.module_utils.six.moves import shlex_quote from ansible.module_utils.six.moves import shlex_quote
from ansible.module_utils._text import to_bytes, to_native, to_text from ansible.module_utils._text import to_bytes, to_native, to_text

View file

@ -43,9 +43,6 @@ hacking/tests/gen_distribution_version_testcase.py metaclass-boilerplate
lib/ansible/cli/console.py pylint:blacklisted-name lib/ansible/cli/console.py pylint:blacklisted-name
lib/ansible/cli/scripts/ansible_cli_stub.py shebang lib/ansible/cli/scripts/ansible_cli_stub.py shebang
lib/ansible/cli/scripts/ansible_connection_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/base.yml no-unwanted-files
lib/ansible/config/module_defaults.yml no-unwanted-files lib/ansible/config/module_defaults.yml no-unwanted-files
lib/ansible/executor/playbook_executor.py pylint:blacklisted-name 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/basic.py metaclass-boilerplate
lib/ansible/module_utils/common/network.py future-import-boilerplate lib/ansible/module_utils/common/network.py future-import-boilerplate
lib/ansible/module_utils/common/network.py metaclass-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 future-import-boilerplate
lib/ansible/module_utils/connection.py metaclass-boilerplate lib/ansible/module_utils/connection.py metaclass-boilerplate
lib/ansible/module_utils/distro/__init__.py empty-init # breaks namespacing, bundled, do not override lib/ansible/module_utils/distro/__init__.py empty-init # breaks namespacing, bundled, do not override

View file

@ -54,6 +54,9 @@ MODULE_UTILS_BASIC_IMPORTS = frozenset((('ansible', '__init__'),
('ansible', 'module_utils', 'common', 'text', 'formatters'), ('ansible', 'module_utils', 'common', 'text', 'formatters'),
('ansible', 'module_utils', 'common', 'validation'), ('ansible', 'module_utils', 'common', 'validation'),
('ansible', 'module_utils', 'common', '_utils'), ('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', '__init__'),
('ansible', 'module_utils', 'distro', '_distro'), ('ansible', 'module_utils', 'distro', '_distro'),
('ansible', 'module_utils', 'parsing', '__init__'), ('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/text/formatters.py',
'ansible/module_utils/common/validation.py', 'ansible/module_utils/common/validation.py',
'ansible/module_utils/common/_utils.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/__init__.py',
'ansible/module_utils/distro/_distro.py', 'ansible/module_utils/distro/_distro.py',
'ansible/module_utils/parsing/__init__.py', 'ansible/module_utils/parsing/__init__.py',

View file

@ -14,6 +14,7 @@ import pytest
from ansible.module_utils._text import to_native from ansible.module_utils._text import to_native
from ansible.module_utils.six import PY2 from ansible.module_utils.six import PY2
from ansible.module_utils.compat import selectors
class OpenBytesIO(BytesIO): class OpenBytesIO(BytesIO):
@ -28,9 +29,6 @@ class OpenBytesIO(BytesIO):
@pytest.fixture @pytest.fixture
def mock_os(mocker): def mock_os(mocker):
def mock_os_read(fd, nbytes):
return os._cmd_out[fd].read(nbytes)
def mock_os_chdir(path): def mock_os_chdir(path):
if path == '/inaccessible': if path == '/inaccessible':
raise OSError(errno.EPERM, "Permission denied: '/inaccessible'") raise OSError(errno.EPERM, "Permission denied: '/inaccessible'")
@ -42,11 +40,6 @@ def mock_os(mocker):
return os.getcwd.return_value + '/' + path return os.getcwd.return_value + '/' + path
os = mocker.patch('ansible.module_utils.basic.os') 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.expandvars.side_effect = lambda x: x
os.path.expanduser.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.getcwd.return_value = '/home/foo'
os.path.isdir.return_value = True os.path.isdir.return_value = True
os.chdir.side_effect = mock_os_chdir os.chdir.side_effect = mock_os_chdir
os.read.side_effect = mock_os_read
os.path.abspath.side_effect = mock_os_abspath os.path.abspath.side_effect = mock_os_abspath
yield os 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 @pytest.fixture
def mock_subprocess(mocker): def mock_subprocess(mocker):
def mock_select(rlist, wlist, xlist, timeout=1):
return (rlist, [], [])
fake_select = mocker.patch('ansible.module_utils.basic.select') class MockSelector(selectors.BaseSelector):
fake_select.select.side_effect = mock_select 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 = 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 = mocker.MagicMock()
cmd.returncode = 0 cmd.returncode = 0
cmd.stdin = OpenBytesIO() cmd.stdin = OpenBytesIO()
cmd.stdout.fileno.return_value = mocker.sentinel.stdout cmd.stdout = subprocess._output[mocker.sentinel.stdout]
cmd.stderr.fileno.return_value = mocker.sentinel.stderr cmd.stderr = subprocess._output[mocker.sentinel.stderr]
subprocess.Popen.return_value = cmd subprocess.Popen.return_value = cmd
yield subprocess yield subprocess
@ -84,7 +130,6 @@ def rc_am(mocker, am, mock_os, mock_subprocess):
am.fail_json = mocker.MagicMock(side_effect=SystemExit) am.fail_json = mocker.MagicMock(side_effect=SystemExit)
am._os = mock_os am._os = mock_os
am._subprocess = mock_subprocess am._subprocess = mock_subprocess
am.get_buffer_size = mocker.MagicMock(return_value=900)
yield am yield am
@ -151,7 +196,11 @@ class TestRunCommandPrompt:
@pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) @pytest.mark.parametrize('stdin', [{}], indirect=['stdin'])
def test_prompt_match_wo_data(self, mocker, rc_am): 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) (rc, _, _) = rc_am.run_command('foo', prompt_regex=r'[pP]assword:', data=None)
assert rc == 257 assert rc == 257
@ -181,7 +230,10 @@ class TestRunCommandOutput:
@pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) @pytest.mark.parametrize('stdin', [{}], indirect=['stdin'])
def test_ascii_stdout(self, mocker, rc_am): 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') (rc, stdout, stderr) = rc_am.run_command('/bin/cat hello.txt')
assert rc == 0 assert rc == 0
# module_utils function. On py3 it returns text and py2 it returns # module_utils function. On py3 it returns text and py2 it returns
@ -190,8 +242,12 @@ class TestRunCommandOutput:
@pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) @pytest.mark.parametrize('stdin', [{}], indirect=['stdin'])
def test_utf8_output(self, mocker, rc_am): def test_utf8_output(self, mocker, rc_am):
rc_am._os._cmd_out[mocker.sentinel.stdout] = BytesIO(u'Žarn§'.encode('utf-8')) rc_am._subprocess._output = {mocker.sentinel.stdout:
rc_am._os._cmd_out[mocker.sentinel.stderr] = BytesIO(u'لرئيسية'.encode('utf-8')) 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') (rc, stdout, stderr) = rc_am.run_command('/bin/something_ugly')
assert rc == 0 assert rc == 0
# module_utils function. On py3 it returns text and py2 it returns # module_utils function. On py3 it returns text and py2 it returns

View file

@ -26,10 +26,10 @@ import pytest
from ansible import constants as C from ansible import constants as C
from ansible.errors import AnsibleAuthenticationFailure from ansible.errors import AnsibleAuthenticationFailure
from ansible.compat.selectors import SelectorKey, EVENT_READ
from units.compat import unittest from units.compat import unittest
from units.compat.mock import patch, MagicMock, PropertyMock from units.compat.mock import patch, MagicMock, PropertyMock
from ansible.errors import AnsibleError, AnsibleConnectionFailure, AnsibleFileNotFound 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.six.moves import shlex_quote
from ansible.module_utils._text import to_bytes from ansible.module_utils._text import to_bytes
from ansible.playbook.play_context import PlayContext 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_popen = mock_popen
request.cls.mock_selector = MockSelector() 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') request.cls.mock_openpty = mocker.patch('pty.openpty')