reliability: guard PPP connection cleanup paths

blueman/main/PPPConnection.py

@@ -39,6 +39,8 @@
     19: "We failed to authenticate ourselves to the peer."
 }
 
+RFCOMM_FILE_CLOSED = "RFCOMM file descriptor is not open"
+
 
 class PPPException(Exception):
     pass
@@ -66,6 +68,11 @@ def __init__(self, port: str, number: str = "*99#", apn: str = "", user: str = "
         self.pwd = pwd
         self.port = port
         self.interface: str | None = None
+        self.file: int | None = None
+        self.pppd: subprocess.Popen[bytes] | None = None
+        self.io_watch: int | None = None
+        self.timeout: int | None = None
+        self.buffer = ""
 
         self.commands: MutableSequence[str | tuple[str, Callable[[list[str]], None], Iterable[str]]] = [
             "ATZ E0 V1 X4 &C1 +FCLASS=0",
@@ -81,7 +88,24 @@ def __init__(self, port: str, number: str = "*99#", apn: str = "", user: str = "
             self.commands.insert(-1, f'AT+CGDCONT=1,"IP","{self.apn}"')
 
     def cleanup(self) -> None:
-        os.close(self.file)
+        if self.file is None:
+            return
+        try:
+            os.close(self.file)
+        except OSError:
+            logging.exception("Failed to close PPP rfcomm file descriptor")
+        finally:
+            self.file = None
+
+    def _remove_io_watch(self) -> None:
+        if self.io_watch is not None:
+            GLib.source_remove(self.io_watch)
+            self.io_watch = None
+
+    def _remove_timeout(self) -> None:
+        if self.timeout is not None:
+            GLib.source_remove(self.timeout)
+            self.timeout = None
 
     def connect_callback(self, response: list[str]) -> None:
         if "CONNECT" in response:
@@ -163,6 +187,9 @@ def on_pppd_stdout(self, source: IO[bytes], cond: GLib.IOCondition) -> bool:
         return True
 
     def check_pppd(self) -> bool:
+        if self.pppd is None:
+            return False
+
         status = self.pppd.poll()
         if status is not None:
             if status == 0:
@@ -180,24 +207,29 @@ def check_pppd(self) -> bool:
         return True
 
     def send_command(self, command: str) -> None:
+        if self.file is None:
+            raise PPPException(RFCOMM_FILE_CLOSED)
         logging.info(f"--> {command}")
         out = f"{command}\r\n"
         os.write(self.file, out.encode("UTF-8"))
         termios.tcdrain(self.file)
 
     def on_data_ready(self, _source: int, condition: GLib.IOCondition, command_id: int) -> bool:
         if condition & GLib.IO_ERR or condition & GLib.IO_HUP:
-            GLib.source_remove(self.timeout)
+            self._remove_timeout()
             self.__cmd_response_cb(None, PPPException("Socket error"), command_id)
             self.cleanup()
             return False
         try:
+            if self.file is None:
+                raise OSError(errno.EBADF, RFCOMM_FILE_CLOSED)
             self.buffer += os.read(self.file, 1).decode('utf-8')
         except OSError as e:
             if e.errno == errno.EAGAIN:
                 logging.error("Got EAGAIN")
                 return True
             else:
+                self._remove_timeout()
                 self.__cmd_response_cb(None, PPPException("Socket error"), command_id)
                 logging.exception(e)
                 self.cleanup()
@@ -211,21 +243,26 @@ def on_data_ready(self, _source: int, condition: GLib.IOCondition, command_id: i
             lines = [x.strip("\r\n") for x in lines if x != ""]
             logging.info(f"<-- {lines}")
 
+            self._remove_timeout()
             self.__cmd_response_cb(lines, None, command_id)
             return False
 
         return True
 
     def wait_for_reply(self, command_id: int) -> None:
         def on_timeout() -> bool:
-            GLib.source_remove(self.io_watch)
+            self.timeout = None
+            self._remove_io_watch()
             self.__cmd_response_cb(None, PPPException("Modem initialization timed out"), command_id)
             self.cleanup()
             return False
 
         self.buffer = ""
         self.term_found = False
 
+        if self.file is None:
+            raise PPPException(RFCOMM_FILE_CLOSED)
+
         self.io_watch = GLib.io_add_watch(self.file, GLib.IO_IN | GLib.IO_ERR | GLib.IO_HUP, self.on_data_ready,
                                           command_id)
         self.timeout = GLib.timeout_add(15000, on_timeout)

test/main/test_pppconnection.py

@@ -1,4 +1,8 @@
+import errno
 from unittest import TestCase
+from unittest.mock import patch
+
+from gi.repository import GLib
 
 from blueman.main.PPPConnection import PPPConnection, PPPException
 
@@ -7,6 +11,12 @@ def _cgdcont_commands(conn: PPPConnection):
     return [c for c in conn.commands if isinstance(c, str) and c.startswith("AT+CGDCONT")]
 
 
+def _capture_errors(conn: PPPConnection):
+    errors = []
+    conn.connect("error-occurred", lambda _conn, message: errors.append(message))
+    return errors
+
+
 class TestPPPConnectionApn(TestCase):
     def test_empty_apn_adds_no_cgdcont(self):
         conn = PPPConnection("/dev/rfcomm0", apn="")
@@ -73,3 +83,111 @@ def test_fuzz_apn_no_unexpected_exception(self):
                     self.assertEqual(cmds, [])
                 else:
                     self.assertEqual(cmds, [f'AT+CGDCONT=1,"IP","{apn}"'])
+
+
+class TestPPPConnectionCrashGuards(TestCase):
+    def test_lifecycle_attributes_are_initialized(self):
+        conn = PPPConnection("/dev/rfcomm0")
+
+        self.assertIsNone(conn.file)
+        self.assertIsNone(conn.pppd)
+        self.assertIsNone(conn.io_watch)
+        self.assertIsNone(conn.timeout)
+        self.assertEqual(conn.buffer, "")
+
+    def test_cleanup_without_open_file_does_not_raise(self):
+        PPPConnection("/dev/rfcomm0").cleanup()
+
+    @patch("blueman.main.PPPConnection.os.close")
+    def test_cleanup_closes_open_file_once(self, close_mock):
+        conn = PPPConnection("/dev/rfcomm0")
+        conn.file = 42
+
+        conn.cleanup()
+        conn.cleanup()
+
+        close_mock.assert_called_once_with(42)
+        self.assertIsNone(conn.file)
+
+    @patch("blueman.main.PPPConnection.os.close", side_effect=OSError("bad fd"))
+    def test_cleanup_clears_file_after_close_error(self, close_mock):
+        conn = PPPConnection("/dev/rfcomm0")
+        conn.file = 42
+
+        with self.assertLogs(level="ERROR"):
+            conn.cleanup()
+
+        close_mock.assert_called_once_with(42)
+        self.assertIsNone(conn.file)
+
+    def test_check_pppd_before_spawn_stops_polling(self):
+        self.assertFalse(PPPConnection("/dev/rfcomm0").check_pppd())
+
+    @patch("blueman.main.PPPConnection.os.close")
+    @patch("blueman.main.PPPConnection.GLib.source_remove")
+    def test_socket_condition_removes_timeout_and_closes_file(self, source_remove_mock, close_mock):
+        conn = PPPConnection("/dev/rfcomm0")
+        conn.file = 42
+        conn.timeout = 99
+        errors = _capture_errors(conn)
+
+        keep = conn.on_data_ready(42, GLib.IO_HUP, 0)
+
+        self.assertFalse(keep)
+        source_remove_mock.assert_called_once_with(99)
+        close_mock.assert_called_once_with(42)
+        self.assertIsNone(conn.timeout)
+        self.assertIsNone(conn.file)
+        self.assertEqual(errors, ["Socket error"])
+
+    @patch("blueman.main.PPPConnection.os.close")
+    @patch("blueman.main.PPPConnection.os.read", side_effect=OSError(errno.EBADF, "bad fd"))
+    @patch("blueman.main.PPPConnection.GLib.source_remove")
+    def test_read_error_removes_timeout_and_closes_file(self, source_remove_mock, read_mock, close_mock):
+        conn = PPPConnection("/dev/rfcomm0")
+        conn.file = 42
+        conn.timeout = 99
+        errors = _capture_errors(conn)
+
+        with self.assertLogs(level="ERROR"):
+            keep = conn.on_data_ready(42, GLib.IOCondition(0), 0)
+
+        self.assertFalse(keep)
+        read_mock.assert_called_once_with(42, 1)
+        source_remove_mock.assert_called_once_with(99)
+        close_mock.assert_called_once_with(42)
+        self.assertIsNone(conn.timeout)
+        self.assertIsNone(conn.file)
+        self.assertEqual(errors, ["Socket error"])
+
+    @patch("blueman.main.PPPConnection.os.close")
+    @patch("blueman.main.PPPConnection.GLib.source_remove")
+    @patch("blueman.main.PPPConnection.GLib.io_add_watch", return_value=11)
+    def test_wait_timeout_removes_io_watch_and_closes_file(self, io_add_watch_mock, source_remove_mock, close_mock):
+        timeout_callbacks = []
+
+        def capture_timeout(_interval, callback):
+            timeout_callbacks.append(callback)
+            return 22
+
+        conn = PPPConnection("/dev/rfcomm0")
+        conn.file = 42
+        errors = _capture_errors(conn)
+
+        with patch("blueman.main.PPPConnection.GLib.timeout_add", side_effect=capture_timeout):
+            conn.wait_for_reply(0)
+
+        self.assertEqual(conn.io_watch, 11)
+        self.assertEqual(conn.timeout, 22)
+        self.assertEqual(len(timeout_callbacks), 1)
+
+        keep = timeout_callbacks[0]()
+
+        self.assertFalse(keep)
+        io_add_watch_mock.assert_called_once()
+        source_remove_mock.assert_called_once_with(11)
+        close_mock.assert_called_once_with(42)
+        self.assertIsNone(conn.io_watch)
+        self.assertIsNone(conn.timeout)
+        self.assertIsNone(conn.file)
+        self.assertEqual(errors, ["Modem initialization timed out"])