diff --git a/changelogs/fragments/74783-run-command-thread-safety.yml b/changelogs/fragments/74783-run-command-thread-safety.yml new file mode 100644 index 00000000000..d89de441034 --- /dev/null +++ b/changelogs/fragments/74783-run-command-thread-safety.yml @@ -0,0 +1,5 @@ +bugfixes: + - > + ``AnsibleModule.run_command`` - Address thread safety issues, concerning mutating the environment, + current working directory, and umask. + (https://github.com/ansible/ansible/issues/74783) diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index 11c409eb683..00bd927d3f5 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -1863,7 +1863,7 @@ class AnsibleModule(object): :kw prompt_regex: Regex string (not a compiled regex) which can be used to detect prompts in the stdout which would otherwise cause the execution to hang (especially if no input data is specified) - :kw environ_update: dictionary to *update* os.environ with + :kw environ_update: dictionary to *update* environ variables with :kw umask: Umask to be used when running the command. Default None :kw encoding: Since we return native strings, on python3 we need to know the encoding to use to transform from bytes to text. If you @@ -1958,23 +1958,16 @@ class AnsibleModule(object): msg = None st_in = None - # Manipulate the environ we'll send to the new process - old_env_vals = {} + env = os.environ.copy() # We can set this from both an attribute and per call - for key, val in self.run_command_environ_update.items(): - old_env_vals[key] = os.environ.get(key, None) - os.environ[key] = val - if environ_update: - for key, val in environ_update.items(): - old_env_vals[key] = os.environ.get(key, None) - os.environ[key] = val + env.update(self.run_command_environ_update or {}) + env.update(environ_update or {}) if path_prefix: - path = os.environ.get('PATH', '') - old_env_vals['PATH'] = path + path = env.get('PATH', '') if path: - os.environ['PATH'] = "%s:%s" % (path_prefix, path) + env['PATH'] = "%s:%s" % (path_prefix, path) else: - os.environ['PATH'] = path_prefix + env['PATH'] = path_prefix # If using test-module.py and explode, the remote lib path will resemble: # /tmp/test_module_scratch/debug_dir/ansible/module_utils/basic.py @@ -1982,18 +1975,22 @@ class AnsibleModule(object): # /tmp/ansible_vmweLQ/ansible_modlib.zip/ansible/module_utils/basic.py # Clean out python paths set by ansiballz - if 'PYTHONPATH' in os.environ: - pypaths = os.environ['PYTHONPATH'].split(':') - pypaths = [x for x in pypaths - if not x.endswith('/ansible_modlib.zip') and + if 'PYTHONPATH' in env: + pypaths = [x for x in env['PYTHONPATH'].split(':') + if x and + not x.endswith('/ansible_modlib.zip') and not x.endswith('/debug_dir')] - os.environ['PYTHONPATH'] = ':'.join(pypaths) - if not os.environ['PYTHONPATH']: - del os.environ['PYTHONPATH'] + if pypaths and any(pypaths): + env['PYTHONPATH'] = ':'.join(pypaths) if data: st_in = subprocess.PIPE + def preexec(): + self._restore_signal_handlers() + if umask: + os.umask(umask) + kwargs = dict( executable=executable, shell=shell, @@ -2001,33 +1998,22 @@ class AnsibleModule(object): stdin=st_in, stdout=subprocess.PIPE, stderr=subprocess.PIPE, - preexec_fn=self._restore_signal_handlers, + preexec_fn=preexec, + env=env, ) if PY3 and pass_fds: kwargs["pass_fds"] = pass_fds elif PY2 and pass_fds: kwargs['close_fds'] = False - # store the pwd - prev_dir = os.getcwd() - # make sure we're in the right working directory if cwd: + cwd = to_bytes(os.path.abspath(os.path.expanduser(cwd)), errors='surrogate_or_strict') if os.path.isdir(cwd): - cwd = to_bytes(os.path.abspath(os.path.expanduser(cwd)), errors='surrogate_or_strict') kwargs['cwd'] = cwd - try: - os.chdir(cwd) - except (OSError, IOError) as e: - self.fail_json(rc=e.errno, msg="Could not chdir to %s, %s" % (cwd, to_native(e)), - exception=traceback.format_exc()) elif not ignore_invalid_cwd: self.fail_json(msg="Provided cwd is not a valid directory: %s" % cwd) - old_umask = None - if umask: - old_umask = os.umask(umask) - try: if self._debug: self.log('Executing: ' + self._clean_args(args)) @@ -2103,23 +2089,10 @@ class AnsibleModule(object): self.log("Error Executing CMD:%s Exception:%s" % (self._clean_args(args), to_native(traceback.format_exc()))) self.fail_json(rc=257, stdout=b'', stderr=b'', msg=to_native(e), exception=traceback.format_exc(), cmd=self._clean_args(args)) - # Restore env settings - for key, val in old_env_vals.items(): - if val is None: - del os.environ[key] - else: - os.environ[key] = val - - if old_umask: - os.umask(old_umask) - if rc != 0 and check_rc: msg = heuristic_log_sanitize(stderr.rstrip(), self.no_log_values) self.fail_json(cmd=self._clean_args(args), rc=rc, stdout=stdout, stderr=stderr, msg=msg) - # reset the pwd - os.chdir(prev_dir) - if encoding is not None: return (rc, to_native(stdout, encoding=encoding, errors=errors), to_native(stderr, encoding=encoding, errors=errors)) diff --git a/test/units/module_utils/basic/test_run_command.py b/test/units/module_utils/basic/test_run_command.py index 25f1c48ed99..04211e2df28 100644 --- a/test/units/module_utils/basic/test_run_command.py +++ b/test/units/module_utils/basic/test_run_command.py @@ -163,27 +163,22 @@ class TestRunCommandArgs: class TestRunCommandCwd: @pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) def test_cwd(self, mocker, rc_am): - rc_am._os.getcwd.return_value = '/old' rc_am.run_command('/bin/ls', cwd='/new') - assert rc_am._os.chdir.mock_calls == [mocker.call(b'/new'), mocker.call('/old'), ] + assert rc_am._subprocess.Popen.mock_calls[0][2]['cwd'] == b'/new' @pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) def test_cwd_relative_path(self, mocker, rc_am): - rc_am._os.getcwd.return_value = '/old' rc_am.run_command('/bin/ls', cwd='sub-dir') - assert rc_am._os.chdir.mock_calls == [mocker.call(b'/old/sub-dir'), mocker.call('/old'), ] + assert rc_am._subprocess.Popen.mock_calls[0][2]['cwd'] == b'/home/foo/sub-dir' @pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) def test_cwd_not_a_dir(self, mocker, rc_am): - rc_am._os.getcwd.return_value = '/old' - rc_am._os.path.isdir.side_effect = lambda d: d != '/not-a-dir' rc_am.run_command('/bin/ls', cwd='/not-a-dir') - assert rc_am._os.chdir.mock_calls == [mocker.call('/old'), ] + assert rc_am._subprocess.Popen.mock_calls[0][2]['cwd'] == b'/not-a-dir' @pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) def test_cwd_not_a_dir_noignore(self, rc_am): - rc_am._os.getcwd.return_value = '/old' - rc_am._os.path.isdir.side_effect = lambda d: d != '/not-a-dir' + rc_am._os.path.isdir.side_effect = lambda d: d != b'/not-a-dir' with pytest.raises(SystemExit): rc_am.run_command('/bin/ls', cwd='/not-a-dir', ignore_invalid_cwd=False) assert rc_am.fail_json.called