mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-21 10:29:06 +00:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
cbfb0e035d |
Regular → Executable
@@ -37,10 +37,6 @@ echo " systemctl: $SYSTEMCTL_PATH"
|
|||||||
echo ""
|
echo ""
|
||||||
echo "Step 1: Configuring sudo permissions for nmcli..."
|
echo "Step 1: Configuring sudo permissions for nmcli..."
|
||||||
SUDOERS_FILE="/etc/sudoers.d/ledmatrix_wifi"
|
SUDOERS_FILE="/etc/sudoers.d/ledmatrix_wifi"
|
||||||
SYSCTL_PATH=$(command -v sysctl || echo /usr/sbin/sysctl)
|
|
||||||
NFT_PATH=$(command -v nft || echo /usr/sbin/nft)
|
|
||||||
RFKILL_PATH=$(command -v rfkill || echo /usr/sbin/rfkill)
|
|
||||||
MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir)
|
|
||||||
|
|
||||||
# Create a temporary sudoers file using mktemp (handles permissions better)
|
# Create a temporary sudoers file using mktemp (handles permissions better)
|
||||||
TEMP_SUDOERS=$(mktemp) || {
|
TEMP_SUDOERS=$(mktemp) || {
|
||||||
@@ -66,36 +62,6 @@ $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH start dnsmasq
|
|||||||
$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH stop dnsmasq
|
$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH stop dnsmasq
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart dnsmasq
|
$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart dnsmasq
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart NetworkManager
|
$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart NetworkManager
|
||||||
# The captive portal turns IP forwarding on while the access point is up and
|
|
||||||
# restores the previous value when it comes down (wifi_manager._setup_iptables_
|
|
||||||
# redirect / _teardown_iptables_redirect). Without this rule that sudo call
|
|
||||||
# needs a password, so forwarding stays off and clients associate to the AP but
|
|
||||||
# cannot route. It goes unnoticed on a stock Raspberry Pi image, where
|
|
||||||
# /etc/sudoers.d/010_pi-nopasswd grants the default user blanket NOPASSWD and
|
|
||||||
# masks every gap in this file -- it only bites once that blanket rule is
|
|
||||||
# removed.
|
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=0
|
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=1
|
|
||||||
# The portal's redirect lives in its own nftables table, created when the AP
|
|
||||||
# comes up and deleted when it goes down, and the radio has to be unblocked
|
|
||||||
# before the AP can start at all. Same story as the sysctl rules above: called
|
|
||||||
# with sudo, never granted here, and invisible on a stock Pi image.
|
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH add table ip ledmatrix
|
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH delete table ip ledmatrix
|
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $RFKILL_PATH unblock wifi
|
|
||||||
# NetworkManager's dnsmasq drop-in directory, exact path.
|
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $MKDIR_PATH -p /etc/NetworkManager/dnsmasq-shared.d
|
|
||||||
#
|
|
||||||
# iptables is deliberately NOT granted here. Its rules are built from the live
|
|
||||||
# interface name and port, so a rule covering them needs a trailing wildcard --
|
|
||||||
# and `iptables --modprobe=/path/to/anything` runs that path as root, so
|
|
||||||
# `NOPASSWD: iptables *` is a root shell for the web user by another name. That
|
|
||||||
# is a worse outcome than the gap it would close, which today is masked anyway
|
|
||||||
# by the blanket NOPASSWD rule on stock Pi images.
|
|
||||||
#
|
|
||||||
# Closing it safely means a wrapper script that builds the rules itself and
|
|
||||||
# takes only an interface and a port, granted the way safe_plugin_rm.sh already
|
|
||||||
# is. That belongs in its own change rather than being smuggled into this one.
|
|
||||||
|
|
||||||
# Allow copying hostapd and dnsmasq config files into place
|
# Allow copying hostapd and dnsmasq config files into place
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf
|
$WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf
|
||||||
|
|||||||
Regular → Executable
Regular → Executable
@@ -62,9 +62,6 @@ class StartupValidator:
|
|||||||
# Validate plugins if plugin manager is available
|
# Validate plugins if plugin manager is available
|
||||||
if self.plugin_manager:
|
if self.plugin_manager:
|
||||||
self._validate_plugins()
|
self._validate_plugins()
|
||||||
|
|
||||||
# Warn when the running systemd unit no longer matches the repo's
|
|
||||||
self._validate_systemd_units()
|
|
||||||
|
|
||||||
is_valid = len(self.errors) == 0
|
is_valid = len(self.errors) == 0
|
||||||
|
|
||||||
@@ -77,71 +74,6 @@ class StartupValidator:
|
|||||||
|
|
||||||
return (is_valid, self.errors.copy(), self.warnings.copy())
|
return (is_valid, self.errors.copy(), self.warnings.copy())
|
||||||
|
|
||||||
#: Units this project installs, and where each is installed to.
|
|
||||||
_UNITS = (
|
|
||||||
("systemd/ledmatrix.service", "/etc/systemd/system/ledmatrix.service"),
|
|
||||||
("systemd/ledmatrix-web.service", "/etc/systemd/system/ledmatrix-web.service"),
|
|
||||||
)
|
|
||||||
|
|
||||||
def _validate_systemd_units(self) -> None:
|
|
||||||
"""Warn when an installed unit has drifted from the repo's template.
|
|
||||||
|
|
||||||
Nothing re-applies these after the first install. `git pull` -- which is
|
|
||||||
what the web UI's update button runs -- brings a new template into the
|
|
||||||
checkout, but nothing copies it to /etc/systemd/system and nothing runs
|
|
||||||
`systemctl daemon-reload`, so the unit that actually runs is whatever
|
|
||||||
first_time_install.sh wrote on day one.
|
|
||||||
|
|
||||||
That makes every hardening added to a unit inert on existing installs.
|
|
||||||
Measured on one rig: the installed unit was thirteen days older than the
|
|
||||||
repo's and differed in content, so a MemoryMax the repo had specified
|
|
||||||
was not being enforced at all -- `systemctl show` reported
|
|
||||||
MemoryMax=infinity.
|
|
||||||
|
|
||||||
A warning rather than an error, and certainly 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 boots.
|
|
||||||
The remedy is to re-run scripts/install/install_service.sh.
|
|
||||||
"""
|
|
||||||
try:
|
|
||||||
project_root = Path(__file__).resolve().parent.parent
|
|
||||||
for template_rel, installed_path in self._UNITS:
|
|
||||||
template = project_root / template_rel
|
|
||||||
installed = Path(installed_path)
|
|
||||||
if not template.is_file() or not installed.is_file():
|
|
||||||
continue
|
|
||||||
|
|
||||||
# The template carries placeholders the installer substitutes,
|
|
||||||
# so compare the substituted form rather than the raw file.
|
|
||||||
expected = template.read_text(encoding="utf-8")
|
|
||||||
expected = expected.replace("__PROJECT_ROOT_DIR__", str(project_root))
|
|
||||||
expected = expected.replace("__USER__", "root")
|
|
||||||
|
|
||||||
try:
|
|
||||||
actual = installed.read_text(encoding="utf-8")
|
|
||||||
except PermissionError:
|
|
||||||
continue
|
|
||||||
|
|
||||||
if self._unit_body(expected) != self._unit_body(actual):
|
|
||||||
self.warnings.append(
|
|
||||||
f"{installed.name} differs from {template_rel}; the "
|
|
||||||
"installed unit is not refreshed by an update, so "
|
|
||||||
"settings added to the template are not in effect. "
|
|
||||||
"Re-run scripts/install/install_service.sh to apply them."
|
|
||||||
)
|
|
||||||
except OSError as e:
|
|
||||||
self.logger.debug("Could not compare systemd units: %s", e)
|
|
||||||
|
|
||||||
@staticmethod
|
|
||||||
def _unit_body(text: str) -> str:
|
|
||||||
"""A unit's meaningful lines: no comments, no blanks, no ordering noise."""
|
|
||||||
lines = []
|
|
||||||
for line in text.splitlines():
|
|
||||||
line = line.strip()
|
|
||||||
if line and not line.startswith("#"):
|
|
||||||
lines.append(line)
|
|
||||||
return "\n".join(sorted(lines))
|
|
||||||
|
|
||||||
def _validate_config(self) -> None:
|
def _validate_config(self) -> None:
|
||||||
"""Validate configuration files."""
|
"""Validate configuration files."""
|
||||||
try:
|
try:
|
||||||
|
|||||||
Regular → Executable
Regular → Executable
@@ -8,18 +8,6 @@ Type=simple
|
|||||||
User=root
|
User=root
|
||||||
WorkingDirectory=__PROJECT_ROOT_DIR__
|
WorkingDirectory=__PROJECT_ROOT_DIR__
|
||||||
Environment=PYTHONDONTWRITEBYTECODE=1
|
Environment=PYTHONDONTWRITEBYTECODE=1
|
||||||
# glibc gives each allocating thread its own malloc arena, up to 8 x CPU count,
|
|
||||||
# and an arena that has grown is never handed back to the OS. This process runs
|
|
||||||
# 9 threads on a 3-core Pi, so the ceiling is 24 arenas -- and a rig measured at
|
|
||||||
# 1030 MB resident held 23 large anonymous mappings on 64 MB-aligned addresses,
|
|
||||||
# 920 MB of them, while the live data it was actually holding (widest scroll
|
|
||||||
# strip seen: 35,746 x 64) accounts for roughly 15 MB. That gap is arena bloat,
|
|
||||||
# not leaked objects: RSS was flat across repeated sampling, not climbing.
|
|
||||||
#
|
|
||||||
# Capping the arenas trades a little allocator concurrency for a large amount of
|
|
||||||
# resident memory on a device that has neither to spare. 2 is the usual value;
|
|
||||||
# raise it if frame times regress.
|
|
||||||
Environment=MALLOC_ARENA_MAX=2
|
|
||||||
ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/run.py
|
ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/run.py
|
||||||
# Restart=always, not on-failure: run.py exiting 0 (a clean shutdown path taken
|
# Restart=always, not on-failure: run.py exiting 0 (a clean shutdown path taken
|
||||||
# for a reason that no longer applies, e.g. a config reload) would otherwise leave
|
# for a reason that no longer applies, e.g. a config reload) would otherwise leave
|
||||||
|
|||||||
@@ -58,7 +58,7 @@ def repos(tmp_path):
|
|||||||
def test_branch_with_upstream_uses_a_plain_pull(repos):
|
def test_branch_with_upstream_uses_a_plain_pull(repos):
|
||||||
args, note, error = resolve_pull_command(str(repos))
|
args, note, error = resolve_pull_command(str(repos))
|
||||||
assert error is None
|
assert error is None
|
||||||
assert args == ['git', 'pull', '--rebase']
|
assert args == ['git', 'pull', '--rebase', '--autostash']
|
||||||
assert note == ''
|
assert note == ''
|
||||||
|
|
||||||
|
|
||||||
@@ -73,7 +73,7 @@ def test_branch_without_upstream_falls_back_to_origin_branch(repos):
|
|||||||
|
|
||||||
args, note, error = resolve_pull_command(str(repos))
|
args, note, error = resolve_pull_command(str(repos))
|
||||||
assert error is None
|
assert error is None
|
||||||
assert args == ['git', 'pull', '--rebase', 'origin', 'audit']
|
assert args == ['git', 'pull', '--rebase', '--autostash', 'origin', 'audit']
|
||||||
assert 'audit' in note
|
assert 'audit' in note
|
||||||
|
|
||||||
|
|
||||||
@@ -155,7 +155,7 @@ def test_switching_attaches_tracking_so_pull_needs_no_fallback(repos):
|
|||||||
|
|
||||||
args, note, error = resolve_pull_command(str(repos))
|
args, note, error = resolve_pull_command(str(repos))
|
||||||
assert error is None
|
assert error is None
|
||||||
assert args == ['git', 'pull', '--rebase']
|
assert args == ['git', 'pull', '--rebase', '--autostash']
|
||||||
assert note == ''
|
assert note == ''
|
||||||
|
|
||||||
|
|
||||||
@@ -200,3 +200,37 @@ def test_stash_option_lets_the_switch_through_and_keeps_the_work(repos):
|
|||||||
assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'other'
|
assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'other'
|
||||||
# The edit is not lost — it is on the stash.
|
# The edit is not lost — it is on the stash.
|
||||||
assert 'switch to other' in _git('stash', 'list', cwd=repos).stdout
|
assert 'switch to other' in _git('stash', 'list', cwd=repos).stdout
|
||||||
|
|
||||||
|
|
||||||
|
class TestInstallerDoesNotBlockTheUpdateButton:
|
||||||
|
"""first_time_install.sh chmods scripts that git tracked as 644.
|
||||||
|
|
||||||
|
With core.fileMode true -- the default on Linux -- that leaves five
|
||||||
|
permanently modified tracked files on every machine that ran the
|
||||||
|
installer, and `git pull --rebase` refuses to start:
|
||||||
|
|
||||||
|
error: cannot pull with rebase: You have unstaged changes.
|
||||||
|
|
||||||
|
Tracking them as executable makes the installer's chmod a no-op.
|
||||||
|
"""
|
||||||
|
|
||||||
|
CHMODDED = [
|
||||||
|
'first_time_install.sh',
|
||||||
|
'start_display.sh',
|
||||||
|
'stop_display.sh',
|
||||||
|
'scripts/install/install_service.sh',
|
||||||
|
'scripts/install/install_web_service.sh',
|
||||||
|
]
|
||||||
|
|
||||||
|
def test_scripts_the_installer_chmods_are_tracked_executable(self):
|
||||||
|
import subprocess
|
||||||
|
from pathlib import Path
|
||||||
|
root = Path(__file__).resolve().parent.parent
|
||||||
|
out = subprocess.run(['git', 'ls-files', '-s', *self.CHMODDED],
|
||||||
|
capture_output=True, text=True, cwd=str(root)).stdout
|
||||||
|
modes = {line.split()[3]: line.split()[0] for line in out.strip().split('\n') if line}
|
||||||
|
non_exec = sorted(f for f, m in modes.items() if m != '100755')
|
||||||
|
assert not non_exec, (
|
||||||
|
f"{non_exec} are chmodded by the installer but tracked non-executable, "
|
||||||
|
"so every install leaves the working tree dirty and the update "
|
||||||
|
"button cannot pull")
|
||||||
|
|||||||
@@ -1,120 +0,0 @@
|
|||||||
"""The captive portal's fixed-argument sudo calls must be granted.
|
|
||||||
|
|
||||||
The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and
|
|
||||||
ledmatrix_wifi. A sudo call absent from both needs a password, which a service
|
|
||||||
cannot supply, so it fails.
|
|
||||||
|
|
||||||
Four such calls were ungranted, all of them captive-portal teardown/setup:
|
|
||||||
|
|
||||||
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
|
|
||||||
mkdir -p .../dnsmasq-shared.d wifi_manager.py:922
|
|
||||||
|
|
||||||
It goes unnoticed because a stock Raspberry Pi image ships
|
|
||||||
/etc/sudoers.d/010_pi-nopasswd granting the default user
|
|
||||||
`ALL=(ALL) NOPASSWD: ALL`, which satisfies every gap in both files. It only
|
|
||||||
bites once that blanket rule is removed or the service runs as another user.
|
|
||||||
|
|
||||||
Scope, deliberately narrow: this pins the four commands above, each of which
|
|
||||||
can be written out literally. The portal makes further sudo calls whose
|
|
||||||
arguments are built at runtime -- iptables and nft rules carrying an interface
|
|
||||||
name and a port, `ip addr`, `ip link` -- and those cannot be granted safely
|
|
||||||
here. A rule covering them needs a trailing wildcard, and
|
|
||||||
`iptables --modprobe=/path/to/anything` runs that path as root, so
|
|
||||||
`NOPASSWD: iptables *` is a root shell for the web user by another name.
|
|
||||||
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, and belongs in its own
|
|
||||||
change.
|
|
||||||
"""
|
|
||||||
import re
|
|
||||||
from pathlib import Path
|
|
||||||
|
|
||||||
import pytest
|
|
||||||
|
|
||||||
ROOT = Path(__file__).resolve().parent.parent
|
|
||||||
INSTALLERS = (
|
|
||||||
ROOT / "first_time_install.sh",
|
|
||||||
ROOT / "scripts" / "install" / "configure_wifi_permissions.sh",
|
|
||||||
)
|
|
||||||
|
|
||||||
#: Commands this change grants, each fully literal in the source.
|
|
||||||
REQUIRED = (
|
|
||||||
("sysctl", "-w", "net.ipv4.ip_forward=0"),
|
|
||||||
("sysctl", "-w", "net.ipv4.ip_forward=1"),
|
|
||||||
("nft", "add", "table", "ip", "ledmatrix"),
|
|
||||||
("nft", "delete", "table", "ip", "ledmatrix"),
|
|
||||||
("rfkill", "unblock", "wifi"),
|
|
||||||
("mkdir", "-p", "/etc/NetworkManager/dnsmasq-shared.d"),
|
|
||||||
)
|
|
||||||
|
|
||||||
#: Tools with an option that executes a program of the caller's choosing.
|
|
||||||
#: A trailing wildcard on any of these is a privilege escalation.
|
|
||||||
EXEC_CAPABLE = ("iptables", "ip6tables", "nft", "tcpdump", "find", "awk",
|
|
||||||
"sed", "perl", "python", "python3", "env")
|
|
||||||
|
|
||||||
|
|
||||||
def _grant_lines():
|
|
||||||
lines = []
|
|
||||||
for installer in INSTALLERS:
|
|
||||||
if not installer.is_file():
|
|
||||||
continue
|
|
||||||
for line in installer.read_text(encoding="utf-8", errors="replace").splitlines():
|
|
||||||
if "NOPASSWD:" in line:
|
|
||||||
lines.append(line.split("NOPASSWD:", 1)[1])
|
|
||||||
return lines
|
|
||||||
|
|
||||||
|
|
||||||
def _normalised_grants():
|
|
||||||
"""Grants with binary-path variables reduced to tool names.
|
|
||||||
|
|
||||||
Rules are written as `$SYSCTL_PATH -w ...`, so matching the literal
|
|
||||||
"sysctl" finds nothing and every rule looks absent -- which is exactly how
|
|
||||||
an earlier version of this test reported six gaps that did not exist.
|
|
||||||
Only NOPASSWD lines are considered, because taking the whole script let a
|
|
||||||
variable definition such as NFT_PATH=$(command -v nft) satisfy the check on
|
|
||||||
its own while the grant itself had been deleted.
|
|
||||||
"""
|
|
||||||
text = "\n".join(_grant_lines())
|
|
||||||
text = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", lambda m: m.group(1).lower(), text)
|
|
||||||
return re.sub(r"/usr/(?:s?bin)/", "", text)
|
|
||||||
|
|
||||||
|
|
||||||
def test_the_installers_are_present():
|
|
||||||
missing = [str(p.relative_to(ROOT)) for p in INSTALLERS if not p.is_file()]
|
|
||||||
assert not missing, f"installer(s) missing: {missing}"
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize("command", REQUIRED, ids=lambda c: " ".join(c))
|
|
||||||
def test_the_command_is_granted(command):
|
|
||||||
"""Whole command, not just the binary.
|
|
||||||
|
|
||||||
Checking only the binary made this far weaker than it looked: with
|
|
||||||
`sysctl` present anywhere, deleting the ip_forward=0 grant still passed,
|
|
||||||
and the portal would then be unable to restore forwarding on teardown.
|
|
||||||
"""
|
|
||||||
pattern = r"\s+".join(re.escape(word) for word in command)
|
|
||||||
assert re.search(pattern, _normalised_grants()), (
|
|
||||||
f"no installer grants `{' '.join(command)}`")
|
|
||||||
|
|
||||||
|
|
||||||
def test_no_wildcard_on_a_tool_that_can_exec():
|
|
||||||
"""`NOPASSWD: iptables *` hands the web user root.
|
|
||||||
|
|
||||||
iptables --modprobe=/path runs that path as root. This caught a grant added
|
|
||||||
in this very change, which is why it is here.
|
|
||||||
"""
|
|
||||||
offenders = []
|
|
||||||
for rule in _grant_lines():
|
|
||||||
rule = rule.strip()
|
|
||||||
if not rule.endswith("*"):
|
|
||||||
continue
|
|
||||||
haystack = rule.replace("_PATH", "").lower()
|
|
||||||
for tool in EXEC_CAPABLE:
|
|
||||||
if re.search(rf"(^|/|\s|\$){tool}(\s|$)", haystack):
|
|
||||||
offenders.append(rule)
|
|
||||||
break
|
|
||||||
assert not offenders, (
|
|
||||||
"wildcard grant on a tool that can execute another program:\n "
|
|
||||||
+ "\n ".join(offenders))
|
|
||||||
@@ -1,95 +0,0 @@
|
|||||||
"""The display unit must cap glibc's malloc arenas.
|
|
||||||
|
|
||||||
glibc hands each allocating thread its own malloc arena, up to 8 x CPU count,
|
|
||||||
and an arena that has grown is never returned to the OS. This process runs
|
|
||||||
threads for the render loop, the update workers and the background fetchers, so
|
|
||||||
on a 3-core Pi the ceiling is 24 arenas.
|
|
||||||
|
|
||||||
Measured on a live rig, 2.5 hours in:
|
|
||||||
|
|
||||||
RSS 1030 MB
|
|
||||||
Private_Dirty 988 MB
|
|
||||||
anonymous mappings > 10 MB 23 (ceiling is 8 x 3 = 24)
|
|
||||||
largest few 104, 79, 66, 63, 63 MB, on 64 MB-aligned addresses
|
|
||||||
|
|
||||||
against live data that 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. Repeated sampling showed RSS flat between 990 and 1030 MB rather than
|
|
||||||
climbing, so this is arena bloat rather than a leak: memory Python has freed
|
|
||||||
but glibc is holding per-arena.
|
|
||||||
|
|
||||||
The device had 59 MB free at the time.
|
|
||||||
|
|
||||||
Capping the arena count trades a little allocator concurrency for that resident
|
|
||||||
memory. The render loop is latency-sensitive, so if p99 frame time regresses the
|
|
||||||
right response is to raise this rather than remove it.
|
|
||||||
"""
|
|
||||||
import re
|
|
||||||
from pathlib import Path
|
|
||||||
|
|
||||||
import pytest
|
|
||||||
|
|
||||||
UNIT = (Path(__file__).resolve().parent.parent / "systemd" / "ledmatrix.service")
|
|
||||||
|
|
||||||
#: The value the unit is expected to carry. 2 is the usual choice for a
|
|
||||||
#: threaded Python process; 1-4 all keep some of the saving, but only one of
|
|
||||||
#: them is what this project ships.
|
|
||||||
EXPECTED_ARENA_MAX = 2
|
|
||||||
|
|
||||||
|
|
||||||
def _environment(unit_text):
|
|
||||||
return dict(
|
|
||||||
line.split("=", 2)[1:3] if line.count("=") >= 2 else (line.split("=", 1)[1], "")
|
|
||||||
for line in unit_text.splitlines()
|
|
||||||
if line.startswith("Environment=")
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
def test_the_unit_exists():
|
|
||||||
assert UNIT.is_file(), f"{UNIT} is missing"
|
|
||||||
|
|
||||||
|
|
||||||
def test_malloc_arena_max_is_capped():
|
|
||||||
env = _environment(UNIT.read_text(encoding="utf-8"))
|
|
||||||
assert "MALLOC_ARENA_MAX" in env, (
|
|
||||||
"the display unit does not cap glibc arenas; on a 3-core Pi the default "
|
|
||||||
"ceiling is 24 and a measured rig held 23 of them, 920 MB"
|
|
||||||
)
|
|
||||||
value = int(env["MALLOC_ARENA_MAX"])
|
|
||||||
# Pinned, not a range. A range let a change to 4 -- which hands most of the
|
|
||||||
# saving back -- pass unnoticed, which was the point of the finding that
|
|
||||||
# prompted this. Raising it is a legitimate response to a frame-time
|
|
||||||
# regression, but it should be a visible edit here rather than a silent
|
|
||||||
# drift, so the number lives in one place and changing it shows up in
|
|
||||||
# review.
|
|
||||||
assert value == EXPECTED_ARENA_MAX, (
|
|
||||||
f"MALLOC_ARENA_MAX={value}, expected {EXPECTED_ARENA_MAX}. If this was "
|
|
||||||
"raised deliberately because frame times regressed, update "
|
|
||||||
"EXPECTED_ARENA_MAX here and say so in the commit."
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
def test_the_reason_is_recorded_next_to_it():
|
|
||||||
"""A bare tuning knob invites removal by whoever meets it next."""
|
|
||||||
text = UNIT.read_text(encoding="utf-8")
|
|
||||||
index = text.index("Environment=MALLOC_ARENA_MAX")
|
|
||||||
preamble = text[:index].splitlines()[-12:]
|
|
||||||
comment = "\n".join(line for line in preamble if line.startswith("#"))
|
|
||||||
assert "arena" in comment.lower(), "no explanation precedes the setting"
|
|
||||||
assert re.search(r"\d", comment), (
|
|
||||||
"the explanation cites no measurement, so a reader cannot tell whether "
|
|
||||||
"it still applies to their hardware"
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize("unit", ["ledmatrix.service"])
|
|
||||||
def test_the_unit_still_parses_as_ini(unit):
|
|
||||||
"""systemd will refuse a malformed unit, and the panel stays dark."""
|
|
||||||
import configparser
|
|
||||||
|
|
||||||
path = UNIT.parent / unit
|
|
||||||
parser = configparser.ConfigParser(strict=False)
|
|
||||||
# systemd allows repeated keys; ConfigParser needs them merged, not rejected.
|
|
||||||
parser.read_string(path.read_text(encoding="utf-8"))
|
|
||||||
assert parser.has_section("Service")
|
|
||||||
assert parser.has_option("Service", "ExecStart")
|
|
||||||
@@ -1,133 +0,0 @@
|
|||||||
"""An installed unit that no longer matches the repo's must be reported.
|
|
||||||
|
|
||||||
Nothing re-applies systemd units after the first install. `git pull` -- 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 or 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. Measured on a
|
|
||||||
live rig: the installed unit was dated 2026-08-06 and the repo's 2026-08-19,
|
|
||||||
and they differed -- with the result that a MemoryMax=85% present in the repo's
|
|
||||||
template was not being enforced at all. `systemctl show` reported
|
|
||||||
MemoryMax=infinity.
|
|
||||||
|
|
||||||
This is 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 boots.
|
|
||||||
"""
|
|
||||||
import logging
|
|
||||||
from pathlib import Path
|
|
||||||
from unittest.mock import MagicMock
|
|
||||||
|
|
||||||
import pytest
|
|
||||||
|
|
||||||
from src.startup_validator import StartupValidator
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.fixture
|
|
||||||
def validator():
|
|
||||||
v = StartupValidator(config_manager=MagicMock())
|
|
||||||
v.logger = logging.getLogger("test")
|
|
||||||
v.warnings = []
|
|
||||||
v.errors = []
|
|
||||||
return v
|
|
||||||
|
|
||||||
|
|
||||||
def test_a_matching_unit_produces_no_warning(validator, tmp_path):
|
|
||||||
"""The installed unit, substituted exactly as the installer would."""
|
|
||||||
project_root = Path("src/startup_validator.py").resolve().parent.parent
|
|
||||||
template_rel = "systemd/ledmatrix.service"
|
|
||||||
template = project_root / template_rel
|
|
||||||
if not template.is_file():
|
|
||||||
pytest.skip("repo unit template not present")
|
|
||||||
|
|
||||||
installed = tmp_path / "ledmatrix.service"
|
|
||||||
installed.write_text(
|
|
||||||
template.read_text(encoding="utf-8")
|
|
||||||
.replace("__PROJECT_ROOT_DIR__", str(project_root))
|
|
||||||
.replace("__USER__", "root"),
|
|
||||||
encoding="utf-8")
|
|
||||||
|
|
||||||
validator._UNITS = ((template_rel, str(installed)),)
|
|
||||||
validator._validate_systemd_units()
|
|
||||||
assert not validator.warnings, f"a matching unit warned: {validator.warnings}"
|
|
||||||
assert not validator.errors
|
|
||||||
|
|
||||||
|
|
||||||
def test_comments_and_blank_lines_are_not_drift():
|
|
||||||
"""Otherwise every comment the repo adds would look like a changed unit."""
|
|
||||||
a = "[Service]\n# explains a setting\nExecStart=/x\nRestart=always\n"
|
|
||||||
b = "[Service]\nExecStart=/x\n\nRestart=always\n"
|
|
||||||
assert StartupValidator._unit_body(a) == StartupValidator._unit_body(b)
|
|
||||||
|
|
||||||
|
|
||||||
def test_a_changed_directive_is_drift():
|
|
||||||
a = "[Service]\nExecStart=/x\nMemoryMax=85%\n"
|
|
||||||
b = "[Service]\nExecStart=/x\n"
|
|
||||||
assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b)
|
|
||||||
|
|
||||||
|
|
||||||
def test_reordered_directives_are_not_drift():
|
|
||||||
"""systemd does not care about order within a section, so neither should this."""
|
|
||||||
a = "[Service]\nExecStart=/x\nRestart=always\n"
|
|
||||||
b = "[Service]\nRestart=always\nExecStart=/x\n"
|
|
||||||
assert StartupValidator._unit_body(a) == StartupValidator._unit_body(b)
|
|
||||||
|
|
||||||
|
|
||||||
def test_cosmetic_differences_do_not_warn(validator, tmp_path):
|
|
||||||
"""Through the real comparison, not the helper.
|
|
||||||
|
|
||||||
The repo's template carries explanatory comments the installed copy may not
|
|
||||||
have, and the installer does not preserve ordering or blank lines. If those
|
|
||||||
counted as drift, every boot would warn and the warning would be ignored.
|
|
||||||
Asserting this on _unit_body alone would not catch a comparison that stopped
|
|
||||||
calling it -- which is exactly what a careless edit does.
|
|
||||||
"""
|
|
||||||
project_root = Path("src/startup_validator.py").resolve().parent.parent
|
|
||||||
template_rel = "systemd/ledmatrix.service"
|
|
||||||
template = project_root / template_rel
|
|
||||||
if not template.is_file():
|
|
||||||
pytest.skip("repo unit template not present")
|
|
||||||
|
|
||||||
substituted = (template.read_text(encoding="utf-8")
|
|
||||||
.replace("__PROJECT_ROOT_DIR__", str(project_root))
|
|
||||||
.replace("__USER__", "root"))
|
|
||||||
# Same directives, stripped of comments and blank lines and reordered.
|
|
||||||
directives = sorted(line.strip() for line in substituted.splitlines()
|
|
||||||
if line.strip() and not line.strip().startswith("#"))
|
|
||||||
installed = tmp_path / "ledmatrix.service"
|
|
||||||
installed.write_text("\n".join(reversed(directives)) + "\n", encoding="utf-8")
|
|
||||||
|
|
||||||
validator._UNITS = ((template_rel, str(installed)),)
|
|
||||||
validator._validate_systemd_units()
|
|
||||||
assert not validator.warnings, (
|
|
||||||
f"cosmetic-only difference reported as drift: {validator.warnings}")
|
|
||||||
|
|
||||||
|
|
||||||
def test_drift_is_reported_as_a_warning(validator, tmp_path):
|
|
||||||
"""The whole point: a real difference must surface, and only as a warning."""
|
|
||||||
installed = tmp_path / "ledmatrix.service"
|
|
||||||
installed.write_text("[Service]\nExecStart=/usr/bin/python3 /x/run.py\n")
|
|
||||||
|
|
||||||
project_root = Path("src/startup_validator.py").resolve().parent.parent
|
|
||||||
template_rel = "systemd/ledmatrix.service"
|
|
||||||
template = project_root / template_rel
|
|
||||||
if not template.is_file():
|
|
||||||
pytest.skip("repo unit template not present")
|
|
||||||
|
|
||||||
validator._UNITS = ((template_rel, str(installed)),)
|
|
||||||
validator._validate_systemd_units()
|
|
||||||
|
|
||||||
assert validator.warnings, "a differing unit produced no warning"
|
|
||||||
assert "install_service.sh" in validator.warnings[0], (
|
|
||||||
"the warning does not tell the user how to fix it")
|
|
||||||
assert not validator.errors, "drift must not be fatal at startup"
|
|
||||||
|
|
||||||
|
|
||||||
def test_a_missing_installed_unit_is_silent(validator, tmp_path):
|
|
||||||
"""Development checkouts have no /etc/systemd unit; that is not drift."""
|
|
||||||
validator._UNITS = (("systemd/ledmatrix.service", str(tmp_path / "absent.service")),)
|
|
||||||
validator._validate_systemd_units()
|
|
||||||
assert not validator.warnings
|
|
||||||
assert not validator.errors
|
|
||||||
@@ -1657,13 +1657,22 @@ def resolve_pull_command(project_dir):
|
|||||||
backup, or following an install guide that names one. The update button
|
backup, or following an install guide that names one. The update button
|
||||||
then reports a failure the user cannot act on.
|
then reports a failure the user cannot act on.
|
||||||
|
|
||||||
|
``--autostash`` is passed for the same reason. Rebase refuses to start
|
||||||
|
when any tracked file is modified, and on these installs something always
|
||||||
|
is: first_time_install.sh chmods five scripts that git tracked as 644, so
|
||||||
|
every machine that ran the installer carries five permanent mode changes
|
||||||
|
and the update button reports "cannot pull with rebase: You have unstaged
|
||||||
|
changes". Those modes are corrected in this commit, but a user cannot pull
|
||||||
|
the correction while the pull is what is blocked, and any other local edit
|
||||||
|
would reproduce it anyway. Autostash reapplies the changes afterwards.
|
||||||
|
|
||||||
Returns ``(args, note, error)``. When ``origin/<branch>`` exists the pull
|
Returns ``(args, note, error)``. When ``origin/<branch>`` exists the pull
|
||||||
is made explicit against it, so the update proceeds and the branch is
|
is made explicit against it, so the update proceeds and the branch is
|
||||||
given tracking information afterwards.
|
given tracking information afterwards.
|
||||||
"""
|
"""
|
||||||
upstream = _git_upstream(project_dir)
|
upstream = _git_upstream(project_dir)
|
||||||
if upstream:
|
if upstream:
|
||||||
return ['git', 'pull', '--rebase'], '', None
|
return ['git', 'pull', '--rebase', '--autostash'], '', None
|
||||||
|
|
||||||
branch = _git_current_branch(project_dir)
|
branch = _git_current_branch(project_dir)
|
||||||
if not branch:
|
if not branch:
|
||||||
@@ -1673,7 +1682,7 @@ def resolve_pull_command(project_dir):
|
|||||||
)
|
)
|
||||||
if _git_remote_branch_exists(project_dir, branch):
|
if _git_remote_branch_exists(project_dir, branch):
|
||||||
return (
|
return (
|
||||||
['git', 'pull', '--rebase', 'origin', branch],
|
['git', 'pull', '--rebase', '--autostash', 'origin', branch],
|
||||||
f"Branch '{branch}' had no upstream; pulled from origin/{branch} and set it as the upstream.",
|
f"Branch '{branch}' had no upstream; pulled from origin/{branch} and set it as the upstream.",
|
||||||
None,
|
None,
|
||||||
)
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user