From e0cc3e8cbd1be71f58963de08f810656bf24ffd4 Mon Sep 17 00:00:00 2001 From: Marc Hartmayer Date: Wed, 17 Dec 2025 13:37:08 +0000 Subject: [PATCH 1/3] first_stage_test: Add tests for closed STDIN/STDOUT Signed-off-by: Marc Hartmayer --- tests/first_stage_test.py | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/tests/first_stage_test.py b/tests/first_stage_test.py index 354f7479..046f5d7a 100644 --- a/tests/first_stage_test.py +++ b/tests/first_stage_test.py @@ -1,6 +1,7 @@ import errno import operator import os +import pty import mitogen.core import mitogen.parent @@ -142,6 +143,28 @@ class DummyConnectionEOFRead(mitogen.parent.Connection): return proc +class DummyConnectionClosedStdout(mitogen.parent.Connection): + """Dummy closed stdout connection""" + + create_child = staticmethod(create_child_using_pipes) + name_prefix = "dummy_closed_stdout" + + #: Dictionary of extra kwargs passed to :attr:`create_child`. + create_child_args = {"blocking": True, + "preexec_fn": lambda: os.close(pty.STDOUT_FILENO)} + + +class DummyConnectionClosedStdin(mitogen.parent.Connection): + """Dummy closed stdin connection""" + + create_child = staticmethod(create_child_using_pipes) + name_prefix = "dummy_closed_stdin" + + #: Dictionary of extra kwargs passed to :attr:`create_child`. + create_child_args = {"blocking": True, + "preexec_fn": lambda: os.close(pty.STDIN_FILENO)} + + class DummyConnectionEndlessBlockingRead(mitogen.parent.Connection): """Dummy connection that triggers a non-returning read(STDIN) call in the first_stage. @@ -200,6 +223,24 @@ class ConnectionTest(testlib.RouterMixin, testlib.TestCase): ctx = self.router._connect(DummyConnectionBlocking, connect_timeout=0.5) self.assertEqual(3, ctx.call(operator.add, 1, 2)) + def test_closed_communication_channel(self): + """Test that first stage does not work with a closed STDIN or STDOUT + + The first stage of the boot command should bail out as soon as it + detects that STDIN or STDOUT is closed/unavailable and this results in + disconnected streams and that is mapped by the broker to an + :class:`mitogen.parent.EofError`. + + """ + with testlib.LogCapturer() as _: + e = self.assertRaises(mitogen.parent.EofError, + self.router._connect, DummyConnectionClosedStdout, connect_timeout=0.5) + self.assertIn("Bad file descriptor", str(e)) + + e = self.assertRaises(mitogen.parent.EofError, + self.router._connect, DummyConnectionClosedStdin, connect_timeout=0.5) + self.assertIn("Bad file descriptor", str(e)) + def test_broker_connect_eof_error(self): """Test that broker takes care about EOF errors in the first stage @@ -365,3 +406,87 @@ class CommandLineTest(testlib.RouterMixin, testlib.TestCase): finally: proc.stdout.close() proc.stderr.close() + + def test_closed_stdin(self): + """This test closes STDIN of the child process. + + 1. The child process detects that STDIN is unavailable + 2. The child process terminates early with an OSError exception, and + reports the issue via exception printed on STDERR. + 3. The parent process correctly identifies this condition. + + """ + + options = mitogen.parent.Options(max_message_size=123) + conn = mitogen.parent.Connection(options, self.router) + conn.context = mitogen.core.Context(None, 123) + proc = testlib.subprocess.Popen( + args=conn.get_boot_command(), + stdout=testlib.subprocess.PIPE, + stderr=testlib.subprocess.PIPE, + preexec_fn=lambda: os.close(pty.STDIN_FILENO), + close_fds=True, + ) + try: + stdout, stderr = proc.communicate(timeout=1) + except testlib.subprocess.TimeoutExpired: + proc.kill() + proc.wait(timeout=3) + self.fail("Closed STDIN situation was not recognized") + self.assertEqual(1, proc.returncode) + self.assertEqual(stdout, b"") + self.assertIn( + b("Bad file descriptor"), + stderr, + ) + + def test_closed_stdout(self): + """Test that first stage bails out if STDOUT is closed + + This test closes STDOUT of the child process. + + 1. The child process detects that STDOUT is unavailable + 2. The child process terminates early with an OSError exception, and + reports the issue via exception printed on STDERR. + 3. The parent process correctly identifies this condition. + + """ + options = mitogen.parent.Options(max_message_size=123) + conn = mitogen.parent.Connection(options, self.router) + conn.context = mitogen.core.Context(None, 123) + + stdout_r, stdout_w = mitogen.core.pipe() + mitogen.core.set_cloexec(stdout_r.fileno()) + stderr_r, stderr_w = mitogen.core.pipe() + mitogen.core.set_cloexec(stderr_r.fileno()) + try: + proc = testlib.subprocess.Popen( + args=conn.get_boot_command(), + stdout=stdout_w, + stderr=stderr_w, + preexec_fn=lambda: os.close(pty.STDOUT_FILENO), + ) + except Exception: + stdout_r.close() + stdout_w.close() + stderr_w.close() + stderr_r.close() + raise + stdout_w.close() + stderr_w.close() + try: + returncode = proc.wait(timeout=1) + except testlib.subprocess.TimeoutExpired: + proc.kill() + proc.wait(timeout=3) + self.fail("Closed STDOUT situation was not detected") + else: + stderr = stderr_r.read() + finally: + stderr_r.close() + stdout_r.close() + self.assertEqual(1, returncode) + self.assertIn( + b("Bad file descriptor"), + stderr, + ) From b68dfbe6021d7c03b78d826aa79168dbcd67eb5a Mon Sep 17 00:00:00 2001 From: Marc Hartmayer Date: Wed, 17 Dec 2025 08:47:05 +0000 Subject: [PATCH 2/3] mitogen/parent: Bail out if STDIN or STDOUT is closed Bail out if STDIN or STDOUT is closed or unavailable, as these streams are required for the communication with the parent process. Without this check, the later `os.pipe()` calls in the first_stage may return file descriptors 0 and 1, leading to a confusing and hard-to-diagnose situation. -SSH command size: 838 +SSH command size: 850 -mitogen.parent 99240 96.9KiB 51244 50.0KiB 51.6% 12956 12.7KiB 13.1% +mitogen.parent 99496 97.2KiB 51275 50.1KiB 51.5% 12964 12.7KiB 13.0% Signed-off-by: Marc Hartmayer --- mitogen/parent.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/mitogen/parent.py b/mitogen/parent.py index 97681653..fc83ef79 100644 --- a/mitogen/parent.py +++ b/mitogen/parent.py @@ -1420,6 +1420,11 @@ class Connection(object): # "[1234 refs]" during exit. @staticmethod def _first_stage(): + # Bail out in case STDIN or STDOUT is not accessible (e.g. closed). + # Otherwise, os.pipe() could reuse file descriptors 0 or 1, leading to + # unexpected behavior that is difficult to diagnose. + os.fstat(0) + os.fstat(1) R,W=os.pipe() r,w=os.pipe() if os.fork(): From 0efd2c33e2fbc3192f2aa1290307653087a72f0e Mon Sep 17 00:00:00 2001 From: Marc Hartmayer Date: Mon, 15 Dec 2025 09:10:57 +0000 Subject: [PATCH 3/3] mitogen: first_stage: Add comments about closed STDERR Signed-off-by: Marc Hartmayer --- mitogen/parent.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/mitogen/parent.py b/mitogen/parent.py index fc83ef79..29ac8f55 100644 --- a/mitogen/parent.py +++ b/mitogen/parent.py @@ -1415,9 +1415,6 @@ class Connection(object): # W: write side of interpreter stdin. # r: read side of core_src FD. # w: write side of core_src FD. - - # Final os.close(STDERR_FILENO) to avoid --py-debug build corrupting stream with - # "[1234 refs]" during exit. @staticmethod def _first_stage(): # Bail out in case STDIN or STDOUT is not accessible (e.g. closed). @@ -1425,6 +1422,8 @@ class Connection(object): # unexpected behavior that is difficult to diagnose. os.fstat(0) os.fstat(1) + # If STDERR (2) is already closed, it might be possible that one of the + # returned pipe FDs is 2. R,W=os.pipe() r,w=os.pipe() if os.fork(): @@ -1460,6 +1459,10 @@ class Connection(object): f.write(C) f.close() os.write(1,'MITO001\n'.encode()) + # Final os.close(STDERR_FILENO) to avoid `--py-debug` build corrupting + # stream with "[1234 refs]" during exit. + # If STDERR is already closed an OSError is raised, but no one cares + # as STDERR is closed and the exit status is not forwarded. os.close(2) def get_python_argv(self):