2 unicode-compatibility fixes for cmdmod.py

First, `shlex.split()` will raise an exception when passed a unicode
type with unicode characters in the string. This modifies our
`shlex.split()` helper to first convert the passed string to a `str`
type, and then return a decoded copy of the result of the split.

Second, this uses our `to_unicode` helper to more gracefully decode the
stdout and stderr from the command. Unit tests have been added to
confirm that the output is properly decoded, including instances where
decoding fails because the return from the command contains binary data.

salt/modules/cmdmod.py

@@ -281,8 +281,9 @@ def _run(cmd,
     if _is_valid_shell(shell) is False:
         log.warning(
             'Attempt to run a shell command with what may be an invalid shell! '
-            'Check to ensure that the shell <{0}> is valid for this user.'
-            .format(shell))
+            'Check to ensure that the shell <%s> is valid for this user.',
+            shell
+        )
 
     log_callback = _check_cb(log_callback)
 
@@ -347,14 +348,15 @@ def _run(cmd,
     # checked if blacklisted
     if '__pub_jid' in kwargs:
         if not _check_avail(cmd):
-            msg = 'This shell command is not permitted: "{0}"'.format(cmd)
-            raise CommandExecutionError(msg)
+            raise CommandExecutionError(
+                'The shell command "{0}" is not permitted'.format(cmd)
+            )
 
     env = _parse_env(env)
 
     for bad_env_key in (x for x, y in six.iteritems(env) if y is None):
-        log.error('Environment variable \'{0}\' passed without a value. '
-                  'Setting value to an empty string'.format(bad_env_key))
+        log.error('Environment variable \'%s\' passed without a value. '
+                  'Setting value to an empty string', bad_env_key)
         env[bad_env_key] = ''
 
     def _get_stripped(cmd):
@@ -504,8 +506,7 @@ def _run(cmd,
         try:
             _umask = int(_umask, 8)
         except ValueError:
-            msg = 'Invalid umask: \'{0}\''.format(umask)
-            raise CommandExecutionError(msg)
+            raise CommandExecutionError("Invalid umask: '{0}'".format(umask))
     else:
         _umask = None
 
@@ -570,20 +571,28 @@ def _run(cmd,
             return ret
 
         try:
-            out = proc.stdout.decode(__salt_system_encoding__)
-        except AttributeError:
+            out = salt.utils.stringutils.to_unicode(proc.stdout)
+        except TypeError:
+            # stdout is None
             out = ''
         except UnicodeDecodeError:
-            log.error('UnicodeDecodeError while decoding output of cmd {0}'.format(cmd))
-            out = proc.stdout.decode(__salt_system_encoding__, 'replace')
+            log.error(
+                'Failed to decode stdout from command %s, non-decodable '
+                'characters have been replaced', cmd
+            )
+            out = salt.utils.stringutils.to_unicode(proc.stdout, errors='replace')
 
         try:
-            err = proc.stderr.decode(__salt_system_encoding__)
-        except AttributeError:
+            err = salt.utils.stringutils.to_unicode(proc.stderr)
+        except TypeError:
+            # stderr is None
             err = ''
         except UnicodeDecodeError:
-            log.error('UnicodeDecodeError while decoding error of cmd {0}'.format(cmd))
-            err = proc.stderr.decode(__salt_system_encoding__, 'replace')
+            log.error(
+                'Failed to decode stderr from command %s, non-decodable '
+                'characters have been replaced', cmd
+            )
+            err = salt.utils.stringutils.to_unicode(proc.stderr, errors='replace')
 
         if rstrip:
             if out is not None:
@@ -648,9 +657,8 @@ def _run(cmd,
                         ret['retcode'] = 1
                         break
                 except salt.utils.vt.TerminalException as exc:
-                    log.error(
-                        'VT: {0}'.format(exc),
-                        exc_info_on_loglevel=logging.DEBUG)
+                    log.error('VT: %s', exc,
+                              exc_info_on_loglevel=logging.DEBUG)
                     ret = {'retcode': 1, 'pid': '2'}
                     break
                 # only set stdout on success as we already mangled in other
@@ -1401,11 +1409,11 @@ def run_stdout(cmd,
             )
             log.error(log_callback(msg))
         if ret['stdout']:
-            log.log(lvl, 'stdout: {0}'.format(log_callback(ret['stdout'])))
+            log.log(lvl, 'stdout: %s', log_callback(ret['stdout']))
         if ret['stderr']:
-            log.log(lvl, 'stderr: {0}'.format(log_callback(ret['stderr'])))
+            log.log(lvl, 'stderr: %s', log_callback(ret['stderr']))
         if ret['retcode']:
-            log.log(lvl, 'retcode: {0}'.format(ret['retcode']))
+            log.log(lvl, 'retcode: %s', ret['retcode'])
     return ret['stdout'] if not hide_output else ''
 
 
@@ -1603,11 +1611,11 @@ def run_stderr(cmd,
             )
             log.error(log_callback(msg))
         if ret['stdout']:
-            log.log(lvl, 'stdout: {0}'.format(log_callback(ret['stdout'])))
+            log.log(lvl, 'stdout: %s', log_callback(ret['stdout']))
         if ret['stderr']:
-            log.log(lvl, 'stderr: {0}'.format(log_callback(ret['stderr'])))
+            log.log(lvl, 'stderr: %s', log_callback(ret['stderr']))
         if ret['retcode']:
-            log.log(lvl, 'retcode: {0}'.format(ret['retcode']))
+            log.log(lvl, 'retcode: %s', ret['retcode'])
     return ret['stderr'] if not hide_output else ''
 
 
@@ -1832,11 +1840,11 @@ def run_all(cmd,
             )
             log.error(log_callback(msg))
         if ret['stdout']:
-            log.log(lvl, 'stdout: {0}'.format(log_callback(ret['stdout'])))
+            log.log(lvl, 'stdout: %s', log_callback(ret['stdout']))
         if ret['stderr']:
-            log.log(lvl, 'stderr: {0}'.format(log_callback(ret['stderr'])))
+            log.log(lvl, 'stderr: %s', log_callback(ret['stderr']))
         if ret['retcode']:
-            log.log(lvl, 'retcode: {0}'.format(ret['retcode']))
+            log.log(lvl, 'retcode: %s', ret['retcode'])
 
     if hide_output:
         ret['stdout'] = ret['stderr'] = ''
@@ -2017,7 +2025,7 @@ def retcode(cmd,
                 )
             )
             log.error(log_callback(msg))
-        log.log(lvl, 'output: {0}'.format(log_callback(ret['stdout'])))
+        log.log(lvl, 'output: %s', log_callback(ret['stdout']))
     return ret['retcode']
 
 
@@ -2217,10 +2225,8 @@ def script(source,
             __salt__['file.remove'](path)
         except (SaltInvocationError, CommandExecutionError) as exc:
             log.error(
-                'cmd.script: Unable to clean tempfile \'{0}\': {1}'.format(
-                    path,
-                    exc
-                )
+                'cmd.script: Unable to clean tempfile \'%s\': %s',
+                path, exc, exc_info_on_loglevel=logging.DEBUG
             )
 
     if '__env__' in kwargs:
@@ -2853,7 +2859,7 @@ def shells():
                 else:
                     ret.append(line)
         except OSError:
-            log.error("File '{0}' was not found".format(shells_fn))
+            log.error("File '%s' was not found", shells_fn)
     return ret
 
 
@@ -2980,7 +2986,7 @@ def shell_info(shell, list_modules=False):
         newenv = os.environ
         if ('HOME' not in newenv) and (not salt.utils.platform.is_windows()):
             newenv['HOME'] = os.path.expanduser('~')
-            log.debug('HOME environment set to {0}'.format(newenv['HOME']))
+            log.debug('HOME environment set to %s', newenv['HOME'])
         try:
             proc = salt.utils.timed_subprocess.TimedProc(
                 shell_data,
@@ -3231,7 +3237,7 @@ def powershell(cmd,
     if encode_cmd:
         # Convert the cmd to UTF-16LE without a BOM and base64 encode.
         # Just base64 encoding UTF-8 or including a BOM is not valid.
-        log.debug('Encoding PowerShell command \'{0}\''.format(cmd))
+        log.debug('Encoding PowerShell command \'%s\'', cmd)
         cmd_utf16 = cmd.decode('utf-8').encode('utf-16le')
         cmd = base64.standard_b64encode(cmd_utf16)
         encoded_cmd = True
@@ -3534,7 +3540,7 @@ def powershell_all(cmd,
     if encode_cmd:
         # Convert the cmd to UTF-16LE without a BOM and base64 encode.
         # Just base64 encoding UTF-8 or including a BOM is not valid.
-        log.debug('Encoding PowerShell command \'{0}\''.format(cmd))
+        log.debug('Encoding PowerShell command \'%s\'', cmd)
         cmd_utf16 = cmd.decode('utf-8').encode('utf-16le')
         cmd = base64.standard_b64encode(cmd_utf16)
         encoded_cmd = True

salt/utils/args.py

@@ -269,7 +269,13 @@ def shlex_split(s, **kwargs):
     Only split if variable is a string
     '''
     if isinstance(s, six.string_types):
-        return shlex.split(s, **kwargs)
+        # On PY2, shlex.split will fail with unicode types if there are
+        # non-ascii characters in the string. So, we need to make sure we
+        # invoke it with a str type, and then decode the resulting string back
+        # to unicode to return it.
+        return salt.utils.data.decode(
+            shlex.split(salt.utils.stringutils.to_str(s), **kwargs)
+        )
     else:
         return s
 

tests/integration/files/file/base/random_bytes

@@ -0,0 +1 @@
+ｩ維

tests/integration/modules/test_cmdmod.py

@@ -121,7 +121,7 @@ class CMDModuleTest(ModuleCase):
         '''
         self.assertEqual(self.run_function('cmd.run',
                 ['bad_command --foo']).rstrip(),
-                'ERROR: This shell command is not permitted: "bad_command --foo"')
+                'ERROR: The shell command "bad_command --foo" is not permitted')
 
     def test_script(self):
         '''

tests/unit/modules/test_cmdmod.py

@@ -10,14 +10,17 @@ import sys
 import tempfile
 
 # Import Salt Libs
+import salt.utils.files
 import salt.utils.platform
 import salt.modules.cmdmod as cmdmod
 from salt.exceptions import CommandExecutionError
 from salt.log import LOG_LEVELS
+from salt.ext.six.moves import builtins  # pylint: disable=import-error
 
 # Import Salt Testing Libs
 from tests.support.mixins import LoaderModuleMockMixin
 from tests.support.unit import TestCase, skipIf
+from tests.support.paths import FILES
 from tests.support.mock import (
     mock_open,
     Mock,
@@ -33,6 +36,39 @@ MOCK_SHELL_FILE = '# List of acceptable shells\n' \
                   '/bin/bash\n'
 
 
+class MockTimedProc(object):
+    '''
+    Class used as a stand-in for salt.utils.timed_subprocess.TimedProc
+    '''
+    class _Process(object):
+        '''
+        Used to provide a dummy "process" attribute
+        '''
+        def __init__(self, returncode=0, pid=12345):
+            self.returncode = returncode
+            self.pid = pid
+
+    def __init__(self, stdout=None, stderr=None, returncode=0, pid=12345):
+        if stdout is not None and not isinstance(stdout, bytes):
+            raise TypeError('Must pass stdout to MockTimedProc as bytes')
+        if stderr is not None and not isinstance(stderr, bytes):
+            raise TypeError('Must pass stderr to MockTimedProc as bytes')
+        self._stdout = stdout
+        self._stderr = stderr
+        self.process = self._Process(returncode=returncode, pid=pid)
+
+    def run(self):
+        pass
+
+    @property
+    def stdout(self):
+        return self._stdout
+
+    @property
+    def stderr(self):
+        return self._stderr
+
+
 @skipIf(NO_MOCK, NO_MOCK_REASON)
 class CMDMODTestCase(TestCase, LoaderModuleMockMixin):
     '''
@@ -303,3 +339,70 @@ class CMDMODTestCase(TestCase, LoaderModuleMockMixin):
             pass
         else:
             raise RuntimeError
+
+    def test_run_all_binary_replace(self):
+        '''
+        Test for failed decoding of binary data, for instance when doing
+        something silly like using dd to read from /dev/urandom and write to
+        /dev/stdout.
+        '''
+        # Since we're using unicode_literals, read the random bytes from a file
+        rand_bytes_file = os.path.join(FILES, 'file', 'base', 'random_bytes')
+        with salt.utils.files.fopen(rand_bytes_file, 'rb') as fp_:
+            stdout_bytes = fp_.read()
+
+        # stdout with the non-decodable bits replaced with the unicode
+        # replacement character U+FFFD.
+        stdout_unicode = '\ufffd\x1b\ufffd\ufffd\n'
+        stderr_bytes = b'1+0 records in\n1+0 records out\n' \
+                       b'4 bytes copied, 9.1522e-05 s, 43.7 kB/s\n'
+        stderr_unicode = stderr_bytes.decode()
+
+        proc = MagicMock(
+            return_value=MockTimedProc(
+                stdout=stdout_bytes,
+                stderr=stderr_bytes
+            )
+        )
+        with patch('salt.utils.timed_subprocess.TimedProc', proc):
+            ret = cmdmod.run_all(
+                'dd if=/dev/urandom of=/dev/stdout bs=4 count=1',
+                rstrip=False)
+
+        self.assertEqual(ret['stdout'], stdout_unicode)
+        self.assertEqual(ret['stderr'], stderr_unicode)
+
+    def test_run_all_none(self):
+        '''
+        Tests cases when proc.stdout or proc.stderr are None. These should be
+        caught and replaced with empty strings.
+        '''
+        proc = MagicMock(return_value=MockTimedProc(stdout=None, stderr=None))
+        with patch('salt.utils.timed_subprocess.TimedProc', proc):
+            ret = cmdmod.run_all('some command', rstrip=False)
+
+        self.assertEqual(ret['stdout'], '')
+        self.assertEqual(ret['stderr'], '')
+
+    def test_run_all_unicode(self):
+        '''
+        Ensure that unicode stdout and stderr are decoded properly
+        '''
+        stdout_unicode = 'Here is some unicode: спам'
+        stderr_unicode = 'Here is some unicode: яйца'
+        stdout_bytes = stdout_unicode.encode('utf-8')
+        stderr_bytes = stderr_unicode.encode('utf-8')
+
+        proc = MagicMock(
+            return_value=MockTimedProc(
+                stdout=stdout_bytes,
+                stderr=stderr_bytes
+            )
+        )
+
+        with patch('salt.utils.timed_subprocess.TimedProc', proc), \
+                patch.object(builtins, '__salt_system_encoding__', 'utf-8'):
+            ret = cmdmod.run_all('some command', rstrip=False)
+
+        self.assertEqual(ret['stdout'], stdout_unicode)
+        self.assertEqual(ret['stderr'], stderr_unicode)