From 6cef37f310b7c129b1a7c9d78a0296e665a802a8 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Thu, 1 Feb 2018 22:24:28 -0600 Subject: [PATCH] 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 | 80 +++++++------- salt/utils/args.py | 8 +- .../integration/files/file/base/random_bytes | 1 + tests/integration/modules/test_cmdmod.py | 2 +- tests/unit/modules/test_cmdmod.py | 103 ++++++++++++++++++ 5 files changed, 155 insertions(+), 39 deletions(-) create mode 100644 tests/integration/files/file/base/random_bytes diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 09e71dc90e..f8041310f0 100644 --- a/salt/modules/cmdmod.py +++ b/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 diff --git a/salt/utils/args.py b/salt/utils/args.py index 29ba3d8211..660810703b 100644 --- a/salt/utils/args.py +++ b/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 diff --git a/tests/integration/files/file/base/random_bytes b/tests/integration/files/file/base/random_bytes new file mode 100644 index 0000000000..bac5364ab8 --- /dev/null +++ b/tests/integration/files/file/base/random_bytes @@ -0,0 +1 @@ + diff --git a/tests/integration/modules/test_cmdmod.py b/tests/integration/modules/test_cmdmod.py index 18f08744ff..ec00a83d80 100644 --- a/tests/integration/modules/test_cmdmod.py +++ b/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): ''' diff --git a/tests/unit/modules/test_cmdmod.py b/tests/unit/modules/test_cmdmod.py index fff5732baa..82a739c24b 100644 --- a/tests/unit/modules/test_cmdmod.py +++ b/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)