diff --git a/blueman/main/PPPConnection.py b/blueman/main/PPPConnection.py index 566c6dae8..61818dac5 100644 --- a/blueman/main/PPPConnection.py +++ b/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,6 +207,8 @@ 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")) @@ -187,17 +216,20 @@ def send_command(self, command: str) -> None: 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,6 +243,7 @@ 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 @@ -218,7 +251,8 @@ def on_data_ready(self, _source: int, condition: GLib.IOCondition, command_id: i 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 @@ -226,6 +260,9 @@ def on_timeout() -> bool: 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) diff --git a/test/main/test_pppconnection.py b/test/main/test_pppconnection.py index 5f8cb9656..f889f24a4 100644 --- a/test/main/test_pppconnection.py +++ b/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"])