From 238cccf1668579a12c3b80ef9baa27f432accb65 Mon Sep 17 00:00:00 2001 From: Alexander Stock Date: Fri, 26 Aug 2016 16:41:17 +0200 Subject: [PATCH] Fix "Text file busy" exception in atomic_move (#9526) (#17204) tempfile.NamedTemporaryFile keeps a file handle causing os.rename() to fail with windows based vboxfs: [Errno 26] Text file busy. Changed NamedTemporaryFile to mkstemp() and added a finally block to unlink the temp file in each and every case. --- lib/ansible/module_utils/basic.py | 86 ++++++++++++++------------- test/units/module_utils/test_basic.py | 26 ++++---- 2 files changed, 58 insertions(+), 54 deletions(-) diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index 883e488a3f2..6f85f5af1e7 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -1944,55 +1944,59 @@ class AnsibleModule(object): dest_dir = os.path.dirname(dest) dest_file = os.path.basename(dest) try: - tmp_dest = tempfile.NamedTemporaryFile( + tmp_dest_fd, tmp_dest_name = tempfile.mkstemp( prefix=".ansible_tmp", dir=dest_dir, suffix=dest_file) except (OSError, IOError): e = get_exception() self.fail_json(msg='The destination directory (%s) is not writable by the current user. Error was: %s' % (dest_dir, e)) - try: # leaves tmp file behind when sudo and not root - if switched_user and os.getuid() != 0: - # cleanup will happen by 'rm' of tempdir - # copy2 will preserve some metadata - shutil.copy2(src, tmp_dest.name) - else: - shutil.move(src, tmp_dest.name) - if self.selinux_enabled(): - self.set_context_if_different( - tmp_dest.name, context, False) + try: try: - tmp_stat = os.stat(tmp_dest.name) - if dest_stat and (tmp_stat.st_uid != dest_stat.st_uid or tmp_stat.st_gid != dest_stat.st_gid): - os.chown(tmp_dest.name, dest_stat.st_uid, dest_stat.st_gid) - except OSError: - e = get_exception() - if e.errno != errno.EPERM: - raise - os.rename(tmp_dest.name, dest) - except (shutil.Error, OSError, IOError): - e = get_exception() - # sadly there are some situations where we cannot ensure atomicity, but only if - # the user insists and we get the appropriate error we update the file unsafely - if unsafe_writes and e.errno == errno.EBUSY: - #TODO: issue warning that this is an unsafe operation, but doing it cause user insists + # close tmp file handle before file operations to prevent text file busy errors on vboxfs synced folders (windows host) + os.close(tmp_dest_fd) + # leaves tmp file behind when sudo and not root + if switched_user and os.getuid() != 0: + # cleanup will happen by 'rm' of tempdir + # copy2 will preserve some metadata + shutil.copy2(src, tmp_dest_name) + else: + shutil.move(src, tmp_dest_name) + if self.selinux_enabled(): + self.set_context_if_different( + tmp_dest_name, context, False) try: - try: - out_dest = open(dest, 'wb') - in_src = open(src, 'rb') - shutil.copyfileobj(in_src, out_dest) - finally: # assuring closed files in 2.4 compatible way - if out_dest: - out_dest.close() - if in_src: - in_src.close() - except (shutil.Error, OSError, IOError): + tmp_stat = os.stat(tmp_dest_name) + if dest_stat and (tmp_stat.st_uid != dest_stat.st_uid or tmp_stat.st_gid != dest_stat.st_gid): + os.chown(tmp_dest_name, dest_stat.st_uid, dest_stat.st_gid) + except OSError: e = get_exception() - self.fail_json(msg='Could not write data to file (%s) from (%s): %s' % (dest, src, e)) - - else: - self.fail_json(msg='Could not replace file: %s to %s: %s' % (src, dest, e)) - - self.cleanup(tmp_dest.name) + if e.errno != errno.EPERM: + raise + os.rename(tmp_dest_name, dest) + except (shutil.Error, OSError, IOError): + e = get_exception() + # sadly there are some situations where we cannot ensure atomicity, but only if + # the user insists and we get the appropriate error we update the file unsafely + if unsafe_writes and e.errno == errno.EBUSY: + #TODO: issue warning that this is an unsafe operation, but doing it cause user insists + try: + try: + out_dest = open(dest, 'wb') + in_src = open(src, 'rb') + shutil.copyfileobj(in_src, out_dest) + finally: # assuring closed files in 2.4 compatible way + if out_dest: + out_dest.close() + if in_src: + in_src.close() + except (shutil.Error, OSError, IOError): + e = get_exception() + self.fail_json(msg='Could not write data to file (%s) from (%s): %s' % (dest, src, e)) + + else: + self.fail_json(msg='Could not replace file: %s to %s: %s' % (src, dest, e)) + finally: + self.cleanup(tmp_dest_name) if creating: # make sure the file has the correct permissions diff --git a/test/units/module_utils/test_basic.py b/test/units/module_utils/test_basic.py index a68352aea67..7f882622610 100644 --- a/test/units/module_utils/test_basic.py +++ b/test/units/module_utils/test_basic.py @@ -741,7 +741,7 @@ class TestModuleUtilsBasic(ModuleTestCase): with patch('os.lchown', side_effect=OSError) as m: self.assertRaises(SystemExit, am.set_group_if_different, '/path/to/file', 'root', False) - @patch('tempfile.NamedTemporaryFile') + @patch('tempfile.mkstemp') @patch('os.umask') @patch('shutil.copyfileobj') @patch('shutil.move') @@ -755,8 +755,10 @@ class TestModuleUtilsBasic(ModuleTestCase): @patch('os.chmod') @patch('os.stat') @patch('os.path.exists') + @patch('os.close') def test_module_utils_basic_ansible_module_atomic_move( self, + _os_close, _os_path_exists, _os_stat, _os_chmod, @@ -770,7 +772,7 @@ class TestModuleUtilsBasic(ModuleTestCase): _shutil_move, _shutil_copyfileobj, _os_umask, - _tempfile_NamedTemporaryFile, + _tempfile_mkstemp, ): from ansible.module_utils import basic @@ -903,20 +905,21 @@ class TestModuleUtilsBasic(ModuleTestCase): self.assertRaises(SystemExit, am.atomic_move, '/path/to/src', '/path/to/dest') # next we test with EPERM so it continues to the alternate code for moving - # test with NamedTemporaryFile raising an error first + # test with mkstemp raising an error first _os_path_exists.side_effect = [False, False] _os_getlogin.return_value = 'root' _os_getuid.return_value = 0 + _os_close.return_value = None _pwd_getpwuid.return_value = ('root', '', 0, 0, '', '', '') _os_umask.side_effect = [18, 0] _os_rename.side_effect = [OSError(errno.EPERM, 'failing with EPERM'), None] - _tempfile_NamedTemporaryFile.return_value = None - _tempfile_NamedTemporaryFile.side_effect = OSError() + _tempfile_mkstemp.return_value = None + _tempfile_mkstemp.side_effect = OSError() am.selinux_enabled.return_value = False self.assertRaises(SystemExit, am.atomic_move, '/path/to/src', '/path/to/dest') # then test with it creating a temp file - _os_path_exists.side_effect = [False, False] + _os_path_exists.side_effect = [False, False, False] _os_getlogin.return_value = 'root' _os_getuid.return_value = 0 _pwd_getpwuid.return_value = ('root', '', 0, 0, '', '', '') @@ -927,23 +930,20 @@ class TestModuleUtilsBasic(ModuleTestCase): mock_stat3 = MagicMock() _os_stat.return_value = [mock_stat1, mock_stat2, mock_stat3] _os_stat.side_effect = None - mock_tempfile = MagicMock() - mock_tempfile.name = '/path/to/tempfile' - _tempfile_NamedTemporaryFile.return_value = mock_tempfile - _tempfile_NamedTemporaryFile.side_effect = None + _tempfile_mkstemp.return_value = (None, '/path/to/tempfile') + _tempfile_mkstemp.side_effect = None am.selinux_enabled.return_value = False # FIXME: we don't assert anything here yet am.atomic_move('/path/to/src', '/path/to/dest') # same as above, but with selinux enabled - _os_path_exists.side_effect = [False, False] + _os_path_exists.side_effect = [False, False, False] _os_getlogin.return_value = 'root' _os_getuid.return_value = 0 _pwd_getpwuid.return_value = ('root', '', 0, 0, '', '', '') _os_umask.side_effect = [18, 0] _os_rename.side_effect = [OSError(errno.EPERM, 'failing with EPERM'), None] - mock_tempfile = MagicMock() - _tempfile_NamedTemporaryFile.return_value = mock_tempfile + _tempfile_mkstemp.return_value = (None, None) mock_context = MagicMock() am.selinux_default_context.return_value = mock_context am.selinux_enabled.return_value = True