diff --git a/console/novncd b/console/novncd index 758a92d4..b8802fd0 100755 --- a/console/novncd +++ b/console/novncd @@ -5,9 +5,6 @@ import sys import logging import django -import signal -signal.signal(signal.SIGCHLD, signal.SIG_IGN) - DIR_PATH = os.path.dirname(os.path.abspath(__file__)) ROOT_PATH = os.path.abspath(os.path.join(DIR_PATH, "..", "")) os.environ.setdefault("DJANGO_SETTINGS_MODULE", "webvirtcloud.settings") diff --git a/console/socketiod b/console/socketiod index 41c132e2..a4636677 100755 --- a/console/socketiod +++ b/console/socketiod @@ -4,9 +4,6 @@ import sys import logging import django -import signal -signal.signal(signal.SIGCHLD, signal.SIG_IGN) - DIR_PATH = os.path.dirname(os.path.abspath(__file__)) ROOT_PATH = os.path.abspath(os.path.join(DIR_PATH, "..", "")) os.environ.setdefault("DJANGO_SETTINGS_MODULE", "webvirtcloud.settings") @@ -37,7 +34,6 @@ import socket import socketio import pty import select -import subprocess import struct import fcntl import termios @@ -201,7 +197,9 @@ def connect(sid, environ): uuid = conn.get_uuid() uri = conn.wvm.getURI() - subprocess.run(["conf/daemon/consolecallback", uri, uuid]) + # Become the console process; returning would run this server's code + # in the child as well. + os.execv("conf/daemon/consolecallback", ["consolecallback", uri, uuid]) else: # this is the parent process fork. sio.start_background_task(target=read_and_forward_pty_output) @@ -216,7 +214,7 @@ def disconnect(sid): # kill pty process try: os.kill(child_pid, signal.SIGKILL) - os.wait() + os.waitpid(child_pid, 0) # only the child this daemon started except ProcessLookupError: pass except ChildProcessError: diff --git a/console/test_socketiod_disabled.py b/console/test_socketiod_disabled.py index 5e986628..16cb2741 100644 --- a/console/test_socketiod_disabled.py +++ b/console/test_socketiod_disabled.py @@ -1,5 +1,4 @@ import os -import signal import subprocess import sys import tempfile @@ -28,9 +27,6 @@ class SocketiodDisabledTestCase(unittest.TestCase): DJANGO_SETTINGS_MODULE="socketiod_disabled_settings", PYTHONPATH=os.pathsep.join([tmp, str(ROOT)]), ) - # Other test modules import novncd, which sets SIGCHLD to SIG_IGN in - # this process; with that, the child's exit status is lost (always 0). - previous_sigchld = signal.signal(signal.SIGCHLD, signal.SIG_DFL) try: result = subprocess.run( [sys.executable, str(SOCKETIOD)], @@ -42,7 +38,5 @@ class SocketiodDisabledTestCase(unittest.TestCase): ) except subprocess.TimeoutExpired: self.fail("socketiod kept running although SERIAL_CONSOLE_ENABLED=False") - finally: - signal.signal(signal.SIGCHLD, previous_sigchld) self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) self.assertIn("SERIAL_CONSOLE_ENABLED", result.stderr) diff --git a/instances/test_gstfsd.py b/instances/test_gstfsd.py index 2cc198c9..36bc34cc 100644 --- a/instances/test_gstfsd.py +++ b/instances/test_gstfsd.py @@ -1,6 +1,5 @@ import json import os -import signal import socket import subprocess import sys @@ -51,9 +50,6 @@ class GstfsdTestCase(unittest.TestCase): """gstfsd hardening.""" def setUp(self): - # novncd (imported by other tests) sets SIGCHLD to SIG_IGN, which hides exit codes. - previous = signal.signal(signal.SIGCHLD, signal.SIG_DFL) - self.addCleanup(signal.signal, signal.SIGCHLD, previous) self.tmp = tempfile.TemporaryDirectory() self.addCleanup(self.tmp.cleanup) Path(self.tmp.name, "guestfs.py").write_text(FAKE_GUESTFS) diff --git a/webvirtcloud/settings.py.template b/webvirtcloud/settings.py.template index da3edc17..34640cb5 100644 --- a/webvirtcloud/settings.py.template +++ b/webvirtcloud/settings.py.template @@ -4,7 +4,6 @@ Django settings for webvirtcloud project. """ import os -import subprocess try: import ldap from django_auth_ldap.config import LDAPSearch, NestedActiveDirectoryGroupType diff --git a/webvirtcloud/test_child_processes.py b/webvirtcloud/test_child_processes.py new file mode 100644 index 00000000..a6dff469 --- /dev/null +++ b/webvirtcloud/test_child_processes.py @@ -0,0 +1,39 @@ +"""Loading the web app or novncd must not change how child processes are reaped. + +With SIGCHLD ignored process-wide, the kernel reaps every child at once and +subprocess loses its exit status: a failing command reports 0. +""" + +import subprocess +import sys +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent + +CHECK = """ +import signal, subprocess +assert signal.getsignal(signal.SIGCHLD) == signal.SIG_DFL, signal.getsignal(signal.SIGCHLD) +print(subprocess.run(["false"]).returncode) +""" + +LOAD_NOVNCD = """ +import importlib.machinery, importlib.util +loader = importlib.machinery.SourceFileLoader("novncd", "console/novncd") +loader.exec_module(importlib.util.module_from_spec(importlib.util.spec_from_loader("novncd", loader))) +""" + + +class ChildProcessTestCase(unittest.TestCase): + def run_python(self, code): + result = subprocess.run( + [sys.executable, "-c", code], cwd=ROOT, capture_output=True, text=True, timeout=60 + ) + self.assertEqual(result.returncode, 0, result.stderr) + return result.stdout.strip().splitlines()[-1] + + def test_failing_command_is_nonzero_after_loading_the_web_app(self): + self.assertEqual(self.run_python("import webvirtcloud.wsgi\n" + CHECK), "1") + + def test_failing_command_is_nonzero_after_loading_novncd(self): + self.assertEqual(self.run_python(LOAD_NOVNCD + CHECK), "1") diff --git a/webvirtcloud/wsgi.py b/webvirtcloud/wsgi.py index 79961a9a..ab9101c1 100644 --- a/webvirtcloud/wsgi.py +++ b/webvirtcloud/wsgi.py @@ -7,14 +7,6 @@ For more information on this file, see https://docs.djangoproject.com/en/4.2/howto/deployment/wsgi/ """ -import signal -import threading - -# Reap child processes to avoid zombies. Signal handlers can only be set from -# the main thread; runserver imports this module from a worker thread. -if threading.current_thread() is threading.main_thread(): - signal.signal(signal.SIGCHLD, signal.SIG_IGN) - import os from django.core.wsgi import get_wsgi_application