4 Commits

Include renames

Author	SHA1	Message	Date
Claude	b8a64cbaa3	test(install): cover sed_escape_replacement against sed-special characters ... CodeRabbit asked for regression coverage using a project path containing an ampersand; the earlier commits on this branch already fixed the escaping, mktemp usage, and enable/start-on-fatal-render-failure findings, and the l->line rename was already applied -- this closes the one remaining gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-11 02:26:49 +00:00
Claude	a561a7500a	fix(install): escape sed replacements, use mktemp, and make render failures fatal ... Address CodeRabbit findings on install_service.sh, install_web_service.sh and install_wifi_monitor.sh: - Values interpolated into each script's sed expression (project root path, username) were not escaped, so a value containing &, \ or the | delimiter would corrupt the rendered systemd unit. Add a shared sed_escape_replacement() helper in the new scripts/install/lib_systemd_render.sh (sourced by all three scripts) and apply it to every sed replacement. - install_service.sh rendered the main and web units to the predictable path /tmp/ledmatrix.service.tmp before installing them -- a symlink/TOCTOU race (CWE-377). Use mktemp for both, with a trap to clean up on exit. - install_service.sh treated a missing template as a mere warning and then checked only whether a unit already existed at the destination before enabling/starting it, so a render failure could silently fall back to enabling a stale, previously-installed unit. Both unit blocks now exit non-zero on a missing template or a failed render. Also rename the ambiguous loop variable `l` to `line` in test/test_systemd_unit_drift.py (Ruff E741); ruff isn't wired into any CI workflow in this repo today, so this isn't currently CI-blocking, but the rename is trivial and correct regardless. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5	2026-09-11 02:22:37 +00:00
ChuckBuildsandClaude Opus 5	94f70eab31	fix(install): render the systemd units from their templates, not from heredocs ... The installers carried their own inline copies of units that also exist as templates under systemd/, and the copies drifted. install_service.sh renders ledmatrix.service from the template correctly, then wrote ledmatrix-web.service from a heredoc that predated it -- missing Wants=network-online.target, RestartSec=10, SyslogIdentifier, CacheDirectory, CacheDirectoryMode and Environment=USE_THREADING=1. install_web_service.sh had a third copy, and install_wifi_monitor.sh a fourth, that one already differing from its template (syslog where the template says journal). startup_validator.py compares the installed unit against the template, so a rig installed this way warned on every boot -- and the remedy the warning names, "re-run scripts/install/install_service.sh", reinstalled the same stale copy. The warning could never clear. Reproduced on a live rig running exactly that unit. All three installers now render systemd/*.service through the same placeholder substitution. The template gains a __USER__ placeholder rather than hardcoding User=root, because the web interface runs as whoever installed it. That last point was a second, independent cause of a permanent warning: the validator substituted a fixed "root", so any non-root install reported drift forever. It now reads User= from the installed unit -- an install-time decision, not something the template dictates -- and compares everything else strictly. first_time_install.sh already reads the installed User= the same way. Tests cover a non-root web unit not warning, a genuinely changed directive in that unit still warning, the User= fallback, and a grep-based guard that no installer under scripts/install/ contains an inline unit body. That guard is what found the install_wifi_monitor.sh copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9	2026-09-09 18:17:35 -04:00
ChuckandClaude Opus 5	71739d85d1	harden: cap malloc arenas, warn on unit drift, and grant the portal's sudo commands (#476) ... * perf(systemd): cap glibc malloc arenas on the display service Measured on a live rig 2.5 hours after start: RSS 1030 MB Private_Dirty 988 MB anonymous mappings > 10 MB 23 largest 104, 79, 66, 63, 63 MB, on 64 MB-aligned addresses threads 9 cores 3 -> glibc ceiling = 8 x 3 = 24 arenas 23 against a ceiling of 24, all 64 MB-aligned: these are glibc's per-thread malloc arenas, not live objects. The data the process was actually holding accounts for perhaps 15 MB -- the widest scroll strip observed was 35,746 x 64, about 7 MB as RGB and the same again for its numpy mirror. It is bloat rather than a leak: sampled four times over 135 seconds, RSS sat between 990 and 1030 MB rather than climbing. glibc gives each allocating thread its own arena, grows them to hold peak demand, and never gives them back. A process that builds and drops large images across several threads is exactly the shape that produces this. The device had 59 MB free at the time, on 1845 MB total. MALLOC_ARENA_MAX=2 trades a little allocator concurrency for that resident memory. It is a tuning knob rather than a fix for a defect, so the rationale and the measurements sit next to it in the unit file, and a test asserts they stay there -- a bare environment variable invites removal by whoever meets it next. Two things this is NOT, both checked rather than assumed: - Not an OOM problem today. A grep for "oom" in the service journal returned 24 matches, all of which were the radar logging zoom=9 and zoom=7. The kernel OOM killer has not fired: dmesg has zero matches. - Not currently capped by the unit's MemoryMax=85% either. That directive is in this file but absent from the unit actually installed on the rig, which reports MemoryMax=infinity, so nothing is enforcing a ceiling there. The saving is unmeasured on hardware: applying it needs a service restart, which blanks the panel, so that is the user's call rather than something to do mid-audit. If p99 frame time regresses -- it sits at 18.4 ms against a 16.7 ms budget for 60 FPS, so there is not much headroom -- raise the value rather than remove it. (cherry picked from commit 446207ffbc) * test(systemd): pin the arena value instead of accepting a range Review follow-up. The range check accepted 1, 3 and 4, so a change to 4 -- which hands most of the resident saving back -- passed a test whose whole purpose is to notice that. Pinned to the value the unit ships, in one named constant. Raising it is still a legitimate response to a frame-time regression, but it should be a visible edit here rather than silent drift, and the failure message says so. Mutation-checked: changing the unit to 4 now fails. (cherry picked from commit 73fff8d2d5) * fix(startup): warn when an installed systemd unit has drifted from the repo's Nothing re-applies systemd units after the first install. `git pull` -- which is what the web UI's update button runs -- brings a new template into the checkout, but no code in web_interface/ or src/ copies it to /etc/systemd/system, and nothing anywhere runs `systemctl daemon-reload`. The unit that actually runs is whatever first_time_install.sh wrote on day one. So every hardening added to a unit is inert on existing installs, silently. Measured on a live rig: installed /etc/systemd/system/ledmatrix.service 2026-08-06 template systemd/ledmatrix.service 2026-08-19 contents differ with the practical result that the MemoryMax=85% the repo's template specifies was not being enforced at all -- `systemctl show` reported MemoryMax=infinity. Anyone reading the template would reasonably believe the service was capped. Startup now compares each installed unit against its substituted template and warns when they differ, naming install_service.sh as the remedy. A warning, not an error, and deliberately not a silent rewrite: editing files under /etc and restarting services is the installer's job, not something a display process should do to a machine while it is booting. Making it fatal would also brick every development checkout whose unit is legitimately absent or hand-edited. Comparison ignores comments, blank lines and ordering. The template carries explanatory comments the installed copy will not have, and systemd does not care about order within a section, so a literal comparison would warn on every boot and be ignored within a week. Mutation-checked three ways: never reporting drift fails, making it fatal fails, and -- after the first attempt missed it -- comparing raw text now fails too. That last gap is worth noting: the comment-insensitivity tests originally exercised the helper directly, so a comparison that stopped calling the helper passed them all. The test that catches it goes through _validate_systemd_units. 29 startup-validator tests pass. (cherry picked from commit cf521bdfd8) * fix(install): grant the sudo commands the captive portal actually runs The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and ledmatrix_wifi. Anything the code runs under sudo that is not in one of them needs a password, which a service cannot supply, so the call fails. Five commands were being run and none of them granted: sysctl -w net.ipv4.ip_forward=0|1 wifi_manager.py:788, 883 nft add|delete table ip ledmatrix wifi_manager.py:835, 895 rfkill unblock wifi wifi_manager.py:1811 iptables ... wifi_manager.py:796, 813, 818, 871 mkdir -p .../dnsmasq-shared.d wifi_manager.py:922 Together these are the captive portal: unblock the radio, bring up the AP, add the redirect, turn on forwarding, and undo all of it afterwards. Without the grants a hardened install would associate clients to the access point and then fail to route them. Why it has gone unnoticed: a stock Raspberry Pi image ships /etc/sudoers.d/010_pi-nopasswd granting the default user <user> ALL=(ALL) NOPASSWD: ALL which satisfies every one of these regardless of what the allow-lists say. Confirmed on a live rig -- `sudo -n -l` permits sysctl there, and the blanket rule is why. The allow-lists are effectively decorative on a default image and only start mattering once that rule is removed or the service runs as another user. test_sudo_allowlist_covers_calls.py extracts every argv-style sudo call in src/ and web_interface/ and asserts an installer grants it, so the next command added without a rule fails here rather than on someone's hardened box. Getting that test honest took three passes, each worth recording: - Matching the literal "systemctl" against rules written as `$SYSTEMCTL_PATH enable ...` reported six gaps that did not exist. Binary path variables are now normalised before comparing. - Scanning the whole installer let `NFT_PATH=$(command -v nft)` -- a variable definition, not a grant -- satisfy the check on its own, so deleting the actual nft rules still passed. Only NOPASSWD lines are considered now. - `sudo -n <tool>` reported "-n" as the binary. sudo's own flags are skipped. Each of the five grants is individually mutation-checked: removing any one fails the suite. (cherry picked from commit a372b43cd1) * fix(install): drop the iptables wildcard, and pin each grant properly Review follow-up. Two findings, both right, and the first is a hole I opened myself. `NOPASSWD: iptables *` is a root shell for the web user by another name. `iptables --modprobe=/path/to/anything` runs that path as root, so a wildcard grant on iptables escalates rather than restricts. I added that rule while fixing a permissions gap, which is a worse outcome than the gap. It is gone, and a test now fails on any trailing-wildcard grant to a tool that can execute another program -- iptables, nft, tcpdump, find, awk, sed, perl, python, env. The other finding: checking only the binary made the coverage test far weaker than it looked. With `sysctl` present anywhere in the allow-list, deleting the `net.ipv4.ip_forward=0` grant still passed -- and the portal would then be unable to restore forwarding on teardown. Each required command is now matched in full, and each is mutation-checked individually, including that exact single-line case. Scope pulled in deliberately. The first version of this test tried to assert that *every* sudo call in the codebase is granted. Run honestly, it showed the portal also runs iptables, nft, `ip addr`, `ip link` and `cp` with arguments built at runtime -- an interface name, a port. Those cannot be granted safely in a sudoers file: the rule needs a trailing wildcard, and that is the escalation above. Closing that half needs a privileged helper that builds the rules itself and takes only an interface and a port, granted the way safe_plugin_rm.sh already is. That is a design decision, not a one-line grant, so the test now pins the four commands this change actually grants and the docstring says plainly what it does not cover. Better a narrow test that is true than a broad one that is not. (cherry picked from commit 500cfbc9f4) * fix(install): pin PATH, keep unit order, and tighten the sudoers assertions Three review findings, all correct. The installer resolved binaries through an inherited PATH and wrote whatever it found into sudoers as NOPASSWD grants. first_time_install.sh re-execs itself with `sudo -E`, which preserves the caller's environment, so a writable directory early in PATH turned a compromise of the low-privilege web user into permanent root -- via a file the installer itself wrote. PATH is now pinned to the system directories before anything is resolved, and every resolved binary must be root-owned and unwritable by anyone else before it reaches the sudoers file. _unit_body() sorted a unit's lines before comparing. Order is not noise in a systemd unit: repeated ExecStartPre=/ExecStartPost= run in the order they appear, and a directive that moves between [Unit], [Service] and [Install] means something different where it lands. The drift check reported no drift for units that had genuinely changed. Order is preserved now. Two of that check's own tests asserted the wrong thing -- test_reordered_directives_are_not_drift said so in its name -- and are inverted, with a second covering a directive moved between sections. The cosmetic-difference test now varies comments, blank lines and indentation, which is what the installer actually drops, rather than reversing the file. The sudoers assertions matched command prefixes, so `sysctl -w net.ipv4.ip_forward=0 *` satisfied the requirement while granting the caller arbitrary trailing arguments as root. They are exact now. The wildcard check also normalises ${NFT_PATH} the same way as $NFT_PATH; the brace is not a word boundary, so that spelling was skipped entirely. Verified by reintroducing each: a widened required grant fails the exact match, `${NFT_PATH} *` fails the wildcard check, and require_trusted_binary refuses a non-root-owned, world-writable, or missing binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-21 14:29:45 -04:00