diff --git a/CHANGELOG.md b/CHANGELOG.md index f136d44a..c2ae2dc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,6 +120,13 @@ floor on the release that ships them): when the count is only known to the display service. - The Logs tab has a **Plugin errors** panel: per-plugin counts, repeating errors and a Clear button. +- Credential redaction in exception text (`src/redaction.py`) takes time + proportional to the text, not its square. Two patterns were quadratic: URL + `user:password@`, on a long unbroken run of letters or digits (a hex digest, + an ID), and `Authorization:` followed by a long run of whitespace. Either + used to stall every thread of the display service for up to seconds each + time the snapshot was published: about 0.5s for 20k characters of hex, 8s + for 20k spaces. What gets redacted is unchanged. ### Removed diff --git a/docs/SCROLL_PERFORMANCE.md b/docs/SCROLL_PERFORMANCE.md index 82e9f912..b588adfc 100644 --- a/docs/SCROLL_PERFORMANCE.md +++ b/docs/SCROLL_PERFORMANCE.md @@ -303,6 +303,76 @@ journalctl -u ledmatrix --since "-5min" --no-pager | grep -iE "px/s|px/frame" If a plugin logs its scroll config **twice** with different modes, the second line is what is running. +--- + +## A tear across the middle on fast scrolls + +**Symptom:** while text scrolls, the top and bottom halves of the panel look +shifted sideways against each other along a horizontal line at mid-height, and +the shift grows with scroll speed. It shows most in Vegas mode at high speed. + +**It is the panel's scan, not the software.** The measured panel, like most +64-row panels, is multiplexed 1:32 (some panels of the same size scan +differently, so check yours): it lights two rows at a time, one from each half +(row 0 with row 32, row 1 with row 33, …), stepping down both halves together +once per refresh. So row 31, +the last row of the top half, lights almost a whole refresh period after row 32 +right below it. Your eye follows moving text, and moving content that lights at +different times lands in different places, so the two rows meet with an offset +of roughly + +``` +offset ≈ scroll speed × refresh period +``` + +Each frame already reaches the panel whole (`SwapOnVSync` swaps complete frames +between refreshes), so there is nothing to fix in the render path; the shift is +created inside a single refresh. Other panel heights show it too, at the point +where their two scan halves meet. + +On the 2×128×64 chain above, which refreshes at about 130 Hz flat out +(7.7 ms per pass): + +| scroll speed | offset at the midline | +|---|---| +| 50 px/s (Vegas default) | ~0.4 px | +| 100 px/s | ~0.8 px | +| 150 px/s | ~1.2 px, plainly visible | + +### What changes it + +Only a shorter scan period (a faster refresh) or a slower scroll. Measure what +the panel actually achieves first. The library prints the rate with a carriage +return and no newline, so read it from the raw journal: + +```bash +# set display.hardware.show_refresh_rate to true (web UI, Display tab), restart, then: +journalctl -u ledmatrix --since "-1min" --no-pager -o cat --all | grep -a -oE "[0-9.]+Hz" | tail -5 +``` + +Turn it off again afterwards. Measured on that panel (Pi 4, single chain), +changing one setting at a time from `pwm_bits: 7`, `gpio_slowdown: 3`: + +| change | refresh, uncapped | notes | +|---|---|---| +| none | ~130 Hz | the ceiling for this wiring | +| `pwm_bits: 6` | ~138 Hz | barely faster, and half the colour depth | +| `gpio_slowdown: 2` | ~130 Hz | no faster, **and visible glitching**; keep 3 | +| `limit_refresh_rate_hz: 0` | ~130 Hz | Vegas dropped from 100 to 72–95 fps as the refresh thread took more CPU | + +None of these helps much, because the time goes into shifting each row's pixels +out: a 2×128 chain pushes 256 pixels per row down one output. What does help is +**fewer pixels per output**. On a bonnet with more than one output (the +`regular` and `classic` mappings have 3; `adafruit-hat` has 1), put each panel +on its own output and set `parallel` to the number of outputs used and +`chain_length` to the panels per output, for example `parallel: 2`, +`chain_length: 1` for two panels. Each refresh then shifts half the data, which +should roughly double the refresh rate and halve the offset. That is a cable +change, so measure again afterwards. + +Short of rewiring, keep fast scrolls moderate: at the default 50 px/s the +offset is under half a pixel. + ## Rebuilding the binding ```bash diff --git a/src/redaction.py b/src/redaction.py index b84bf1b1..a1294309 100644 --- a/src/redaction.py +++ b/src/redaction.py @@ -24,8 +24,13 @@ _REDACT_CREDENTIAL = re.compile( # silently leak the ones nobody thought of. Not covered by the generic pattern # above, whose value part stops at whitespace and so would keep the credential # once a space follows the scheme. +# +# The opening quote and the whitespace after it are one optional unit. Written +# `\s*["\']?\s*`, a whitespace run with no quote in it could be split between +# the two `\s*` in every possible way, and a header with no credential after +# it tried them all: quadratic, 8s for 20k spaces. _REDACT_AUTH_HEADER = re.compile( - r'((?:proxy-)?authorization["\']?\s*[=:]\s*["\']?\s*' + r'((?:proxy-)?authorization["\']?\s*[=:]\s*(?:["\']\s*)?' r'(?:[A-Za-z][\w.+-]*[ \t]+)?)' # optional scheme name, kept r'([^\s,"\'<>}]+)', # the credential, redacted re.IGNORECASE, @@ -34,8 +39,16 @@ _REDACT_AUTH_HEADER = re.compile( # Credentials embedded in a URL: https://user:password@host. requests quotes # the full URL in its exceptions, so this is a realistic leak. The username is # kept -- it identifies which account failed without being the secret. -_REDACT_URL_USERINFO = re.compile(r'([a-z][a-z0-9+.-]*://[^/\s:@]+:)([^/\s@]+)(@)', - re.IGNORECASE) +# +# A match may only start where a run of scheme characters starts. Unanchored, +# `[a-z][a-z0-9+.-]*://` was tried from every letter of a long run (a hex +# digest, an ID, a blob of response body), each attempt reading to the end of +# the run: quadratic, 1.6s for 20k characters, all of it holding the GIL. +# Leading digits and `+.-` sit inside group 1 so the substitution puts them +# back; the scheme proper still has to start with a letter. +_REDACT_URL_USERINFO = re.compile( + r'((? str: diff --git a/test/test_redaction.py b/test/test_redaction.py new file mode 100644 index 00000000..5f59d696 --- /dev/null +++ b/test/test_redaction.py @@ -0,0 +1,110 @@ +"""redact_credentials must stay linear in the length of its input. + +Regressions under test, both quadratic regexes in src/redaction.py: + +- The URL-userinfo pattern (`scheme://user:password@`) could start a match at + every letter of a run of scheme characters, and each attempt read to the end + of the run looking for `://`: 1.6s for a 20k-character run. +- The Authorization-header pattern had two `\\s*` separated only by an + optional quote, so a header followed by whitespace and no credential tried + every split of that whitespace between them: 8s for 20k spaces. + +The display service redacts every message, stack trace and context value it +publishes in the error snapshot, and re.sub holds the GIL throughout, so an +exception quoting a hex digest or a long ID stalled the render loop with it. +test_error_snapshot_cross_process.py's snapshot-size test spent 140s here. + +The fixed patterns have to redact exactly what the old ones did. +""" + +import time + +import pytest + +from src.redaction import redact_credentials + +# Each timed input took seconds before the fix and takes about a millisecond +# after it; the bound leaves CI plenty of headroom while still failing on a +# quadratic pattern. +_TIME_LIMIT = 1.0 + + +def _timed(text): + start = time.perf_counter() + result = redact_credentials(text) + return result, time.perf_counter() - start + + +class TestUrlUserinfo: + @pytest.mark.parametrize("text,expected", [ + ("401 for https://user:hunter2@example.com/api", + "401 for https://user:@example.com/api"), + ("HTTPS://USER:HUNTER2@EXAMPLE.COM", + "HTTPS://USER:@EXAMPLE.COM"), + ("git+ssh://deploy:hunter2@host/repo", + "git+ssh://deploy:@host/repo"), + # The scheme starts after digits or +.- in the same run. Those + # characters must survive, and the password must still go. + ("1http://user:hunter2@host", "1http://user:@host"), + ("+.-http://user:hunter2@host", "+.-http://user:@host"), + ("a1+http://user:hunter2@host", "a1+http://user:@host"), + ("see a://u:first@b and c://v:second@d", + "see a://u:@b and c://v:@d"), + ]) + def test_password_is_redacted_and_the_rest_kept(self, text, expected): + assert redact_credentials(text) == expected + + def test_a_url_without_a_password_is_untouched(self): + text = "GET https://user@example.com/path failed" + assert redact_credentials(text) == text + + +class TestAuthorizationHeader: + @pytest.mark.parametrize("text,expected", [ + ("Authorization: Bearer eyJ.SECRET.sig", "Authorization: Bearer "), + ("Proxy-Authorization: Basic dXNlcg==", "Proxy-Authorization: Basic "), + ("authorization: barecredential", "authorization: "), + # Whitespace and an opening quote around the value, in either order. + ('authorization=" Bearer tok"', 'authorization=" Bearer "'), + ("authorization: ' tok'", "authorization: ' '"), + ("authorization:\n\tBearer tok", "authorization:\n\tBearer "), + ]) + def test_credential_is_redacted_and_the_rest_kept(self, text, expected): + assert redact_credentials(text) == expected + + @pytest.mark.parametrize("text", ["authorization: ", "authorization: , next"]) + def test_a_header_without_a_credential_is_untouched(self, text): + assert redact_credentials(text) == text + + +class TestLinearTime: + @pytest.mark.parametrize("unit", ["x", "0123456789abcdef", "1a", "a+", "1"]) + def test_long_scheme_character_runs(self, unit): + text = (unit * 50_000)[:50_000] + result, elapsed = _timed(text) + assert result == text + assert elapsed < _TIME_LIMIT, f"{elapsed:.2f}s to redact {len(text)} chars of {unit!r}" + + def test_a_credential_after_a_long_run_is_still_found(self): + run = "ab12" * 10_000 + result, elapsed = _timed(f"{run} https://user:hunter2@example.com") + assert result == f"{run} https://user:@example.com" + assert elapsed < _TIME_LIMIT + + @pytest.mark.parametrize("header,whitespace", [ + ("authorization:", " "), + ("Proxy-Authorization:", "\t"), + ("authorization=", "\n"), + ]) + def test_a_header_followed_by_long_whitespace(self, header, whitespace): + text = header + whitespace * 20_000 + "," + result, elapsed = _timed(text) + assert result == text + assert elapsed < _TIME_LIMIT, ( + f"{elapsed:.2f}s to redact {header!r} and {len(text) - len(header)} more chars") + + def test_a_credential_after_long_whitespace_is_still_found(self): + gap = " " * 20_000 + result, elapsed = _timed(f"authorization:{gap}Bearer tok") + assert result == f"authorization:{gap}Bearer " + assert elapsed < _TIME_LIMIT diff --git a/test/web_interface/test_api_v3_names_resolve.py b/test/web_interface/test_api_v3_names_resolve.py new file mode 100644 index 00000000..6b6e6c1c --- /dev/null +++ b/test/web_interface/test_api_v3_names_resolve.py @@ -0,0 +1,25 @@ +"""Names two rarely-run api_v3 paths call must exist. + +Both slipped through because nothing exercised them: the Pixlet editor's +stop route only restarts the display after a SIGKILL, and the Starlark +device-location resolver only builds a cache manager when the web app has +not set one. Either raised NameError when it finally ran. +""" + +from unittest.mock import patch + +from test._api_v3_test_helpers import api_v3_module # noqa: F401 + + +def test_the_editor_stop_route_can_restart_the_display(): + from web_interface.blueprints.api_v3 import starlark + + assert callable(starlark._run_systemctl_command) + + +def test_the_device_location_resolver_builds_without_a_cache_manager(api_v3_module): + pkg = api_v3_module + with patch.object(pkg.api_v3, 'cache_manager', None, create=True), \ + patch.object(pkg, '_starlark_device_location', None): + resolver = pkg._get_starlark_device_location() + assert resolver.cache_manager is None diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index 6d26a980..29a9f4e4 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -1712,7 +1712,7 @@ def _get_starlark_device_location() -> DeviceLocationResolver: global _starlark_device_location if _starlark_device_location is None: _starlark_device_location = DeviceLocationResolver( - getattr(api_v3, 'cache_manager', None) or _ensure_cache_manager(), logger) + getattr(api_v3, 'cache_manager', None), logger) return _starlark_device_location diff --git a/web_interface/blueprints/api_v3/starlark.py b/web_interface/blueprints/api_v3/starlark.py index 856de4a4..08d592c7 100644 --- a/web_interface/blueprints/api_v3/starlark.py +++ b/web_interface/blueprints/api_v3/starlark.py @@ -11,6 +11,7 @@ from web_interface.blueprints.api_v3 import ( _PIXLET_EDITOR_DEFAULT_TIMEOUT, _PIXLET_EDITOR_MAX_TIMEOUT, _PIXLET_EDITOR_SCRIPT, _PIXLET_EDITOR_STATE, _clear_pixlet_editor_state, _find_pixlet_binary, _install_star_file, _pixlet_editor_alive, + _run_systemctl_command, _pixlet_editor_status, _read_pixlet_editor_state, _STARLARK_APPS_DIR, _standalone_render_starlark_app, _starlark_github_token, _starlark_manifest_lock,