Harden blueman/main/NetConf.py: reliability, concurrency, portability, tests - #3318
Open
geraldo-netto wants to merge 16 commits into
Open
Harden blueman/main/NetConf.py: reliability, concurrency, portability, tests#3318geraldo-netto wants to merge 16 commits into
geraldo-netto wants to merge 16 commits into
Conversation
DHCPHandler.clean_up() announced process termination with print(), bypassing the logging configuration so the event left no trace in the mechanism's syslog/journal. Route it through logging.info with the binary name and pid for context. Add a clean_up test asserting the log line and that the dhcp lock is released. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
NetConf.clean_up() silently swallowed BridgeException from destroy_bridge with a bare `pass`, hiding teardown failures of the pan1 bridge. Log it at warning level instead. The message reports e.errno rather than str(e) because the _blueman BridgeException __str__ returns bytes and would raise TypeError when formatted. Add a test that a failing destroy_bridge is logged and the netconfig lock is still released. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The iptables binary was hardcoded as /sbin/iptables in both _add_ipt_rule and _del_ipt_rules, which fails on distributions that ship it elsewhere (usr-merged or nftables-based layouts). Add a _iptables() resolver backed by have()/_get_binary and use it in both places. Add tests for resolution, the missing-binary error, and that an added rule uses the resolved path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_is_running unconditionally read /proc/{pid}/cmdline, which is
Linux-only and crashed on platforms without procfs. Route /proc access
through a module-level _PROC_PATH and, when procfs is absent, fall back
to a liveness check via os.kill(pid, 0) (the binary name cannot be
matched there). Guard the cmdline read against OSError (pid may exit
between the exists() check and the read). Add tests for name match /
mismatch, absent pid, and the no-procfs liveness fallback.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…at-4) _enable_ip4_forwarding wrote directly to /proc/sys/net/ipv4, which only exists on Linux; on other platforms it raised an opaque FileNotFoundError mid-apply. Check the sysctl tree exists first and raise a descriptive NetworkSetupError instead. Add a test that a missing sysctl path aborts apply_settings with the explanatory message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The DHCP pid path was hardcoded to /var/run and NetConf._RUN_PATH (lock files) was a separate /var/run literal. Point _RUN_PATH at the canonical /run (falling back to /var/run only where /run is absent) and derive the handler pid path from it, so pid and lock files share one configurable location. Add tests that the pid path tracks _RUN_PATH. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a local resolver held port 53 but DNSServerProvider returned no addresses, DnsMasqHandler._start still appended --dhcp-option=option:dns-server, with an empty, trailing-comma value that dnsmasq rejects — failing the entire start instead of degrading to "address but no DNS option". Emit --port=0 unconditionally in that case but add the dns-server option only when servers exist. Add a test for the reachable-resolver / empty-servers path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DHCPHandler.apply locked "dhcp" after a successful _start even when the pid file had not been written yet (read_pid_file returned None). A later clean_up then found no pid, logged a stale-lockfile warning, and never killed the orphaned daemon — leaking a DHCP server bound to pan1. Add a bounded _poll_pid_file helper that retries reading the pid file and returns as soon as it appears. apply now polls for the pid; if none is obtained it tears down the configuration and raises instead of locking a daemon it cannot supervise. Add tests for the poll helper (present, late, never) and the no-pid teardown path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
UdhcpdHandler._start blocked on a fixed sleep(0.1) inside the privileged mechanism to wait for udhcpd's pid file. Reuse the bounded _poll_pid_file helper, which returns as soon as the pid file appears and only sleeps between retries when it is still missing. Update the success test to assert no blocking sleep occurs when the pid file is already present. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DHCPHandler.clean_up read and signaled _pid with no guard: concurrent or repeated calls could SIGTERM a recycled, unrelated pid, and the `next(...)` over running binaries had no default so it raised StopIteration when none matched. Clear _pid before signaling so a second pass cannot re-target it, give the binary lookup a None default, and swallow ProcessLookupError when the daemon is already gone. Add tests for pid clearing/idempotency, an already-dead pid, the stale-lock path, and the not-locked no-op. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d-7) DnsMasqHandler, DhcpdHandler and UdhcpdHandler each ran p.communicate() with no timeout, so a spawned daemon that never closed its stderr would block the mechanism main loop indefinitely. Add a _communicate_stderr helper that applies a bounded timeout and, on expiry, kills the process and reports a timeout error; route all three handlers through it. Add tests for the within-timeout and kill-on-timeout paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_ipt_rules was in-memory class state, but the kernel rules it tracked survive a mechanism restart (idle-exit after 30s). After re-activation the list was empty while stale MASQUERADE/FORWARD rules persisted, so _del_ipt_rules deleted nothing and a new apply skipped re-adding, leaving stale rules for the previous address. Tag every installed rule with an iptables comment (blueman-pan1) and make _del_ipt_rules reconcile against the live ruleset: it runs `iptables -S` on each blueman target chain and deletes the comment-tagged rules it finds, instead of trusting in-memory state. Update existing iptables assertions for the comment and add a flush-by-comment test that ignores unrelated rules and works with an empty in-memory list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lock/unlock/locked were plain touch/unlink/exists with no atomic check-and-set, so two near-simultaneous apply_settings calls (the mechanism is a system D-Bus service) could both see locked()==False and both enable forwarding, append iptables rules, and start DHCP daemons on pan1, corrupting the shared class state. Add an _exclusive_lock context manager that holds an fcntl.flock(LOCK_EX) on a dedicated lock file for the whole operation, and wrap both apply_settings and clean_up in it so mechanism requests are processed strictly serially. Add tests for acquire/release ordering and release on exception. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
apply_settings configures the bridge, IPv4 forwarding, iptables rules, and DHCP in sequence. If a later step raised (e.g. the DHCP _start), earlier locks, forwarding and rules stayed applied, leaving a half-configured system with no DHCP and no rollback. Wrap the apply in try/except (inside the exclusive lock) that runs the full unlocked _clean_up on any failure, making the operation all-or-nothing, then re-raises. Add a test that a failing DHCP handler leaves the bridge destroyed and the netconfig/iptables locks released. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_apply_settings had grown to a long five-phase method (cyclomatic ~10). Split each phase into a focused helper — _ensure_handler, _ensure_bridge, _configure_interface, _apply_iptables — leaving _apply_settings as a flat orchestration of validate -> handler -> bridge -> netconfig -> iptables -> dhcp. Behaviour is unchanged (including the existing duplicate FORWARD rule); _ensure_handler returns the handler instance so the dhcp step keeps its non-None type. _apply_settings cyclomatic complexity drops from ~10 to 4. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A 0 or negative pid parsed from a corrupt pid file was dangerous: /proc/0 does not exist, and on the no-procfs fallback os.kill(0/-N, 0) targets a whole process group and would report "running", after which clean_up's os.kill(pid, SIGTERM) could signal the mechanism's own group. Reject pid <= 0 up front on both paths. Also split the fallback's OSError handling: ProcessLookupError (ESRCH) means gone -> False, but PermissionError (EPERM) means the process exists and we merely cannot signal it -> running. Other OSErrors stay False. Add tests for the EPERM/ESRCH/other-OSError branches, the non-positive pid guard (with and without procfs, asserting os.kill is never reached), and a fuzz sweep over pids x os.kill behaviours asserting a bool result, no raised exception, and no os.kill for pid <= 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
geraldo-netto
commented
Jun 19, 2026
| 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") |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Works through the open
blueman/main/NetConf.pyfindings. One commit per fix; coverage ofNetConf.pyrises to ~93% viatest/main/test_netconf.py.Reliability & consistency
dns-serverdhcp-option when there are no servers, instead of emitting a dnsmasq-rejected trailing comma.DHCPHandler.clean_upidempotent — clear the cached pid before signalling, default the running-binary lookup (fixes a latentStopIteration), and swallowProcessLookupError.communicate()calls with a timeout and kill on expiry.blueman-pan1comment and reconcile against the live ruleset (iptables -S) instead of trusting in-memory state across mechanism restarts.fcntl.flockacross apply/clean_up so concurrent D-Bus calls are serialized.apply_settingsfails partway.Portability
iptablesvia PATH instead of hardcoding/sbin/iptables./procaccess in_is_runningwith anos.killliveness fallback./runand derive the pid path from a single_RUN_PATH.sleep(0.1)with the bounded pid-file poll.Observability
print().BridgeExceptionon cleanup.Tests
test/main/test_netconf.pyextended to ~45 tests;NetConf.pycoverage ~93%.mypy -p blueman --strictclean; flake8 (core codes) clean.🤖 Generated with Claude Code