diff --git a/releasenotes/notes/fix-hanging-on-pipe-7c4b5f9c81623b524.yaml b/releasenotes/notes/fix-hanging-on-pipe-7c4b5f9c81623b524.yaml new file mode 100644 index 0000000..b5ca084 --- /dev/null +++ b/releasenotes/notes/fix-hanging-on-pipe-7c4b5f9c81623b524.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Properly closes standard I/O streams to prevent shell-piped processes from + hanging infinitely on a dead pipe. diff --git a/virtualbmc/cmd/vbmc.py b/virtualbmc/cmd/vbmc.py index 2cf6959..6eb7cb8 100644 --- a/virtualbmc/cmd/vbmc.py +++ b/virtualbmc/cmd/vbmc.py @@ -12,7 +12,6 @@ import json import logging -import os import sys import time @@ -28,6 +27,7 @@ from virtualbmc.cmd import vbmcd from virtualbmc import config as vbmc_config from virtualbmc.exception import VirtualBMCError from virtualbmc import log +from virtualbmc import utils CONF = vbmc_config.get_config() @@ -110,9 +110,10 @@ class ZmqClient(object): "future releases!") # attempt to start and daemonize the server - if os.fork() == 0: - # this will also fork and detach properly - vbmcd.main([]) + with utils.detach_process() as pid: + if pid == 0: + # NOTE(etingof): this child will never return + vbmcd.main(['--foreground']) # TODO(etingof): perform some more retries time.sleep(CONF['default']['server_spawn_wait'] / 1000.) diff --git a/virtualbmc/cmd/vbmcd.py b/virtualbmc/cmd/vbmcd.py index 4e707ba..e757c1c 100644 --- a/virtualbmc/cmd/vbmcd.py +++ b/virtualbmc/cmd/vbmcd.py @@ -85,9 +85,14 @@ def main(argv=sys.argv[1:]): if args.foreground: return wrap_with_pidfile(control.application, os.getpid()) + else: with utils.detach_process() as pid: + if pid > 0: + return 0 + return wrap_with_pidfile(control.application, pid) + if __name__ == '__main__': sys.exit(main()) diff --git a/virtualbmc/tests/unit/cmd/test_vbmcd.py b/virtualbmc/tests/unit/cmd/test_vbmcd.py index 82429bd..7b066f3 100644 --- a/virtualbmc/tests/unit/cmd/test_vbmcd.py +++ b/virtualbmc/tests/unit/cmd/test_vbmcd.py @@ -30,22 +30,23 @@ class VBMCDTestCase(base.TestCase): @mock.patch.object(os, 'kill') @mock.patch.object(os, 'unlink') def test_main_foreground(self, mock_unlink, mock_kill, mock_open): - with mock.patch.object(control, 'application') as ml: + with mock.patch.object(control, 'application') as mock_ml: mock_kill.side_effect = OSError() vbmcd.main(['--foreground']) mock_kill.assert_called_once() - ml.assert_called_once() + mock_ml.assert_called_once() mock_unlink.assert_called_once() @mock.patch.object(builtins, 'open') @mock.patch.object(os, 'kill') @mock.patch.object(os, 'unlink') def test_main_background(self, mock_unlink, mock_kill, mock_open): - with mock.patch.object(utils, 'detach_process') as dp, \ - mock.patch.object(control, 'application') as ml: - mock_kill.side_effect = OSError() - vbmcd.main([]) - mock_kill.assert_called_once() - dp.assert_called_once() - ml.assert_called_once() - mock_unlink.assert_called_once() + with mock.patch.object(utils, 'detach_process') as mock_dp: + with mock.patch.object(control, 'application') as mock_ml: + mock_kill.side_effect = OSError() + mock_dp.return_value.__enter__.return_value = 0 + vbmcd.main([]) + mock_kill.assert_called_once() + mock_dp.assert_called_once() + mock_ml.assert_called_once() + mock_unlink.assert_called_once() diff --git a/virtualbmc/tests/unit/test_utils.py b/virtualbmc/tests/unit/test_utils.py index 66384b4..c173026 100644 --- a/virtualbmc/tests/unit/test_utils.py +++ b/virtualbmc/tests/unit/test_utils.py @@ -114,60 +114,49 @@ class LibvirtUtilsTestCase(base.TestCase): self._test_libvirt_open_sasl(readonly=True) -@mock.patch.object(os, 'getpid') -@mock.patch.object(os, 'umask') -@mock.patch.object(os, 'chdir') -@mock.patch.object(os, 'setsid') -@mock.patch.object(os, '_exit') -@mock.patch.object(os, 'fork') +@mock.patch.object(utils, 'os') class DetachProcessUtilsTestCase(base.TestCase): - def test_detach_process(self, mock_fork, mock__exit, mock_setsid, - mock_chdir, mock_umask, mock_getpid): + def test_detach_process(self, mock_os): # 2nd value > 0 so _exit get called and we can assert that we've # killed the parent's process - mock_fork.side_effect = (0, 999) + mock_os.fork.side_effect = (0, 999) + mock_os.devnull = os.devnull - mock_getpid.return_value = 123 with utils.detach_process() as pid: - self.assertEqual(123, pid) + self.assertEqual(0, pid) # assert fork() has been called twice expected_fork_calls = [mock.call()] * 2 - self.assertEqual(expected_fork_calls, mock_fork.call_args_list) + self.assertEqual(expected_fork_calls, mock_os.fork.call_args_list) - mock_setsid.assert_called_once_with() - mock_chdir.assert_called_once_with('/') - mock_umask.assert_called_once_with(0) - mock__exit.assert_called_once_with(0) - mock_getpid.assert_called_once_with() + mock_os.setsid.assert_called_once_with() + mock_os.chdir.assert_called_once_with('/') + mock_os.umask.assert_called_once_with(0) + mock_os._exit.assert_called_once_with(0) - def test_detach_process_fork_fail(self, mock_fork, mock__exit, mock_setsid, - mock_chdir, mock_umask, mock_getpid): + def test_detach_process_fork_fail(self, mock_os): error_msg = 'Kare-a-tay!' - mock_fork.side_effect = OSError(error_msg) + mock_os.fork.side_effect = OSError(error_msg) with self.assertRaisesRegex(exception.DetachProcessError, error_msg): with utils.detach_process(): pass - mock_fork.assert_called_once_with() - self.assertFalse(mock_setsid.called) - self.assertFalse(mock_chdir.called) - self.assertFalse(mock_umask.called) - self.assertFalse(mock__exit.called) - self.assertFalse(mock_getpid.called) + mock_os.fork.assert_called_once_with() + self.assertFalse(mock_os.setsid.called) + self.assertFalse(mock_os.chdir.called) + self.assertFalse(mock_os.umask.called) + self.assertFalse(mock_os._exit.called) - def test_detach_process_chdir_fail(self, mock_fork, mock__exit, - mock_setsid, mock_chdir, mock_umask, - mock_getpid): + def test_detach_process_chdir_fail(self, mock_os): # 2nd value > 0 so _exit get called and we can assert that we've # killed the parent's process - mock_fork.side_effect = (0, 999) + mock_os.fork.side_effect = (0, 999) error_msg = 'Fish paste!' - mock_chdir.side_effect = Exception(error_msg) + mock_os.chdir.side_effect = Exception(error_msg) with self.assertRaisesRegex(exception.DetachProcessError, error_msg): with utils.detach_process(): @@ -175,23 +164,20 @@ class DetachProcessUtilsTestCase(base.TestCase): # assert fork() has been called twice expected_fork_calls = [mock.call()] * 2 - self.assertEqual(expected_fork_calls, mock_fork.call_args_list) + self.assertEqual(expected_fork_calls, mock_os.fork.call_args_list) - mock_setsid.assert_called_once_with() - mock_chdir.assert_called_once_with('/') - mock__exit.assert_called_once_with(0) - self.assertFalse(mock_umask.called) - self.assertFalse(mock_getpid.called) + mock_os.setsid.assert_called_once_with() + mock_os.chdir.assert_called_once_with('/') + mock_os._exit.assert_called_once_with(0) + self.assertFalse(mock_os.umask.called) - def test_detach_process_umask_fail(self, mock_fork, mock__exit, - mock_setsid, mock_chdir, mock_umask, - mock_getpid): + def test_detach_process_umask_fail(self, mock_os): # 2nd value > 0 so _exit get called and we can assert that we've # killed the parent's process - mock_fork.side_effect = (0, 999) + mock_os.fork.side_effect = (0, 999) error_msg = 'Barnacles!' - mock_umask.side_effect = Exception(error_msg) + mock_os.umask.side_effect = Exception(error_msg) with self.assertRaisesRegex(exception.DetachProcessError, error_msg): with utils.detach_process(): @@ -199,10 +185,9 @@ class DetachProcessUtilsTestCase(base.TestCase): # assert fork() has been called twice expected_fork_calls = [mock.call()] * 2 - self.assertEqual(expected_fork_calls, mock_fork.call_args_list) + self.assertEqual(expected_fork_calls, mock_os.fork.call_args_list) - mock_setsid.assert_called_once_with() - mock_chdir.assert_called_once_with('/') - mock__exit.assert_called_once_with(0) - mock_umask.assert_called_once_with(0) - self.assertFalse(mock_getpid.called) + mock_os.setsid.assert_called_once_with() + mock_os.chdir.assert_called_once_with('/') + mock_os._exit.assert_called_once_with(0) + mock_os.umask.assert_called_once_with(0) diff --git a/virtualbmc/utils.py b/virtualbmc/utils.py index 401eb39..f0de2d7 100644 --- a/virtualbmc/utils.py +++ b/virtualbmc/utils.py @@ -10,8 +10,10 @@ # License for the specific language governing permissions and limitations # under the License. -import libvirt import os +import sys + +import libvirt from virtualbmc import exception @@ -97,12 +99,14 @@ def mask_dict_password(dictionary, secret='***'): class detach_process(object): """Detach the process from its parent and session.""" - def _fork(self): + def _fork(self, parent_exits): try: - ret = os.fork() - if ret > 0: - # Exit the parent process + pid = os.fork() + if pid > 0 and parent_exits: os._exit(0) + + return pid + except OSError as e: raise exception.DetachProcessError(error=e) @@ -133,13 +137,29 @@ class detach_process(object): raise exception.DetachProcessError(error=error) def __enter__(self): - self._fork() + pid = self._fork(parent_exits=False) + if pid > 0: + return pid + os.setsid() - self._fork() + + self._fork(parent_exits=True) + self._change_root_directory() self._change_file_creation_mask() - return os.getpid() + sys.stdout.flush() + sys.stderr.flush() + + si = open(os.devnull, 'r') + so = open(os.devnull, 'a+') + se = open(os.devnull, 'a+') + + os.dup2(si.fileno(), sys.stdin.fileno()) + os.dup2(so.fileno(), sys.stdout.fileno()) + os.dup2(se.fileno(), sys.stderr.fileno()) + + return pid def __exit__(self, type, value, traceback): pass