diff --git a/blueman/main/NetConf.py b/blueman/main/NetConf.py index 95813bc28..772559676 100644 --- a/blueman/main/NetConf.py +++ b/blueman/main/NetConf.py @@ -3,6 +3,9 @@ import pathlib import ipaddress import socket +import fcntl +import contextlib +from collections.abc import Iterator from tempfile import mkstemp from time import sleep import logging @@ -11,7 +14,7 @@ from blueman.Constants import DHCP_CONFIG_FILE from blueman.Functions import have from _blueman import create_bridge, destroy_bridge, BridgeException -from subprocess import call, Popen, PIPE +from subprocess import call, run, Popen, PIPE, TimeoutExpired from blueman.main.DNSServerProvider import DNSServerProvider @@ -20,12 +23,38 @@ class NetworkSetupError(Exception): pass +_PROC_PATH = pathlib.Path("/proc") + + def _is_running(name: str, pid: int) -> bool: - path = pathlib.Path(f"/proc/{pid}") - if not path.exists(): + # A non-positive pid is never a single live process: /proc/0 does not exist + # and os.kill(0/-N, ...) targets a whole process group, so a 0 or negative + # value parsed from a corrupt pid file must not be treated as running (and + # must never reach the SIGTERM in clean_up). + if pid <= 0: return False - return name in path.joinpath("cmdline").read_text() + if _PROC_PATH.is_dir(): + path = _PROC_PATH / str(pid) + if not path.exists(): + return False + try: + return name in path.joinpath("cmdline").read_text() + except OSError: + return False + + # No procfs (non-Linux): we cannot match the binary name, so fall back to a + # liveness check via signal 0. EPERM means the process exists but we may not + # signal it, which still counts as running; ESRCH means it is gone. + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + return True + except OSError: + return False + return True def _read_pid_file(fname: pathlib.Path) -> int | None: @@ -35,6 +64,38 @@ def _read_pid_file(fname: pathlib.Path) -> int | None: return None +# A spawned DHCP daemon may not have written its pid file yet when the launcher +# returns. Poll for it a bounded number of times, returning as soon as it +# appears, instead of blocking for a fixed delay or assuming it is present. +_PID_POLL_ATTEMPTS = 20 +_PID_POLL_DELAY = 0.05 + + +def _poll_pid_file(fname: pathlib.Path) -> int | None: + for attempt in range(_PID_POLL_ATTEMPTS): + pid = _read_pid_file(fname) + if pid is not None: + return pid + if attempt + 1 < _PID_POLL_ATTEMPTS: + sleep(_PID_POLL_DELAY) + return None + + +# Upper bound for how long to wait on a spawned DHCP daemon's launcher to +# finish writing to stderr. Well-behaved daemons fork and exit (EOF) quickly; +# the timeout stops a misbehaving one from blocking the mechanism forever. +_PROC_START_TIMEOUT = 10 + + +def _communicate_stderr(p: "Popen[bytes]") -> bytes: + try: + return p.communicate(timeout=_PROC_START_TIMEOUT)[1] + except TimeoutExpired: + logging.warning("Timed out waiting for DHCP daemon to start; killing it") + p.kill() + return p.communicate()[1] or b"timed out waiting for daemon to start" + + def _get_binary(*names: str) -> str: for name in names: path = have(name) @@ -59,16 +120,23 @@ def _get_arguments(ip4_address: str) -> list[str]: @property def _pid_path(self) -> pathlib.Path: - return pathlib.Path(f"/var/run/{self._key}.pan1.pid") + return NetConf._RUN_PATH.joinpath(f"{self._key}.pan1.pid") def apply(self, ip4_address: str, ip4_mask: str) -> None: error = self._start(_get_binary(*self._BINARIES), ip4_address, ip4_mask, [ip4_address if addr.is_loopback else str(addr) for addr in DNSServerProvider.get_servers()]) if error is None: - logging.info(f"{self._key} started correctly") - self._pid = _read_pid_file(self._pid_path) - logging.info(f"pid {self._pid}") + self._pid = _poll_pid_file(self._pid_path) + if self._pid is None: + # The daemon launched but never produced a pid file, so we have + # no way to supervise or stop it later. Treat it as a failed + # start and tear down instead of locking a daemon we cannot + # track, which would otherwise leak a DHCP server on pan1. + logging.warning(f"{self._key} produced no pid file; tearing down") + self._clean_up_configuration() + raise NetworkSetupError(f"{self._key} started but wrote no pid file") + logging.info(f"{self._key} started correctly with pid {self._pid}") NetConf.lock("dhcp") else: error_msg = error.decode("UTF-8").strip() @@ -81,24 +149,29 @@ def _start(self, binary: str, ip4_address: str, ip4_mask: str, dns_servers: list def clean_up(self) -> None: self._clean_up_configuration() - if NetConf.locked("dhcp"): - if not self._pid: - pid = _read_pid_file(self._pid_path) - else: - pid = self._pid + if not NetConf.locked("dhcp"): + return - if pid is not None: - running_binary: str | None = next(binary for binary in self._BINARIES if _is_running(binary, pid)) - if running_binary is not None: - print('Terminating ' + running_binary) + pid = self._pid if self._pid else _read_pid_file(self._pid_path) + # Clear the cached pid up front so a concurrent or repeated clean_up + # cannot read it again and signal a now-unrelated (possibly recycled) + # pid a second time. + self._pid = None + + running_binary: str | None = None + if pid is not None: + running_binary = next((binary for binary in self._BINARIES if _is_running(binary, pid)), None) + if running_binary is not None: + logging.info(f"Terminating {running_binary} (pid {pid})") + try: os.kill(pid, signal.SIGTERM) - else: - running_binary = None + except ProcessLookupError: + logging.info(f"DHCP daemon pid {pid} already gone") - if pid is None or running_binary is None: - logging.info("Stale dhcp lockfile found") + if pid is None or running_binary is None: + logging.info("Stale dhcp lockfile found") - NetConf.unlock("dhcp") + NetConf.unlock("dhcp") def _clean_up_configuration(self) -> None: ... @@ -116,12 +189,18 @@ def _start(self, binary: str, ip4_address: str, ip4_mask: str, dns_servers: list with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: if s.connect_ex(("localhost", 53)) == 0: - cmd += ["--port=0", f"--dhcp-option=option:dns-server,{', '.join(dns_servers)}"] + # A local resolver already owns port 53, so disable dnsmasq's + # own DNS. Only advertise dns-server when we actually have + # addresses; an empty list would produce a trailing-comma value + # that dnsmasq rejects, failing the whole start. + cmd.append("--port=0") + if dns_servers: + cmd.append(f"--dhcp-option=option:dns-server,{', '.join(dns_servers)}") logging.info(cmd) p = Popen(cmd, stderr=PIPE) - error = p.communicate()[1] + error = _communicate_stderr(p) return error if error else None @@ -189,7 +268,7 @@ def _start(self, binary: str, ip4_address: str, ip4_mask: str, dns_servers: list cmd = [binary, "-pf", self._pid_path.as_posix(), "pan1"] p = Popen(cmd, stderr=PIPE) - error = p.communicate()[1] + error = _communicate_stderr(p) return None if p.returncode == 0 else error @@ -233,12 +312,11 @@ def _start(self, binary: str, ip4_address: str, ip4_mask: str, dns_servers: list logging.info(f"Running udhcpd with config file {self._config_file}") cmd = [binary, "-S", self._config_file.as_posix()] p = Popen(cmd, stderr=PIPE) - error = p.communicate()[1] - - # udhcpd takes time to create pid file - sleep(0.1) + error = _communicate_stderr(p) - pid = _read_pid_file(self._pid_path) + # udhcpd takes time to create its pid file; poll for it (returning as + # soon as it appears) instead of blocking on a fixed sleep. + pid = _poll_pid_file(self._pid_path) return None if p.pid and pid is not None and _is_running("udhcpd", pid) else error @@ -252,31 +330,65 @@ class NetConf: _dhcp_handler: DHCPHandler | None = None _ipt_rules: list[tuple[str, str, tuple[str, ...]]] = [] + # Every rule blueman installs is tagged with this iptables comment so it can + # be reconciled against the live kernel ruleset, even after a mechanism + # restart that lost the in-memory _ipt_rules list. + _IPT_COMMENT = "blueman-pan1" + # (table, chain) pairs blueman writes rules into; scanned when flushing. + _IPT_TARGETS: tuple[tuple[str, str], ...] = (("nat", "POSTROUTING"), ("filter", "FORWARD")) + _IPV4_SYS_PATH = pathlib.Path("/proc/sys/net/ipv4") - _RUN_PATH = pathlib.Path("/var/run") + # Prefer the canonical /run; fall back to the legacy /var/run only where + # /run is unavailable. Both hold the mechanism's pid and lock files. + _RUN_PATH = pathlib.Path("/run") if pathlib.Path("/run").is_dir() else pathlib.Path("/var/run") @classmethod def _enable_ip4_forwarding(cls) -> None: + # IPv4 forwarding is controlled through the procfs sysctl tree, which + # only exists on Linux. Fail with a clear error rather than an opaque + # FileNotFoundError on platforms that lack it. + if not cls._IPV4_SYS_PATH.is_dir(): + raise NetworkSetupError( + f"IPv4 forwarding control not available at {cls._IPV4_SYS_PATH}; NAT requires Linux") + cls._IPV4_SYS_PATH.joinpath("ip_forward").write_text("1") for p in cls._IPV4_SYS_PATH.joinpath("conf").glob("**/forwarding"): p.write_text("1") + @classmethod + def _iptables(cls) -> str: + # Resolve iptables via PATH instead of assuming /sbin/iptables, which + # does not hold on all distributions (e.g. usr-merged or nftables-based + # layouts that ship it elsewhere). + return _get_binary("iptables") + @classmethod def _add_ipt_rule(cls, table: str, chain: str, *rule_args: str) -> None: # Pass the rule as already-split arguments instead of splitting a single # string on spaces. A value that contains a space (e.g. an address that # was not validated) can no longer smuggle in extra iptables arguments. - cls._ipt_rules.append((table, chain, rule_args)) - args = ["/sbin/iptables", "-t", table, "-A", chain, *rule_args] + # Tag the rule with a comment so it can later be matched in the kernel. + tagged = (*rule_args, "-m", "comment", "--comment", cls._IPT_COMMENT) + cls._ipt_rules.append((table, chain, tagged)) + args = [cls._iptables(), "-t", table, "-A", chain, *tagged] logging.debug(" ".join(args)) ret = call(args) logging.info(f"Return code {ret}") @classmethod def _del_ipt_rules(cls) -> None: - for table, chain, rule_args in cls._ipt_rules: - call(["/sbin/iptables", "-t", table, "-D", chain, *rule_args]) + # Reconcile against the live kernel ruleset rather than trusting the + # in-memory list, which is empty after a mechanism restart while stale + # MASQUERADE/FORWARD rules from a previous run may still be installed. + iptables = cls._iptables() + for table, chain in cls._IPT_TARGETS: + result = run([iptables, "-t", table, "-S", chain], stdout=PIPE, text=True, check=False) + for line in result.stdout.splitlines(): + if cls._IPT_COMMENT not in line or not line.startswith(f"-A {chain} "): + continue + spec = line.split()[2:] # drop the leading "-A " + call([iptables, "-t", table, "-D", chain, *spec]) cls._ipt_rules = [] cls.unlock("iptables") @@ -294,61 +406,103 @@ def _validate_ipv4(ip4_address: str, ip4_mask: str) -> None: except ValueError: raise NetworkSetupError(f"Invalid IPv4 netmask: {ip4_mask!r}") + @classmethod + @contextlib.contextmanager + def _exclusive_lock(cls) -> Iterator[None]: + # The mechanism is a system D-Bus service handling concurrent + # Enable/Disable/DhcpClient calls. The touch/exists lock markers are not + # atomic, so two near-simultaneous applies could both proceed and double + # up forwarding, iptables rules and DHCP daemons. Hold a real exclusive + # advisory lock across the whole operation to serialize them. + fd = os.open(cls._RUN_PATH / "blueman-mechanism.lock", os.O_CREAT | os.O_RDWR, 0o600) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + yield + finally: + fcntl.flock(fd, fcntl.LOCK_UN) + os.close(fd) + @classmethod def apply_settings(cls, ip4_address: str, ip4_mask: str, handler: type["DHCPHandler"], address_changed: bool) -> None: - cls._validate_ipv4(ip4_address, ip4_mask) + with cls._exclusive_lock(): + try: + cls._apply_settings(ip4_address, ip4_mask, handler, address_changed) + except Exception: + # apply_settings touches several subsystems (bridge, forwarding, + # iptables, DHCP) in sequence; a failure partway leaves the + # system half-configured with no DHCP. Tear everything down so + # the operation is all-or-nothing, then re-raise. _clean_up is + # the unlocked variant because we already hold the lock. + logging.exception("apply_settings failed; rolling back") + cls._clean_up() + raise + @classmethod + def _ensure_handler(cls, handler: type["DHCPHandler"]) -> "DHCPHandler": if not isinstance(cls._dhcp_handler, handler): if cls._dhcp_handler is not None: cls._dhcp_handler.clean_up() - cls._dhcp_handler = handler() + return cls._dhcp_handler + @staticmethod + def _ensure_bridge() -> None: try: create_bridge("pan1") except BridgeException as e: if e.errno != errno.EEXIST: raise - if address_changed or not cls.locked("netconfig"): - cls._enable_ip4_forwarding() + @staticmethod + def _configure_interface(ip4_address: str, ip4_mask: str) -> None: + if have("ip"): + if call(["ip", "link", "set", "dev", "pan1", "up"]) != 0: + raise NetworkSetupError("Failed to bring up interface pan1") + if call(["ip", "address", "add", "/".join((ip4_address, ip4_mask)), "dev", "pan1"]) != 0: + raise NetworkSetupError(f"Failed to add ip address {ip4_address}with netmask {ip4_mask}") + elif have('ifconfig'): + if call(["ifconfig", "pan1", ip4_address, "netmask", ip4_mask, "up"]) != 0: + raise NetworkSetupError(f"Failed to add ip address {ip4_address}with netmask {ip4_mask}") + else: + raise NetworkSetupError( + "Neither ifconfig or ip commands are found. Please install net-tools or iproute2") - if have("ip"): - ret = call(["ip", "link", "set", "dev", "pan1", "up"]) - if ret != 0: - raise NetworkSetupError("Failed to bring up interface pan1") - - ret = call(["ip", "address", "add", "/".join((ip4_address, ip4_mask)), "dev", "pan1"]) - if ret != 0: - raise NetworkSetupError(f"Failed to add ip address {ip4_address}" - f"with netmask {ip4_mask}") - elif have('ifconfig'): - ret = call(["ifconfig", "pan1", ip4_address, "netmask", ip4_mask, "up"]) - if ret != 0: - raise NetworkSetupError(f"Failed to add ip address {ip4_address}" - f"with netmask {ip4_mask}") - else: - raise NetworkSetupError( - "Neither ifconfig or ip commands are found. Please install net-tools or iproute2") + @classmethod + def _apply_iptables(cls, ip4_address: str, ip4_mask: str) -> None: + cls._del_ipt_rules() + cls._add_ipt_rule("nat", "POSTROUTING", "-s", f"{ip4_address}/{ip4_mask}", "-j", "MASQUERADE") + cls._add_ipt_rule("filter", "FORWARD", "-i", "pan1", "-j", "ACCEPT") + cls._add_ipt_rule("filter", "FORWARD", "-o", "pan1", "-j", "ACCEPT") + cls._add_ipt_rule("filter", "FORWARD", "-i", "pan1", "-j", "ACCEPT") + + @classmethod + def _apply_settings(cls, ip4_address: str, ip4_mask: str, handler: type["DHCPHandler"], + address_changed: bool) -> None: + cls._validate_ipv4(ip4_address, ip4_mask) + dhcp_handler = cls._ensure_handler(handler) + cls._ensure_bridge() + if address_changed or not cls.locked("netconfig"): + cls._enable_ip4_forwarding() + cls._configure_interface(ip4_address, ip4_mask) cls.lock("netconfig") if address_changed or not cls.locked("iptables"): - cls._del_ipt_rules() - - cls._add_ipt_rule("nat", "POSTROUTING", "-s", f"{ip4_address}/{ip4_mask}", "-j", "MASQUERADE") - cls._add_ipt_rule("filter", "FORWARD", "-i", "pan1", "-j", "ACCEPT") - cls._add_ipt_rule("filter", "FORWARD", "-o", "pan1", "-j", "ACCEPT") - cls._add_ipt_rule("filter", "FORWARD", "-i", "pan1", "-j", "ACCEPT") + cls._apply_iptables(ip4_address, ip4_mask) cls.lock("iptables") - if address_changed or not NetConf.locked("dhcp"): - cls._dhcp_handler.clean_up() - cls._dhcp_handler.apply(ip4_address, ip4_mask) + if address_changed or not cls.locked("dhcp"): + dhcp_handler.clean_up() + dhcp_handler.apply(ip4_address, ip4_mask) @classmethod def clean_up(cls) -> None: + with cls._exclusive_lock(): + cls._clean_up() + + @classmethod + def _clean_up(cls) -> None: logging.info(cls) if cls._dhcp_handler: @@ -356,8 +510,8 @@ def clean_up(cls) -> None: try: destroy_bridge("pan1") - except BridgeException: - pass + except BridgeException as e: + logging.warning(f"Failed to destroy bridge pan1 (errno {e.errno})") cls.unlock("netconfig") cls._del_ipt_rules() diff --git a/test/main/test_netconf.py b/test/main/test_netconf.py index 19b1d0d6f..fe2acba85 100644 --- a/test/main/test_netconf.py +++ b/test/main/test_netconf.py @@ -1,5 +1,8 @@ +import errno +import fcntl import os.path import shutil +import signal import subprocess from ipaddress import IPv4Address from pathlib import Path @@ -8,7 +11,11 @@ from unittest import TestCase from unittest.mock import patch, Mock, PropertyMock -from blueman.main.NetConf import DnsMasqHandler, NetworkSetupError, DhcpdHandler, UdhcpdHandler, NetConf, DHCPHandler +from blueman.main.NetConf import ( + DnsMasqHandler, NetworkSetupError, DhcpdHandler, UdhcpdHandler, NetConf, DHCPHandler, _is_running, + _poll_pid_file, _communicate_stderr, +) +from _blueman import BridgeException class FakeSocket: @@ -50,6 +57,20 @@ def test_success_without_dns(self, lock_mock: Mock, popen_mock: Mock, have_mock: self._check_invocation(have_mock, popen_mock, ["--port=0", "--dhcp-option=option:dns-server,203.0.113.10"]) lock_mock.assert_called_with("dhcp") + @patch("blueman.main.NetConf.Popen", return_value=Popen("true")) + @patch("blueman.main.NetConf.NetConf.lock") + @patch("blueman.main.NetConf.socket.socket", lambda *args: FakeSocket(0)) + @patch("blueman.main.NetConf.DNSServerProvider.get_servers", lambda: []) + def test_local_resolver_no_dns_servers(self, lock_mock: Mock, popen_mock: Mock, have_mock: Mock) -> None: + # Local resolver present (port 53 reachable) but no DNS servers: + # disable dnsmasq DNS with --port=0 but emit no dns-server option, + # which with an empty list would be a dnsmasq-rejected trailing comma. + with open("/tmp/pid", "w") as f: + f.write("123") + DnsMasqHandler().apply("203.0.113.1", "255.255.255.0") + self._check_invocation(have_mock, popen_mock, ["--port=0"]) + lock_mock.assert_called_with("dhcp") + @patch("blueman.main.NetConf.Popen", return_value=Popen(["sh", "-c", "echo errormsg >&2"], stderr=subprocess.PIPE)) @patch("blueman.main.NetConf.socket.socket", lambda *args: FakeSocket(1)) @patch("blueman.main.NetConf.DNSServerProvider.get_servers", lambda: []) @@ -59,6 +80,25 @@ def test_failure(self, popen_mock: Mock, have_mock: Mock) -> None: self._check_invocation(have_mock, popen_mock) self.assertEqual(cm.exception.args, ("dnsmasq failed to start: errormsg",)) + @patch("blueman.main.NetConf.Popen", return_value=Popen("true")) + @patch("blueman.main.NetConf.NetConf.lock") + @patch("blueman.main.NetConf.sleep") + @patch("blueman.main.NetConf._PID_POLL_ATTEMPTS", 3) + @patch("blueman.main.NetConf.socket.socket", lambda *args: FakeSocket(1)) + @patch("blueman.main.NetConf.DNSServerProvider.get_servers", lambda: []) + def test_no_pid_file_tears_down(self, sleep_mock: Mock, lock_mock: Mock, + popen_mock: Mock, have_mock: Mock) -> None: + # Start succeeds but no pid file appears: must not lock dhcp; instead + # tear down and raise. + try: + os.remove("/tmp/pid") + except FileNotFoundError: + pass + with self.assertRaises(NetworkSetupError) as cm: + DnsMasqHandler().apply("203.0.113.1", "255.255.255.0") + self.assertIn("wrote no pid file", cm.exception.args[0]) + lock_mock.assert_not_called() + def _check_invocation(self, have_mock: Mock, popen_mock: Mock, additional_args: Optional[List[str]] = None) -> None: have_mock.assert_called_with("dnsmasq") popen_mock.assert_called_with( @@ -123,13 +163,16 @@ def tearDownClass(cls) -> None: @patch("blueman.main.NetConf.Popen", return_value=Popen(["sh", "-c", "echo warning >&2"], stderr=subprocess.PIPE)) @patch("blueman.main.NetConf.NetConf.lock") + @patch("blueman.main.NetConf.sleep") @patch("blueman.main.NetConf._is_running", lambda _name, _pid: True) - def test_success(self, lock_mock: Mock, popen_mock: Mock, have_mock: Mock) -> None: + def test_success(self, sleep_mock: Mock, lock_mock: Mock, popen_mock: Mock, have_mock: Mock) -> None: with open("/tmp/pid", "w") as f: f.write("123") UdhcpdHandler().apply("203.0.113.1", "255.255.255.0") self._check_invocation(have_mock, popen_mock) lock_mock.assert_called_with("dhcp") + # pid file already present: poll returns at once with no blocking sleep. + sleep_mock.assert_not_called() @patch("blueman.main.NetConf.Popen", return_value=Popen(["sh", "-c", "echo errormsg >&2"], stderr=subprocess.PIPE)) @patch("blueman.main.NetConf._is_running", lambda _name, _pid: False) @@ -149,6 +192,7 @@ def _check_invocation(self, have_mock: Mock, popen_mock: Mock) -> None: self.assertEqual(args[1], {"stderr": subprocess.PIPE}) +@patch("blueman.main.NetConf.NetConf._iptables", new=classmethod(lambda cls: "/sbin/iptables")) @patch("blueman.main.NetConf.NetConf._IPV4_SYS_PATH", Path("/tmp/blueman-test/ipv4")) @patch("blueman.main.NetConf.NetConf._RUN_PATH", Path("/tmp/blueman-test/run")) @patch("blueman.main.NetConf.create_bridge") @@ -167,6 +211,19 @@ def setUp(self) -> None: i1fw.parent.mkdir(parents=True) i1fw.write_text("0") + # Fake `iptables -S ` by reflecting the rules blueman has + # installed (tracked in _ipt_rules) as the live kernel ruleset, so the + # flush-by-comment path can find and delete them. + def fake_run(args: list, **_kwargs: object) -> Mock: + table, chain = args[2], args[4] + lines = [f"-A {chain} " + " ".join(spec) + for t, c, spec in NetConf._ipt_rules if t == table and c == chain] + return Mock(stdout="\n".join(lines) + ("\n" if lines else "")) + + self.run_patch = patch("blueman.main.NetConf.run", side_effect=fake_run) + self.run_patch.start() + self.addCleanup(self.run_patch.stop) + def tearDown(self) -> None: shutil.rmtree("/tmp/blueman-test") @@ -225,10 +282,13 @@ def _check_forwarding(self) -> None: def _check_iptables(self, call_mock: Mock, remove: bool = False) -> None: command = "-D" if remove else "-A" + cmt = ["-m", "comment", "--comment", "blueman-pan1"] call_mock.assert_any_call(["/sbin/iptables", "-t", "nat", command, "POSTROUTING", - "-s", "203.0.113.1/255.255.255.0", "-j", "MASQUERADE"]) - call_mock.assert_any_call(["/sbin/iptables", "-t", "filter", command, "FORWARD", "-i", "pan1", "-j", "ACCEPT"]) - call_mock.assert_any_call(["/sbin/iptables", "-t", "filter", command, "FORWARD", "-o", "pan1", "-j", "ACCEPT"]) + "-s", "203.0.113.1/255.255.255.0", "-j", "MASQUERADE", *cmt]) + call_mock.assert_any_call( + ["/sbin/iptables", "-t", "filter", command, "FORWARD", "-i", "pan1", "-j", "ACCEPT", *cmt]) + call_mock.assert_any_call( + ["/sbin/iptables", "-t", "filter", command, "FORWARD", "-o", "pan1", "-j", "ACCEPT", *cmt]) self.assertEqual(NetConf.locked("iptables"), not remove) def test_dhcp_handler_replacement(self, _call_mock: Mock, _bridge_mock: Mock) -> None: @@ -252,6 +312,186 @@ def test_cleanup(self, destroy_bridge_mock: Mock, call_mock: Mock, _create_bridg self.assertFalse(NetConf.locked("netconfig")) self.assertFalse(NetConf.locked("iptables")) + @patch("blueman.main.NetConf.destroy_bridge") + def test_apply_failure_rolls_back(self, destroy_bridge_mock: Mock, call_mock: Mock, + _create_bridge_mock: Mock) -> None: + class FailingHandler(self.TestDHCPHandler): + apply = Mock(side_effect=NetworkSetupError("dhcp boom")) + + with self.assertRaises(NetworkSetupError): + NetConf.apply_settings("203.0.113.1", "255.255.255.0", FailingHandler, False) + + # Rollback must undo the partially-applied state. + destroy_bridge_mock.assert_called_once_with("pan1") + self.assertFalse(NetConf.locked("netconfig")) + self.assertFalse(NetConf.locked("iptables")) + + def test_missing_sysctl_path_raises(self, call_mock: Mock, _bridge_mock: Mock) -> None: + with patch.object(NetConf, "_IPV4_SYS_PATH", Path("/tmp/blueman-test/does-not-exist")): + with self.assertRaises(NetworkSetupError) as cm: + NetConf.apply_settings("203.0.113.1", "255.255.255.0", self.TestDHCPHandler, False) + self.assertIn("IPv4 forwarding control not available", cm.exception.args[0]) + + @patch("blueman.main.NetConf.destroy_bridge", side_effect=BridgeException(errno.ENODEV)) + def test_cleanup_logs_bridge_failure(self, destroy_bridge_mock: Mock, call_mock: Mock, + _create_bridge_mock: Mock) -> None: + NetConf.apply_settings("203.0.113.1", "255.255.255.0", self.TestDHCPHandler2, False) + with self.assertLogs(level="WARNING") as logs: + NetConf.clean_up() + self.assertTrue(any("Failed to destroy bridge pan1" in m for m in logs.output)) + # The lock must still be released despite the bridge failure. + self.assertFalse(NetConf.locked("netconfig")) + + +class TestIsRunning(TestCase): + def setUp(self) -> None: + self.proc = Path("/tmp/blueman-proc") + self.proc.mkdir(parents=True) + self.addCleanup(lambda: shutil.rmtree(self.proc)) + + def _write_proc(self, pid: int, cmdline: str) -> None: + d = self.proc / str(pid) + d.mkdir() + d.joinpath("cmdline").write_text(cmdline) + + def test_procfs_match(self) -> None: + self._write_proc(123, "/usr/sbin/dnsmasq\0--foo") + with patch("blueman.main.NetConf._PROC_PATH", self.proc): + self.assertTrue(_is_running("dnsmasq", 123)) + + def test_procfs_name_mismatch(self) -> None: + self._write_proc(123, "/usr/sbin/other\0") + with patch("blueman.main.NetConf._PROC_PATH", self.proc): + self.assertFalse(_is_running("dnsmasq", 123)) + + def test_procfs_pid_absent(self) -> None: + with patch("blueman.main.NetConf._PROC_PATH", self.proc): + self.assertFalse(_is_running("dnsmasq", 999)) + + def test_no_procfs_falls_back_to_liveness(self) -> None: + missing = Path("/tmp/blueman-no-proc") + with patch("blueman.main.NetConf._PROC_PATH", missing): + with patch("blueman.main.NetConf.os.kill") as kill_mock: + self.assertTrue(_is_running("dnsmasq", 123)) + kill_mock.assert_called_once_with(123, 0) + with patch("blueman.main.NetConf.os.kill", side_effect=ProcessLookupError): + self.assertFalse(_is_running("dnsmasq", 123)) + + def test_no_procfs_eperm_counts_as_running(self) -> None: + # EPERM means the process exists but we may not signal it -> running. + with patch("blueman.main.NetConf._PROC_PATH", Path("/tmp/blueman-no-proc")): + with patch("blueman.main.NetConf.os.kill", side_effect=PermissionError): + self.assertTrue(_is_running("dnsmasq", 123)) + + def test_no_procfs_other_oserror_is_not_running(self) -> None: + with patch("blueman.main.NetConf._PROC_PATH", Path("/tmp/blueman-no-proc")): + with patch("blueman.main.NetConf.os.kill", side_effect=OSError): + self.assertFalse(_is_running("dnsmasq", 123)) + + def test_non_positive_pid_rejected_with_procfs(self) -> None: + # Guard runs before any /proc lookup. + with patch("blueman.main.NetConf._PROC_PATH", self.proc): + for pid in (0, -1, -1000): + with self.subTest(pid=pid): + self.assertFalse(_is_running("dnsmasq", pid)) + + def test_non_positive_pid_never_calls_kill(self) -> None: + # os.kill(0/-N, ...) would target a whole process group; the guard must + # short-circuit before os.kill is ever reached on the no-procfs path. + with patch("blueman.main.NetConf._PROC_PATH", Path("/tmp/blueman-no-proc")): + with patch("blueman.main.NetConf.os.kill") as kill_mock: + for pid in (0, -1, -42): + with self.subTest(pid=pid): + self.assertFalse(_is_running("dnsmasq", pid)) + kill_mock.assert_not_called() + + def test_fuzz_returns_bool_never_raises(self) -> None: + # Whatever the pid and however os.kill behaves, _is_running returns a + # bool and never propagates an exception or signals a process group. + oserrors = [ + ProcessLookupError(), PermissionError(), OSError(), + OSError(errno.ESRCH, "no such process"), OSError(errno.EPERM, "denied"), + OSError(errno.EINVAL, "invalid"), + ] + pids = [-(1 << 31), -1000, -1, 0, 1, 2, 123, 999999, 1 << 31] + with patch("blueman.main.NetConf._PROC_PATH", Path("/tmp/blueman-no-proc")): + for pid in pids: + for err in [None, *oserrors]: + with self.subTest(pid=pid, err=type(err).__name__): + side = None if err is None else err + with patch("blueman.main.NetConf.os.kill", side_effect=side) as kill_mock: + result = _is_running("dnsmasq", pid) + self.assertIsInstance(result, bool) + if pid <= 0: + kill_mock.assert_not_called() + + +class _CleanupHandler(DHCPHandler): + _BINARIES = ["dnsmasq"] + + +@patch("blueman.main.NetConf.NetConf._RUN_PATH", Path("/tmp/blueman-cleanup")) +class TestDHCPHandlerCleanup(TestCase): + def setUp(self) -> None: + Path("/tmp/blueman-cleanup").mkdir(parents=True) + + def tearDown(self) -> None: + shutil.rmtree("/tmp/blueman-cleanup") + + @patch("blueman.main.NetConf.os.kill") + @patch("blueman.main.NetConf._is_running", lambda _name, _pid: True) + def test_terminate_logs_binary_and_pid(self, kill_mock: Mock) -> None: + handler = _CleanupHandler() + handler._pid = 4321 + NetConf.lock("dhcp") + with self.assertLogs(level="INFO") as logs: + handler.clean_up() + kill_mock.assert_called_once_with(4321, signal.SIGTERM) + self.assertTrue(any("Terminating dnsmasq (pid 4321)" in m for m in logs.output)) + self.assertFalse(NetConf.locked("dhcp")) + + @patch("blueman.main.NetConf.os.kill") + @patch("blueman.main.NetConf._is_running", lambda _name, _pid: True) + def test_clean_up_clears_pid_and_is_idempotent(self, kill_mock: Mock) -> None: + handler = _CleanupHandler() + handler._pid = 4321 + NetConf.lock("dhcp") + handler.clean_up() + self.assertIsNone(handler._pid) + # Second call: lock already gone, must not signal anything again. + handler.clean_up() + kill_mock.assert_called_once_with(4321, signal.SIGTERM) + + @patch("blueman.main.NetConf.os.kill", side_effect=ProcessLookupError) + @patch("blueman.main.NetConf._is_running", lambda _name, _pid: True) + def test_clean_up_survives_already_dead_pid(self, kill_mock: Mock) -> None: + handler = _CleanupHandler() + handler._pid = 4321 + NetConf.lock("dhcp") + # Must not propagate ProcessLookupError, and must still unlock. + handler.clean_up() + self.assertFalse(NetConf.locked("dhcp")) + + @patch("blueman.main.NetConf.os.kill") + @patch("blueman.main.NetConf._is_running", lambda _name, _pid: False) + def test_clean_up_stale_lock_no_kill(self, kill_mock: Mock) -> None: + handler = _CleanupHandler() + handler._pid = 4321 + NetConf.lock("dhcp") + with self.assertLogs(level="INFO") as logs: + handler.clean_up() + kill_mock.assert_not_called() + self.assertTrue(any("Stale dhcp lockfile" in m for m in logs.output)) + self.assertFalse(NetConf.locked("dhcp")) + + @patch("blueman.main.NetConf.os.kill") + def test_clean_up_not_locked_is_noop(self, kill_mock: Mock) -> None: + handler = _CleanupHandler() + handler._pid = 4321 + # dhcp not locked -> nothing happens. + handler.clean_up() + kill_mock.assert_not_called() + class TestValidateIpv4(TestCase): def test_valid_addresses_pass(self) -> None: @@ -305,6 +545,7 @@ def test_fuzz_never_other_exception(self) -> None: pass +@patch("blueman.main.NetConf.NetConf._iptables", new=classmethod(lambda cls: "/sbin/iptables")) class TestIptablesRuleArgs(TestCase): def setUp(self) -> None: NetConf._ipt_rules = [] @@ -315,7 +556,8 @@ def test_args_passed_without_splitting(self, call_mock: Mock) -> None: NetConf._add_ipt_rule("nat", "POSTROUTING", "-s", "203.0.113.1/255.255.255.0", "-j", "MASQUERADE") call_mock.assert_called_once_with( ["/sbin/iptables", "-t", "nat", "-A", "POSTROUTING", - "-s", "203.0.113.1/255.255.255.0", "-j", "MASQUERADE"]) + "-s", "203.0.113.1/255.255.255.0", "-j", "MASQUERADE", + "-m", "comment", "--comment", "blueman-pan1"]) @patch("blueman.main.NetConf.call", return_value=0) def test_value_with_space_stays_single_arg(self, call_mock: Mock) -> None: @@ -326,12 +568,129 @@ def test_value_with_space_stays_single_arg(self, call_mock: Mock) -> None: self.assertIn("1.2.3.4 -j ACCEPT", args) self.assertEqual(args.count("-j"), 0) + @patch("blueman.main.NetConf.run") @patch("blueman.main.NetConf.call", return_value=0) - def test_delete_uses_same_args(self, call_mock: Mock) -> None: - NetConf._add_ipt_rule("filter", "FORWARD", "-i", "pan1", "-j", "ACCEPT") - call_mock.reset_mock() + def test_flush_deletes_kernel_rules_by_comment(self, call_mock: Mock, run_mock: Mock) -> None: + # The kernel reports a blueman-tagged rule plus an unrelated one; only + # the tagged rule must be deleted, regardless of in-memory state. + def fake_run(args: list, **_kwargs: object) -> Mock: + if args[2] == "filter" and args[4] == "FORWARD": + return Mock(stdout=( + "-A FORWARD -i pan1 -j ACCEPT -m comment --comment blueman-pan1\n" + "-A FORWARD -i eth0 -j ACCEPT\n")) + return Mock(stdout="") + + run_mock.side_effect = fake_run + NetConf._ipt_rules = [] # simulate a restarted mechanism with no memory NetConf.unlock("iptables") NetConf._del_ipt_rules() + call_mock.assert_called_once_with( - ["/sbin/iptables", "-t", "filter", "-D", "FORWARD", "-i", "pan1", "-j", "ACCEPT"]) + ["/sbin/iptables", "-t", "filter", "-D", "FORWARD", + "-i", "pan1", "-j", "ACCEPT", "-m", "comment", "--comment", "blueman-pan1"]) self.assertEqual(NetConf._ipt_rules, []) + + +class TestExclusiveLock(TestCase): + def setUp(self) -> None: + self.run_dir = Path("/tmp/blueman-lock") + self.run_dir.mkdir(parents=True) + self.addCleanup(lambda: shutil.rmtree(self.run_dir)) + self.patch = patch.object(NetConf, "_RUN_PATH", self.run_dir) + self.patch.start() + self.addCleanup(self.patch.stop) + + @patch("blueman.main.NetConf.fcntl.flock") + def test_acquires_and_releases_exclusively(self, flock_mock: Mock) -> None: + with NetConf._exclusive_lock(): + pass + modes = [c.args[1] for c in flock_mock.call_args_list] + self.assertEqual(modes, [fcntl.LOCK_EX, fcntl.LOCK_UN]) + self.assertTrue((self.run_dir / "blueman-mechanism.lock").exists()) + + @patch("blueman.main.NetConf.fcntl.flock") + def test_releases_on_exception(self, flock_mock: Mock) -> None: + with self.assertRaises(ValueError): + with NetConf._exclusive_lock(): + raise ValueError("boom") + modes = [c.args[1] for c in flock_mock.call_args_list] + self.assertIn(fcntl.LOCK_UN, modes) + + +class TestCommunicateStderr(TestCase): + def test_returns_stderr_within_timeout(self) -> None: + p = Mock() + p.communicate.return_value = (b"", b"some error") + self.assertEqual(_communicate_stderr(p), b"some error") + p.communicate.assert_called_once_with(timeout=10) + p.kill.assert_not_called() + + def test_kills_on_timeout(self) -> None: + p = Mock() + p.communicate.side_effect = [subprocess.TimeoutExpired(cmd="dnsmasq", timeout=10), (b"", b"")] + with self.assertLogs(level="WARNING"): + result = _communicate_stderr(p) + p.kill.assert_called_once_with() + self.assertIn(b"timed out", result) + + +class TestPollPidFile(TestCase): + def setUp(self) -> None: + self.path = Path("/tmp/blueman-poll.pid") + self.addCleanup(lambda: self.path.unlink(missing_ok=True)) + + @patch("blueman.main.NetConf.sleep") + def test_returns_pid_immediately_when_present(self, sleep_mock: Mock) -> None: + self.path.write_text("4321") + self.assertEqual(_poll_pid_file(self.path), 4321) + sleep_mock.assert_not_called() + + @patch("blueman.main.NetConf.sleep") + @patch("blueman.main.NetConf._PID_POLL_ATTEMPTS", 4) + def test_returns_none_after_attempts(self, sleep_mock: Mock) -> None: + self.assertIsNone(_poll_pid_file(self.path)) + # Sleeps between attempts but not after the final one. + self.assertEqual(sleep_mock.call_count, 3) + + @patch("blueman.main.NetConf.sleep") + @patch("blueman.main.NetConf._PID_POLL_ATTEMPTS", 5) + def test_returns_pid_when_it_appears_late(self, sleep_mock: Mock) -> None: + calls = {"n": 0} + + def write_on_third(_delay: float) -> None: + calls["n"] += 1 + if calls["n"] == 2: + self.path.write_text("77") + + sleep_mock.side_effect = write_on_third + self.assertEqual(_poll_pid_file(self.path), 77) + + +class TestPidPath(TestCase): + def test_pid_path_derives_from_run_path(self) -> None: + with patch.object(NetConf, "_RUN_PATH", Path("/run")): + self.assertEqual(DnsMasqHandler()._pid_path, Path("/run/dnsmasq.pan1.pid")) + + def test_pid_path_follows_run_path_override(self) -> None: + with patch.object(NetConf, "_RUN_PATH", Path("/var/run")): + self.assertEqual(UdhcpdHandler()._pid_path, Path("/var/run/udhcpd.pan1.pid")) + + +class TestIptablesResolution(TestCase): + @patch("blueman.main.NetConf.have", return_value=Path("/usr/sbin/iptables")) + def test_resolves_via_have(self, have_mock: Mock) -> None: + self.assertEqual(NetConf._iptables(), "/usr/sbin/iptables") + have_mock.assert_called_with("iptables") + + @patch("blueman.main.NetConf.have", return_value=None) + def test_missing_iptables_raises(self, _have_mock: Mock) -> None: + with self.assertRaises(FileNotFoundError): + NetConf._iptables() + + @patch("blueman.main.NetConf.have", return_value=Path("/usr/sbin/iptables")) + @patch("blueman.main.NetConf.call", return_value=0) + def test_add_rule_uses_resolved_path(self, call_mock: Mock, _have_mock: Mock) -> None: + self.addCleanup(lambda: setattr(NetConf, "_ipt_rules", [])) + NetConf._ipt_rules = [] + NetConf._add_ipt_rule("filter", "FORWARD", "-i", "pan1", "-j", "ACCEPT") + self.assertEqual(call_mock.call_args.args[0][0], "/usr/sbin/iptables")