diff --git a/lib/ansible/inventory/dir.py b/lib/ansible/inventory/dir.py index 7287f7804d..bb25a27121 100644 --- a/lib/ansible/inventory/dir.py +++ b/lib/ansible/inventory/dir.py @@ -45,11 +45,10 @@ def get_file_parser(hostsfile, groups, loader): parser = None try: - inv_file = open(hostsfile) - first_line = inv_file.readlines()[0] - inv_file.close() - if first_line.startswith('#!'): - shebang_present = True + with open(hostsfile, 'rb') as inv_file: + initial_chars = inv_file.read(2) + if initial_chars.startswith(b'#!'): + shebang_present = True except: pass diff --git a/lib/ansible/inventory/ini.py b/lib/ansible/inventory/ini.py index 81c3ec90e4..c609ad0e4e 100644 --- a/lib/ansible/inventory/ini.py +++ b/lib/ansible/inventory/ini.py @@ -40,7 +40,6 @@ class InventoryParser(object): """ def __init__(self, loader, groups, filename=C.DEFAULT_HOST_LIST): - self._loader = loader self.filename = filename # Start with an empty host list and whatever groups we're passed in @@ -53,12 +52,17 @@ class InventoryParser(object): # Read in the hosts, groups, and variables defined in the # inventory file. - if loader: - (data, private) = loader._get_file_contents(filename) - else: - with open(filename) as fh: - data = to_text(fh.read()) - data = data.split('\n') + with open(filename, 'rb') as fh: + data = fh.read() + try: + # Faster to do to_text once on a long string than many + # times on smaller strings + data = to_text(data, errors='surrogate_or_strict') + data = [line for line in data.splitlines() if not (line.startswith(u';') or line.startswith(u'#'))] + except UnicodeError: + # Skip comment lines here to avoid potential undecodable + # errors in comments: https://github.com/ansible/ansible/issues/17593 + data = [to_text(line, errors='surrogate_or_strict') for line in data.splitlines() if not (line.startswith(b';') or line.startswith(b'#'))] self._parse(data) @@ -88,8 +92,8 @@ class InventoryParser(object): line = line.strip() - # Skip empty lines and comments - if line == '' or line.startswith(";") or line.startswith("#"): + # Skip empty lines + if not line: continue # Is this a [section] header? That tells us what group we're parsing diff --git a/test/units/vars/test_variable_manager.py b/test/units/vars/test_variable_manager.py index 25e8349c16..3c3c8e9c62 100644 --- a/test/units/vars/test_variable_manager.py +++ b/test/units/vars/test_variable_manager.py @@ -20,17 +20,20 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type from collections import defaultdict -from six import iteritems +from ansible.compat.six import iteritems +from ansible.compat.six.moves import builtins from ansible.compat.tests import unittest -from ansible.compat.tests.mock import patch, MagicMock +from ansible.compat.tests.mock import MagicMock, mock_open, patch from ansible.inventory import Inventory from ansible.playbook.play import Play -from ansible.vars import VariableManager from units.mock.loader import DictDataLoader from units.mock.path import mock_unfrackpath_noop +from ansible.vars import VariableManager + + class TestVariableManager(unittest.TestCase): def setUp(self): @@ -192,9 +195,7 @@ class TestVariableManager(unittest.TestCase): v = VariableManager() v._fact_cache = defaultdict(dict) - fake_loader = DictDataLoader({ - # inventory1 - '/etc/ansible/inventory1': """ + inventory1_filedata = """ [group2:children] group1 @@ -206,8 +207,11 @@ class TestVariableManager(unittest.TestCase): [group2:vars] group_var = group_var_from_inventory_group2 - """, + """ + fake_loader = DictDataLoader({ + # inventory1 + '/etc/ansible/inventory1': inventory1_filedata, # role defaults_only1 '/etc/ansible/roles/defaults_only1/defaults/main.yml': """ default_var: "default_var_from_defaults_only1" @@ -231,7 +235,8 @@ class TestVariableManager(unittest.TestCase): }) mock_basedir.return_value = './' - inv1 = Inventory(loader=fake_loader, variable_manager=v, host_list='/etc/ansible/inventory1') + with patch.object(builtins, 'open', mock_open(read_data=inventory1_filedata)): + inv1 = Inventory(loader=fake_loader, variable_manager=v, host_list='/etc/ansible/inventory1') inv1.set_playbook_basedir('./') play1 = Play.load(dict(