From b150a9dd364e5fd4a0735bd4ff58bc6ce49ffa12 Mon Sep 17 00:00:00 2001 From: catborise Date: Sun, 4 Oct 2026 13:40:53 +0300 Subject: [PATCH] fix: stop ignoring SIGCHLD process-wide The web app (wsgi.py), novncd and socketiod set SIGCHLD to SIG_IGN at import. The kernel then reaps every child at once, so subprocess loses the exit status and a failing command reports 0, in the server and in anything else run in that process: the test suite had to reset the signal in two tests. It was added in 2023 against one defunct bash per gunicorn worker and per novncd. That bash came from the settings template of the time, which ran .dec_ldap_pwd.sh with Popen and never waited for it; the template stopped doing that in 561fedf. A settings.py still holding that line leaves at most one zombie per process, not a growing number. - wsgi.py, novncd, socketiod: the global SIG_IGN is removed. subprocess reaps its own children, websockify installs its own SIGCHLD reaper for novncd's connection processes, and novncd's SSH tunnels wait for their ssh process. - socketiod: the pty child execs consolecallback instead of running it and then returning into the server's code; disconnect reaps that child (waitpid) instead of any child (wait). - The settings template drops its unused subprocess import; the two test workarounds are removed. New tests load the web app and novncd in a fresh interpreter and check that SIGCHLD is untouched and a failing command still returns 1; both fail on the previous code. A patched novncd in the test container served five connections and left no child behind. --- console/novncd | 3 --- console/socketiod | 10 +++---- console/test_socketiod_disabled.py | 6 ----- instances/test_gstfsd.py | 4 --- webvirtcloud/settings.py.template | 1 - webvirtcloud/test_child_processes.py | 39 ++++++++++++++++++++++++++++ webvirtcloud/wsgi.py | 8 ------ 7 files changed, 43 insertions(+), 28 deletions(-) create mode 100644 webvirtcloud/test_child_processes.py 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