Fix hanging on shell-pipe

Properly closes standard I/O streams to prevent shell-piped
processes from hanging infinitely on a dead pipe.

Change-Id: Id9dedac4f778cf37d626f2777d519f73cc4b5f2c
Story: 2007219
Task: 38471

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.

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:
+                with utils.detach_process() as pid:
-                    # this will also fork and detach properly
+                    if pid == 0:
-                    vbmcd.main([])
+                        # NOTE(etingof): this child will never return
+                        vbmcd.main(['--foreground'])
 
                 # TODO(etingof): perform some more retries
                 time.sleep(CONF['default']['server_spawn_wait'] / 1000.)

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())

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, \
+        with mock.patch.object(utils, 'detach_process') as mock_dp:
-                mock.patch.object(control, 'application') as ml:
+            with mock.patch.object(control, 'application') as mock_ml:
-            mock_kill.side_effect = OSError()
+                mock_kill.side_effect = OSError()
-            vbmcd.main([])
+                mock_dp.return_value.__enter__.return_value = 0
-            mock_kill.assert_called_once()
+                vbmcd.main([])
-            dp.assert_called_once()
+                mock_kill.assert_called_once()
-            ml.assert_called_once()
+                mock_dp.assert_called_once()
-            mock_unlink.assert_called_once()
+                mock_ml.assert_called_once()
+                mock_unlink.assert_called_once()

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(utils, 'os')
-@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')
 class DetachProcessUtilsTestCase(base.TestCase):
 
-    def test_detach_process(self, mock_fork, mock__exit, mock_setsid,
+    def test_detach_process(self, mock_os):
-                            mock_chdir, mock_umask, mock_getpid):
 
         # 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_os.setsid.assert_called_once_with()
-        mock_chdir.assert_called_once_with('/')
+        mock_os.chdir.assert_called_once_with('/')
-        mock_umask.assert_called_once_with(0)
+        mock_os.umask.assert_called_once_with(0)
-        mock__exit.assert_called_once_with(0)
+        mock_os._exit.assert_called_once_with(0)
-        mock_getpid.assert_called_once_with()
 
-    def test_detach_process_fork_fail(self, mock_fork, mock__exit, mock_setsid,
+    def test_detach_process_fork_fail(self, mock_os):
-                                      mock_chdir, mock_umask, mock_getpid):
         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()
+        mock_os.fork.assert_called_once_with()
-        self.assertFalse(mock_setsid.called)
+        self.assertFalse(mock_os.setsid.called)
-        self.assertFalse(mock_chdir.called)
+        self.assertFalse(mock_os.chdir.called)
-        self.assertFalse(mock_umask.called)
+        self.assertFalse(mock_os.umask.called)
-        self.assertFalse(mock__exit.called)
+        self.assertFalse(mock_os._exit.called)
-        self.assertFalse(mock_getpid.called)
 
-    def test_detach_process_chdir_fail(self, mock_fork, mock__exit,
+    def test_detach_process_chdir_fail(self, mock_os):
-                                       mock_setsid, mock_chdir, mock_umask,
-                                       mock_getpid):
         # 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_os.setsid.assert_called_once_with()
-        mock_chdir.assert_called_once_with('/')
+        mock_os.chdir.assert_called_once_with('/')
-        mock__exit.assert_called_once_with(0)
+        mock_os._exit.assert_called_once_with(0)
-        self.assertFalse(mock_umask.called)
+        self.assertFalse(mock_os.umask.called)
-        self.assertFalse(mock_getpid.called)
 
-    def test_detach_process_umask_fail(self, mock_fork, mock__exit,
+    def test_detach_process_umask_fail(self, mock_os):
-                                       mock_setsid, mock_chdir, mock_umask,
-                                       mock_getpid):
         # 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_os.setsid.assert_called_once_with()
-        mock_chdir.assert_called_once_with('/')
+        mock_os.chdir.assert_called_once_with('/')
-        mock__exit.assert_called_once_with(0)
+        mock_os._exit.assert_called_once_with(0)
-        mock_umask.assert_called_once_with(0)
+        mock_os.umask.assert_called_once_with(0)
-        self.assertFalse(mock_getpid.called)

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()
+            pid = os.fork()
-            if ret > 0:
+            if pid > 0 and parent_exits:
-                # Exit the parent process
                 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