cmueller/ansible
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix "Text file busy" exception in atomic_move (#9526) (#17204)

Browse source 
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.
This commit is contained in:
Alexander Stock 2016-08-26 16:41:17 +02:00 committed by Toshio Kuratomi
parent a942758a07
commit 238cccf166
2 changed files with 57 additions and 53 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

84
lib/ansible/module_utils/basic.py
View file

@ -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))
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))
self.cleanup(tmp_dest.name)
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

26
test/units/module_utils/test_basic.py
View file

@ -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