diff --git a/src/common/logo_helper.py b/src/common/logo_helper.py index c801f9cb..743e7b7b 100644 --- a/src/common/logo_helper.py +++ b/src/common/logo_helper.py @@ -7,6 +7,7 @@ Extracted from LEDMatrix core to provide reusable functionality for plugins. import logging import os +import tempfile from pathlib import Path from typing import Dict, List, Optional, Union @@ -283,12 +284,22 @@ class LogoHelper: # Ensure directory exists with proper permissions ensure_directory_permissions(file_path.parent, get_assets_dir_mode()) - tmp_path = file_path.with_name(file_path.name + '.part') + # A unique temp name, not a fixed ".part": two plugins can + # ask for the same logo at once, and a shared name would let them + # interleave writes into one file, publish the mixture, or delete + # each other's partial. Same directory, so os.replace stays atomic. + fd, tmp_name = tempfile.mkstemp( + dir=str(file_path.parent), prefix=file_path.name + '.', suffix='.part') + tmp_path = Path(tmp_name) try: - with self.session.get(url, timeout=30, stream=True) as response: - response.raise_for_status() - downloaded = 0 - with open(tmp_path, 'wb') as f: + # fdopen outermost so the descriptor mkstemp handed back is + # always adopted and closed, including when the request itself + # raises — load_logo_with_download swallows that, so a leak + # here would accumulate quietly on a URL that keeps failing. + with os.fdopen(fd, 'wb') as f: + with self.session.get(url, timeout=30, stream=True) as response: + response.raise_for_status() + downloaded = 0 for chunk in response.iter_content(chunk_size=64 * 1024): if not chunk: continue diff --git a/src/common/sync_manager.py b/src/common/sync_manager.py index 38239415..13f04cd9 100644 --- a/src/common/sync_manager.py +++ b/src/common/sync_manager.py @@ -495,13 +495,43 @@ class DisplaySyncManager: except Exception as exc: self.logger.debug("Sync: frame decode error: %s", exc) else: - # No magic prefix: try control-message JSON, and treat a - # parse failure as a legacy (pre-magic) PNG frame. Both - # wire formats are self-describing, so no size heuristic - # is needed — a >512-byte control message used to be - # misrouted into image decode and silently dropped. + # No magic prefix. Whether the payload parses as JSON + # decides between a control message and a legacy + # (pre-magic) PNG frame — both wire formats are + # self-describing, so no size heuristic is needed. A + # >512-byte control message used to be misrouted into + # image decode and silently dropped. try: msg = json.loads(data.decode("utf-8")) + except (json.JSONDecodeError, UnicodeDecodeError): + # Not JSON — try a legacy PNG frame. + try: + img = Image.open(io.BytesIO(data)) + if img.width > _MAX_FRAME_W or img.height > _MAX_FRAME_H: + # Same cap the TCP image path applies: decode + # is deferred until load(), so check first. + self.logger.debug( + "Sync: rejected oversized legacy frame %dx%d from %s", + img.width, img.height, sender_ip, + ) + continue + img.load() + self._handle_received_frame(img, sender_ip) + except Exception as exc: + self.logger.debug("Sync: frame decode error: %s", exc) + continue + + # It parsed, so it is a control message and never a + # frame. Read and validate its fields under a guard — + # a UDP payload is attacker-shaped, so a non-object + # body makes .get() raise AttributeError and an "sx" + # carrying a non-numeric x raises ValueError/TypeError + # — but dispatch the callback *outside* it. Running + # the callback in here would let a fault in someone + # else's code read as a malformed packet and be + # logged as one. + fire_new_cycle = False + try: t = msg.get("t") if t == "hello_ack": self._leader_ip = sender_ip @@ -525,36 +555,16 @@ class DisplaySyncManager: sender_ip, ) self.write_status_file() - if self._on_new_cycle: - self._on_new_cycle() # build initial scroll image + fire_new_cycle = True # build initial scroll image elif t == "nc": # Leader started a new scroll cycle — rebuild local image - if self._on_new_cycle: - self._on_new_cycle() - except (json.JSONDecodeError, UnicodeDecodeError, KeyError, - AttributeError, TypeError, ValueError): - # Not a control message — try legacy PNG frame. - # The tuple is wide because a UDP payload is - # attacker-shaped: valid-but-non-object JSON makes - # msg.get() raise AttributeError, and an "sx" with a - # non-numeric x raises ValueError/TypeError from - # float(). Those must land here, not in the outer - # handler, which would skip this fallback and pay - # the error backoff for one malformed packet. - try: - img = Image.open(io.BytesIO(data)) - if img.width > _MAX_FRAME_W or img.height > _MAX_FRAME_H: - # Same cap the TCP image path applies: decode - # is deferred until load(), so check first. - self.logger.debug( - "Sync: rejected oversized legacy frame %dx%d from %s", - img.width, img.height, sender_ip, - ) - continue - img.load() - self._handle_received_frame(img, sender_ip) - except Exception as exc: - self.logger.debug("Sync: frame decode error: %s", exc) + fire_new_cycle = True + except (KeyError, AttributeError, TypeError, ValueError) as exc: + self.logger.debug("Sync: malformed control message: %s", exc) + continue + + if fire_new_cycle and self._on_new_cycle: + self._on_new_cycle() except socket.timeout: continue diff --git a/test/test_logo_helper.py b/test/test_logo_helper.py index 9589f28f..0b02af7d 100644 --- a/test/test_logo_helper.py +++ b/test/test_logo_helper.py @@ -16,6 +16,7 @@ Regression coverage for two fixed bugs: """ import logging +import tempfile from pathlib import Path from unittest.mock import MagicMock, patch @@ -290,6 +291,38 @@ class TestDownloadLogo: assert not path.exists() assert list(tmp_path.glob("*.part")) == [] + def test_concurrent_downloads_do_not_share_a_temp_file(self, helper, tmp_path): + # Two plugins can ask for the same logo at once. A fixed + # ".part" would let them interleave writes into one file and + # publish the mixture; each download gets its own temp name. + path = tmp_path / "PHI.png" + seen = [] + real_mkstemp = tempfile.mkstemp + + def record(*args, **kwargs): + fd, name = real_mkstemp(*args, **kwargs) + seen.append(name) + return fd, name + + with patch("src.common.logo_helper.tempfile.mkstemp", side_effect=record): + helper.session.get = MagicMock(return_value=fake_response(png_bytes())) + helper._download_logo("http://x/logo.png", path) + helper.session.get = MagicMock(return_value=fake_response(png_bytes())) + helper._download_logo("http://x/logo.png", path) + + assert len(seen) == 2 and seen[0] != seen[1] + assert path.exists() + assert list(tmp_path.glob("*.part")) == [] # both cleaned up + + def test_request_failure_leaves_no_temp_file(self, helper, tmp_path): + # mkstemp creates the file up front, so an error before any bytes + # arrive still has something to clean up. + helper.session.get = MagicMock( + side_effect=requests.RequestException("connection reset")) + with pytest.raises(requests.RequestException): + helper._download_logo("http://x/logo.png", tmp_path / "PHI.png") + assert list(tmp_path.glob("*")) == [] + def test_non_image_response_is_deleted_and_raises(self, helper, tmp_path): # Regression: undecodable bytes stayed on disk, so every later # load_logo() call hit the corrupt file instead of re-downloading. diff --git a/test/test_sync_manager.py b/test/test_sync_manager.py index 2992f6f9..bd197c84 100644 --- a/test/test_sync_manager.py +++ b/test/test_sync_manager.py @@ -511,6 +511,28 @@ class TestFollowerRecvLoop: assert mgr.get_latest_scroll_x() is None sleeps.assert_not_called() + def test_callback_failure_is_not_mistaken_for_a_malformed_packet(self, monkeypatch): + # A payload that parses is a control message, full stop. If the + # callback it triggers raises one of the types the field guard + # catches, that fault belongs to the callback: it must not send + # the packet to the image decoder, which would report it as a + # decode error and bury the real cause. The loop still survives + # it — the outer handler catches it like any other fault. + mgr = make_manager(role=SyncRole.FOLLOWER) + mgr._follower_state = FollowerState.FOLLOWER + + def boom(): + raise ValueError("callback is broken") + + mgr._on_new_cycle = boom + fake_clock(monkeypatch, sleep_fn=MagicMock()) + self._drive(mgr, json.dumps({"t": "nc"}).encode()) + + logged = " | ".join(str(c) for c in mgr.logger.debug.call_args_list) + assert "callback is broken" in logged + assert "frame decode error" not in logged + assert "malformed control message" not in logged + def test_oversized_legacy_frame_is_rejected_before_decode(self, monkeypatch): # The UDP path is reachable by any host on the LAN, so it caps # dimensions before load() just as the TCP image server does. @@ -522,7 +544,16 @@ class TestFollowerRecvLoop: def load(self): raise AssertionError("load() must not run past the cap") - monkeypatch.setattr(sync_manager.Image, "open", lambda *a, **kw: Huge()) + # Rebind the module's reference rather than mutating PIL.Image + # itself, which would hand Huge() to every caller in the process + # — including daemon threads earlier tests left running. Same + # reasoning as fake_clock above. The other names the receive loop + # reads off this reference pass through to the real module. + monkeypatch.setattr(sync_manager, "Image", SimpleNamespace( + open=lambda *a, **kw: Huge(), + frombuffer=Image.frombuffer, + DecompressionBombError=Image.DecompressionBombError, + )) self._drive(mgr, b"\x89PNG not really but not JSON either") assert mgr.get_latest_frame() is None @@ -895,7 +926,7 @@ class TestRealSocketHandshake: leader = follower = None # The free-port probe is inherently racy — the port can be taken # between release and rebind — so retry rather than fail on it. - for attempt in range(5): + for _attempt in range(5): # Probed on loopback: this only needs a port number, and the # manager's own bind is what has to succeed. If the port turns # out to be taken on another interface, the retry below covers