From 7743e57ff3a6a706d41d063b2845fd3df04f7466 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Thu, 28 Feb 2019 06:51:14 +0000 Subject: [PATCH] issue #554: track and remove multiple make_tmp_path() calls. --- ansible_mitogen/connection.py | 31 ------------- ansible_mitogen/mixins.py | 45 ++++++++++++++----- docs/changelog.rst | 5 +++ tests/ansible/integration/action/all.yml | 1 + .../action/make_tmp_path__double.yml | 20 +++++++++ 5 files changed, 60 insertions(+), 42 deletions(-) create mode 100644 tests/ansible/integration/action/make_tmp_path__double.yml diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index fe07f44f..bd4330ff 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -33,7 +33,6 @@ import errno import logging import os import pprint -import random import stat import sys import time @@ -703,35 +702,6 @@ class Connection(ansible.plugins.connection.ConnectionBase): self._connect() return self.init_child_result['good_temp_dir'] - def _generate_tmp_path(self): - return os.path.join( - self.get_good_temp_dir(), - 'ansible_mitogen_action_%016x' % ( - random.getrandbits(8*8), - ) - ) - - def _make_tmp_path(self): - assert getattr(self._shell, 'tmpdir', None) is None - self._shell.tmpdir = self._generate_tmp_path() - LOG.debug('Temporary directory: %r', self._shell.tmpdir) - self.get_chain().call_no_reply(os.mkdir, self._shell.tmpdir) - return self._shell.tmpdir - - def _reset_tmp_path(self): - """ - Called by _mitogen_reset(); ask the remote context to delete any - temporary directory created for the action. CallChain is not used here - to ensure exception is logged by the context on failure, since the - CallChain itself is about to be destructed. - """ - if getattr(self._shell, 'tmpdir', None) is not None: - self.context.call_no_reply( - ansible_mitogen.target.prune_tree, - self._shell.tmpdir, - ) - self._shell.tmpdir = None - def _connect(self): """ Establish a connection to the master process's UNIX listener socket, @@ -763,7 +733,6 @@ class Connection(ansible.plugins.connection.ConnectionBase): if not self.context: return - self._reset_tmp_path() self.chain.reset() self.parent.call_service( service_name='ansible_mitogen.services.ContextService', diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py index 5f51cc6f..6aa020fb 100644 --- a/ansible_mitogen/mixins.py +++ b/ansible_mitogen/mixins.py @@ -30,6 +30,7 @@ from __future__ import absolute_import import logging import os import pwd +import random import traceback try: @@ -173,26 +174,48 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): """ assert False, "_is_pipelining_enabled() should never be called." + def _generate_tmp_path(self): + return os.path.join( + self._connection.get_good_temp_dir(), + 'ansible_mitogen_action_%016x' % ( + random.getrandbits(8*8), + ) + ) + + def _generate_tmp_path(self): + return os.path.join( + self._connection.get_good_temp_dir(), + 'ansible_mitogen_action_%016x' % ( + random.getrandbits(8*8), + ) + ) + def _make_tmp_path(self, remote_user=None): """ - Return the directory created by the Connection instance during - connection. + Create a temporary subdirectory as a child of the temporary directory + managed by the remote interpreter. """ LOG.debug('_make_tmp_path(remote_user=%r)', remote_user) - return self._connection._make_tmp_path() + path = self._generate_tmp_path() + LOG.debug('Temporary directory: %r', path) + self._connection.get_chain().call_no_reply(os.mkdir, path) + self._connection._shell.tmpdir = path + return path def _remove_tmp_path(self, tmp_path): """ - Stub out the base implementation's invocation of rm -rf, replacing it - with nothing, as the persistent interpreter automatically cleans up - after itself without introducing roundtrips. + Replace the base implementation's invocation of rm -rf, replacing it + with a pipelined call to :func:`ansible_mitogen.target.prune_tree`. """ - # The actual removal is pipelined by Connection.close(). LOG.debug('_remove_tmp_path(%r)', tmp_path) - # Upstream _remove_tmp_path resets shell.tmpdir here, however - # connection.py uses that as the sole location of the temporary - # directory, if one exists. - # self._connection._shell.tmpdir = None + if tmp_path is None and ansible.__version__ > '2.6': + tmp_path = self._connection._shell.tmpdir # 06f73ad578d + if tmp_path is not None: + self._connection.get_chain().call_no_reply( + ansible_mitogen.target.prune_tree, + tmp_path, + ) + self._connection._shell.tmpdir = None def _transfer_data(self, remote_path, data): """ diff --git a/docs/changelog.rst b/docs/changelog.rst index bf6d9088..165f8a45 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -41,6 +41,10 @@ Fixes TTY-related `ioctl()` calls on Windows Subsystem for Linux 2016 Anniversary Update. +* `#554 `_: third party Ansible + action plug-ins that invoked :func:`_make_tmp_path` repeatedly could trigger + an assertion failure. + * `ffae0355 `_: needless information was removed from the documentation and installation procedure. @@ -77,6 +81,7 @@ bug reports, testing, features and fixes in this release contributed by `Matt Layman `_, `Percy Grunwald `_, `Petr Enkov `_, +`Tony Finch `_, `@elbunda `_, and `@zyphermonkey `_. diff --git a/tests/ansible/integration/action/all.yml b/tests/ansible/integration/action/all.yml index 461c742b..c43d5cc7 100644 --- a/tests/ansible/integration/action/all.yml +++ b/tests/ansible/integration/action/all.yml @@ -2,6 +2,7 @@ - include: fixup_perms2__copy.yml - include: low_level_execute_command.yml - include: make_tmp_path.yml +- include: make_tmp_path__double.yml - include: remote_expand_user.yml - include: remote_file_exists.yml - include: remove_tmp_path.yml diff --git a/tests/ansible/integration/action/make_tmp_path__double.yml b/tests/ansible/integration/action/make_tmp_path__double.yml new file mode 100644 index 00000000..8b24d322 --- /dev/null +++ b/tests/ansible/integration/action/make_tmp_path__double.yml @@ -0,0 +1,20 @@ +# issue #554: double calls to make_tmp_path() fail with assertion error. Ensure +# they succeed and are cleaned up correctly. + +- hosts: target + tasks: + - mitogen_action_script: + script: | + result['t1'] = self._make_tmp_path() + result['t2'] = self._make_tmp_path() + assert result['t1'] != result['t2'] + assert self._remote_file_exists(result['t1']) + assert self._remote_file_exists(result['t2']) + self._remove_tmp_path(result['t1']) + self._remove_tmp_path(result['t2']) + register: out + + - mitogen_action_script: + script: | + assert not self._remote_file_exists("{{ out.t1 }}") + assert not self._remote_file_exists("{{ out.t2 }}")