Make ini parsing slightly more robust

Prior to this commit, the ini parser would fail if the inventory was
not 100% utf-8.  This commit makes this slightly more robust by
omitting full line comments from that requirement.

Fixes #17593

3 files changed, 30 insertions, 22 deletions

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(