From 270bf00f6c63eb2f33e76f6aa833ab141e3065ec Mon Sep 17 00:00:00 2001
From: James Rich <2199651+jamesarich@users.noreply.github.com>
Date: Mon, 31 Aug 2026 12:12:34 -0500
Subject: [PATCH] fix(screen): harden mirror from adversarial review
Gate frame delivery on getAdminAuthorized under access control (screen
pixels are operator content, same rule as mesh packets); move the drain
to lowest getFromRadio/available priority behind the replay drain; give
each PhoneAPI its own (frameId, offset) cursor via ScreenMirror::copyChunk
so coexisting BLE/serial/TCP clients each receive complete frames; disarm
and free the snapshot on client close and on setMirror(false), with
memaudit accounting and a geometry-change realloc guard; honor the
local-connection-only contract for both admin verbs (mp.from == 0);
demote the per-frame onNotify log to TRACE; hoist the guard into a
derived HAS_SCREEN_MIRROR in configuration.h.
Regenerates protos for the reviewed contract (FORMAT_UNSPECIFIED = 0,
MONO_VLSB = 1, width/height int_size:16).
---
src/configuration.h | 8 +++++
src/graphics/Screen.cpp | 5 ++-
src/graphics/ScreenMirror.cpp | 66 ++++++++++++++++++++++-------------
src/graphics/ScreenMirror.h | 23 ++++++++----
src/mesh/PhoneAPI.cpp | 38 +++++++++++++-------
src/mesh/PhoneAPI.h | 18 ++++++++++
src/modules/AdminModule.cpp | 9 ++++-
7 files changed, 120 insertions(+), 47 deletions(-)
diff --git a/src/configuration.h b/src/configuration.h
index 5da3c7c801..503ad68433 100644
--- a/src/configuration.h
+++ b/src/configuration.h
@@ -673,6 +673,14 @@ along with this program. If not, see .
#endif
#endif
+// Display mirroring to the client (FromRadio.display_frame) rides the screen
+// and can be excluded independently with MESHTASTIC_EXCLUDE_SCREEN_MIRROR.
+#if HAS_SCREEN && !defined(MESHTASTIC_EXCLUDE_SCREEN_MIRROR)
+#define HAS_SCREEN_MIRROR 1
+#else
+#define HAS_SCREEN_MIRROR 0
+#endif
+
#ifndef USE_ETHERNET_DEFAULT
#define USE_ETHERNET_DEFAULT 0
#endif
diff --git a/src/graphics/Screen.cpp b/src/graphics/Screen.cpp
index 8159c3fc0c..4c8e18b001 100644
--- a/src/graphics/Screen.cpp
+++ b/src/graphics/Screen.cpp
@@ -184,9 +184,12 @@ static void drawLockdownLockScreen(OLEDDisplay *display)
#endif
// Give ScreenMirror a look at the committed framebuffer (no-op unless armed).
+// Deliberately scoped to updateUiFrame commits: the few direct display()
+// paths it bypasses (EInk re-commits, boot logo) immediately follow or
+// precede a mirrored frame.
static inline void screenMirrorCapture()
{
-#if HAS_SCREEN && !defined(MESHTASTIC_EXCLUDE_SCREEN_MIRROR)
+#if HAS_SCREEN_MIRROR
if (screen)
screenMirror.onRendered(screen->getDisplayDevice());
#endif
diff --git a/src/graphics/ScreenMirror.cpp b/src/graphics/ScreenMirror.cpp
index af5e420be9..4ab4e6cb52 100644
--- a/src/graphics/ScreenMirror.cpp
+++ b/src/graphics/ScreenMirror.cpp
@@ -1,9 +1,10 @@
#include "ScreenMirror.h"
-#if HAS_SCREEN && !defined(MESHTASTIC_EXCLUDE_SCREEN_MIRROR)
+#if HAS_SCREEN_MIRROR
#include "DebugConfiguration.h"
#include "concurrency/LockGuard.h"
+#include "memory/MemAudit.h"
#include
#include
@@ -12,14 +13,28 @@ namespace graphics
ScreenMirror screenMirror;
+void ScreenMirror::freeSnapshotLocked()
+{
+ if (snapshot) {
+ memaudit::add("display", -(int32_t)frameSize);
+ free(snapshot);
+ snapshot = nullptr;
+ }
+ frameSize = 0;
+}
+
void ScreenMirror::setMirror(bool enabled)
{
concurrency::LockGuard g(&lock);
mirroring = enabled;
- // Force an immediate frame on enable so the client doesn't wait for the
- // next on-screen change.
- if (enabled)
+ if (enabled) {
+ // Force an immediate frame so the client doesn't wait for the next
+ // on-screen change.
oneShot = true;
+ } else {
+ oneShot = false;
+ freeSnapshotLocked();
+ }
LOG_INFO("Screen mirror %s", enabled ? "enabled" : "disabled");
}
@@ -39,59 +54,60 @@ void ScreenMirror::onRendered(OLEDDisplay *display)
if (!display || !display->buffer)
return;
- // Don't overwrite a frame the client is still draining; the next
- // render picks up the latest contents.
- if (snapshot && sendOffset < frameSize)
- return;
-
uint16_t w = display->getWidth();
uint16_t h = display->getHeight();
uint16_t size = w * ((h + 7) / 8);
if (size == 0)
return;
- if (!snapshot) {
+ if (snapshot && size != frameSize)
+ freeSnapshotLocked(); // display geometry changed; start over
+
+ bool firstFrame = !snapshot;
+ if (firstFrame) {
snapshot = (uint8_t *)malloc(size);
if (!snapshot) {
LOG_ERROR("Screen mirror: no memory for %u byte snapshot", size);
mirroring = oneShot = false;
return;
}
+ memaudit::add("display", size);
frameSize = size;
- sendOffset = size; // drained; nothing captured yet
width = w;
height = h;
- // Invalidate the change-detect baseline so the first frame always sends
- memset(snapshot, 0xA5, size);
}
- bool changed = memcmp(display->buffer, snapshot, frameSize) != 0;
- if (!changed && !oneShot)
+ if (!firstFrame && !oneShot && memcmp(display->buffer, snapshot, frameSize) == 0)
return;
memcpy(snapshot, display->buffer, frameSize);
- sendOffset = 0;
frameId++;
oneShot = false;
readyId = frameId;
}
- // Notify outside the lock: observers (PhoneAPI) may re-enter hasChunkForPhone.
+ // Notify outside the lock: observers (PhoneAPI) may re-enter hasChunkFor.
frameReady.notifyObservers(readyId);
}
-bool ScreenMirror::hasChunkForPhone()
+bool ScreenMirror::hasChunkFor(uint32_t clientFrameId, uint16_t clientOffset)
{
concurrency::LockGuard g(&lock);
- return snapshot && sendOffset < frameSize;
+ return snapshot && (clientFrameId != frameId || clientOffset < frameSize);
}
-bool ScreenMirror::getChunkForPhone(meshtastic_DisplayFrame &out)
+bool ScreenMirror::copyChunk(uint32_t &clientFrameId, uint16_t &clientOffset, meshtastic_DisplayFrame &out)
{
concurrency::LockGuard g(&lock);
- if (!snapshot || sendOffset >= frameSize)
+ if (!snapshot)
+ return false;
+ if (clientFrameId != frameId) {
+ clientFrameId = frameId;
+ clientOffset = 0;
+ }
+ if (clientOffset >= frameSize)
return false;
- uint16_t len = frameSize - sendOffset;
+ uint16_t len = frameSize - clientOffset;
if (len > sizeof(out.data.bytes))
len = sizeof(out.data.bytes);
@@ -99,11 +115,11 @@ bool ScreenMirror::getChunkForPhone(meshtastic_DisplayFrame &out)
out.height = height;
out.format = meshtastic_DisplayFrame_Format_MONO_VLSB;
out.frame_id = frameId;
- out.offset = sendOffset;
+ out.offset = clientOffset;
out.total_size = frameSize;
out.data.size = len;
- memcpy(out.data.bytes, snapshot + sendOffset, len);
- sendOffset += len;
+ memcpy(out.data.bytes, snapshot + clientOffset, len);
+ clientOffset += len;
return true;
}
diff --git a/src/graphics/ScreenMirror.h b/src/graphics/ScreenMirror.h
index 1ced20f2b5..203e910bfd 100644
--- a/src/graphics/ScreenMirror.h
+++ b/src/graphics/ScreenMirror.h
@@ -1,7 +1,7 @@
#pragma once
#include "configuration.h"
-#if HAS_SCREEN && !defined(MESHTASTIC_EXCLUDE_SCREEN_MIRROR)
+#if HAS_SCREEN_MIRROR
#include "Observer.h"
#include "concurrency/Lock.h"
@@ -13,9 +13,13 @@ namespace graphics
{
/**
- * Streams 1bpp framebuffer snapshots to the local client as
+ * Streams 1bpp framebuffer snapshots to local clients as
* FromRadio.display_frame chunks. Armed via AdminMessage
* set_display_mirror (continuous) or get_display_frame_request (one-shot).
+ *
+ * Holds only the latest captured frame; each PhoneAPI instance keeps its own
+ * (frameId, offset) drain cursor and pulls chunks via copyChunk, so multiple
+ * clients receive complete frames independently.
*/
class ScreenMirror
{
@@ -30,18 +34,23 @@ class ScreenMirror
/// when armed and the contents changed since the last captured frame.
void onRendered(OLEDDisplay *display);
- /// Fills the next pending chunk for the phone; false when drained.
- bool getChunkForPhone(meshtastic_DisplayFrame &out);
- bool hasChunkForPhone();
+ /// True while the client cursor has undelivered bytes of the current frame.
+ bool hasChunkFor(uint32_t clientFrameId, uint16_t clientOffset);
+
+ /// Fills the next chunk for a client cursor, advancing it; false when the
+ /// client is fully caught up (or no frame exists). A frame captured while
+ /// the client was mid-drain restarts it at offset 0 of the new frame.
+ bool copyChunk(uint32_t &clientFrameId, uint16_t &clientOffset, meshtastic_DisplayFrame &out);
private:
+ void freeSnapshotLocked();
+
concurrency::Lock lock;
bool mirroring = false;
bool oneShot = false;
- // Last captured frame; doubles as the change-detection baseline.
+ // Latest captured frame; doubles as the change-detection baseline.
uint8_t *snapshot = nullptr;
uint16_t frameSize = 0;
- uint16_t sendOffset = 0; // == frameSize when drained
uint16_t width = 0;
uint16_t height = 0;
uint32_t frameId = 0;
diff --git a/src/mesh/PhoneAPI.cpp b/src/mesh/PhoneAPI.cpp
index 645a6d65c1..4023478230 100644
--- a/src/mesh/PhoneAPI.cpp
+++ b/src/mesh/PhoneAPI.cpp
@@ -275,7 +275,7 @@ void PhoneAPI::handleStartConfig()
#ifdef FSCom
observe(&xModem.packetReady);
#endif
-#if HAS_SCREEN && !defined(MESHTASTIC_EXCLUDE_SCREEN_MIRROR)
+#if HAS_SCREEN_MIRROR
observe(&graphics::screenMirror.frameReady);
#endif
#ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL
@@ -382,8 +382,14 @@ void PhoneAPI::close()
#ifdef FSCom
unobserve(&xModem.packetReady);
#endif
-#if HAS_SCREEN && !defined(MESHTASTIC_EXCLUDE_SCREEN_MIRROR)
+#if HAS_SCREEN_MIRROR
unobserve(&graphics::screenMirror.frameReady);
+ // This client is gone; PoC keeps one arming flag, so disarm and free
+ // the snapshot rather than stream to nobody. A surviving client
+ // re-arms with another set_display_mirror.
+ graphics::screenMirror.setMirror(false);
+ mirrorFrameId = 0;
+ mirrorOffset = 0;
#endif
releasePhonePacket(); // Don't leak phone packets on shutdown
releaseQueueStatusPhonePacket();
@@ -1063,10 +1069,6 @@ size_t PhoneAPI::getFromRadio(uint8_t *buf)
fromRadioScratch.xmodemPacket = xmodemPacketForPhone;
xmodemPacketForPhone = meshtastic_XModem_init_zero;
}
-#if HAS_SCREEN && !defined(MESHTASTIC_EXCLUDE_SCREEN_MIRROR)
- } else if (graphics::screenMirror.getChunkForPhone(fromRadioScratch.display_frame)) {
- fromRadioScratch.which_payload_variant = meshtastic_FromRadio_display_frame_tag;
-#endif
#ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL
} else if (hasPendingLockdownStatus()) {
concurrency::LockGuard guard(&g_authSlotsMutex);
@@ -1108,6 +1110,12 @@ size_t PhoneAPI::getFromRadio(uint8_t *buf)
fromRadioScratch.which_payload_variant = meshtastic_FromRadio_packet_tag;
fromRadioScratch.packet = replayPkt;
}
+#if HAS_SCREEN_MIRROR
+ } else if (screenMirrorAuthorized() &&
+ graphics::screenMirror.copyChunk(mirrorFrameId, mirrorOffset, fromRadioScratch.display_frame)) {
+ // Lowest priority: mesh traffic and notifications outrank pixels.
+ fromRadioScratch.which_payload_variant = meshtastic_FromRadio_display_frame_tag;
+#endif
}
break;
@@ -1737,11 +1745,6 @@ bool PhoneAPI::available()
}
#endif
-#if HAS_SCREEN && !defined(MESHTASTIC_EXCLUDE_SCREEN_MIRROR)
- if (graphics::screenMirror.hasChunkForPhone())
- return true;
-#endif
-
#ifdef ARCH_ESP32
#if !MESHTASTIC_EXCLUDE_STOREFORWARD
// Check if StoreForward has packets stored for us.
@@ -1757,7 +1760,14 @@ bool PhoneAPI::available()
return true;
// Trailing replay drain - feeds cached satellite-DB packets alongside
// (lower priority than) live traffic.
- return replayPending();
+ if (replayPending())
+ return true;
+
+#if HAS_SCREEN_MIRROR
+ return screenMirrorAuthorized() && graphics::screenMirror.hasChunkFor(mirrorFrameId, mirrorOffset);
+#else
+ return false;
+#endif
}
default:
LOG_ERROR("PhoneAPI::available unexpected state %d", state);
@@ -1944,7 +1954,9 @@ int PhoneAPI::onNotify(uint32_t newValue)
// Consumed by every connected client in this one notify pass, so no per-connection bookkeeping.
if (service->identityMovePending())
state = STATE_RESEND_MY_INFO;
- LOG_INFO("Tell client new packets %u", newValue);
+ // TRACE, not INFO: with display mirroring active this fires once per
+ // captured frame per client, at screen-change rate.
+ LOG_TRACE("Tell client new packets %u", newValue);
onNowHasData(newValue);
} else if (service->identityMovePending() && state != STATE_SEND_NOTHING && state != STATE_SEND_MY_INFO) {
// Mid-sync, so this dump is already carrying the old number in its my_info, its self record or
diff --git a/src/mesh/PhoneAPI.h b/src/mesh/PhoneAPI.h
index 0e68fe9b3c..15176963c6 100644
--- a/src/mesh/PhoneAPI.h
+++ b/src/mesh/PhoneAPI.h
@@ -90,6 +90,24 @@ class PhoneAPI
// file transfer packets destined for phone. Push it to the queue then free it.
meshtastic_XModem xmodemPacketForPhone = meshtastic_XModem_init_zero;
+#if HAS_SCREEN_MIRROR
+ // Per-connection drain cursor into ScreenMirror's current frame, so
+ // coexisting clients (BLE + serial + TCP) each receive complete frames.
+ uint32_t mirrorFrameId = 0;
+ uint16_t mirrorOffset = 0;
+
+ // Screen pixels carry operator content; under access control only an
+ // authorized client may receive them (same rule as mesh packets).
+ bool screenMirrorAuthorized()
+ {
+#ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL
+ return getAdminAuthorized();
+#else
+ return true;
+#endif
+ }
+#endif
+
// Keep QueueStatus packet just as packetForPhone
meshtastic_QueueStatus *queueStatusPacketForPhone = NULL;
diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp
index b9c58e2ae8..e2af8abb14 100644
--- a/src/modules/AdminModule.cpp
+++ b/src/modules/AdminModule.cpp
@@ -696,13 +696,20 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
handleSendInputEvent(r->send_input_event);
break;
}
-#if HAS_SCREEN && !defined(MESHTASTIC_EXCLUDE_SCREEN_MIRROR)
+#if HAS_SCREEN_MIRROR
+ // Both verbs are documented local-connection-only: frames ride FromRadio,
+ // which never crosses the mesh, so honoring a remote arm request would
+ // stream the screen to whatever local client happens to be attached.
case meshtastic_AdminMessage_get_display_frame_request_tag: {
+ if (mp.from != 0)
+ break;
LOG_INFO("Client requests display frame");
graphics::screenMirror.requestFrame();
break;
}
case meshtastic_AdminMessage_set_display_mirror_tag: {
+ if (mp.from != 0)
+ break;
LOG_INFO("Client sets display mirror: %d", r->set_display_mirror);
graphics::screenMirror.setMirror(r->set_display_mirror);
break;