From 5e86a20e8b29dee71713e6b7f848598c2be206a1 Mon Sep 17 00:00:00 2001 From: Thomas Faour Date: Sun, 19 Jul 2026 15:20:26 -0400 Subject: [PATCH] Fix config read-modify-write race and two firmware buffer edge cases Found by a thorough code review: - server/app/config.py's load()/save() each locked only their own file I/O, not the full read-modify-write cycle each route does around them. Since uvicorn dispatches sync routes to a thread pool, two concurrent requests (e.g. the device's own poll landing alongside a web UI edit) could each load() the same on-disk state and the second's save() silently clobber the first's changes. Added config.locked() (backed by an RLock, since load()/save() also take the lock internally) and wrapped every mutating route's load/mutate/save span in it -- kept outside the lock wherever a route also does slow Immich network I/O, re-loading fresh state right before the actual mutation instead. Verified with a new concurrency stress test (many concurrent /api/queue/promote and /api/config calls) alongside the existing scratch suite. - firmware/main/root.html's SSID/password/toolsserver/access-token inputs had no maxlength, so pasting something longer than the matching NVS buffer (wifi_provisioning.h's FRAME_CFG_*_MAX_LEN) was silently truncated with no indication why the device later can't connect or gets 401s. - frame_client.c's share_url buffer (256 bytes) could be too small in the worst case -- toolsserver (128) + "/frame/share/" + asset_id (47) + "?token=" + access_token (64) can reach ~266 bytes, silently dropping the token off a request that would then just 401 with no obvious cause. Widened to 320. --- firmware/main/frame_client.c | 9 +++- firmware/main/root.html | 12 +++-- server/app/config.py | 21 +++++++- server/app/main.py | 97 +++++++++++++++++++++--------------- 4 files changed, 91 insertions(+), 48 deletions(-) diff --git a/firmware/main/frame_client.c b/firmware/main/frame_client.c index 8cef606..754e209 100644 --- a/firmware/main/frame_client.c +++ b/firmware/main/frame_client.c @@ -608,7 +608,14 @@ static esp_err_t show_menu_level(const frame_config_t *cfg, fetch_action_t actio char location_line1[32]; char location_line2[32]; char taken_at[32]; - char share_url[256]; + /* Wider than the other URL buffers in this file: unlike a fixed path, + * this one stacks toolsserver (up to 128) + "/frame/share/" + an + * asset_id (up to 47) + "?token=" + an access_token (up to 64) -- + * worst case ~266 bytes, which a 256-byte buffer could silently + * truncate the token off of (build_url()'s bounds check avoids an + * overflow, but a truncated/dropped token still means the resulting + * request just 401s with no obvious cause). */ + char share_url[320]; fetch_photo_info(cfg, location_line1, sizeof(location_line1), location_line2, sizeof(location_line2), taken_at, sizeof(taken_at), share_url, sizeof(share_url)); diff --git a/firmware/main/root.html b/firmware/main/root.html index 589ebe9..42c7a08 100644 --- a/firmware/main/root.html +++ b/firmware/main/root.html @@ -76,26 +76,30 @@

Please connect me to your local wifi network

+
- +
- +
- +
- +
diff --git a/server/app/config.py b/server/app/config.py index e082342..af02b27 100644 --- a/server/app/config.py +++ b/server/app/config.py @@ -5,14 +5,18 @@ from __future__ import annotations import json import os +from contextlib import contextmanager from pathlib import Path -from threading import Lock +from threading import RLock +from typing import Iterator from pydantic import BaseModel CONFIG_PATH = Path(os.environ.get("CONFIG_PATH", "/data/config.json")) -_lock = Lock() +# Reentrant so load()/save() can each take it internally for their own I/O +# while a caller also holds it for a whole locked() span (see below). +_lock = RLock() class FrameConfig(BaseModel): @@ -63,3 +67,16 @@ def save(cfg: FrameConfig) -> None: with _lock: CONFIG_PATH.parent.mkdir(parents=True, exist_ok=True) CONFIG_PATH.write_text(cfg.model_dump_json(indent=2)) + + +@contextmanager +def locked() -> Iterator[None]: + """Serializes an entire load-mutate-save cycle. load()/save() each + only lock their own I/O, which isn't enough by itself: uvicorn + dispatches sync routes to a thread pool, so two concurrent requests + (e.g. the device's own poll landing alongside a web UI edit) can each + load() the same on-disk state and the second save() silently clobber + the first's changes. Route handlers that mutate config should wrap + their whole load/mutate/save span in this.""" + with _lock: + yield diff --git a/server/app/main.py b/server/app/main.py index 4ac6021..c78e31b 100644 --- a/server/app/main.py +++ b/server/app/main.py @@ -122,21 +122,22 @@ def api_config_save( # docker-compose.yml.example) -- config.load() already applies them, # and this handler doesn't touch cfg.immich_url/immich_api_key at all, # so there's nothing here that could overwrite or clear them. - cfg = config.load() - if album_id != cfg.album_id: - # A newly selected album starts clean -- the old current photo and - # queue don't mean anything in the new album's context. - cfg.current_asset_id = "" - cfg.current_asset_set_at = 0.0 - cfg.queue = [] - cfg.queue_cursor = 0 - cfg.history = [] - cfg.album_id = album_id - cfg.order = order if order in ("sequential", "shuffle") else "sequential" - cfg.refresh_interval_s = max(MIN_REFRESH_INTERVAL_S, min(MAX_REFRESH_INTERVAL_S, refresh_interval_s)) - cfg.smart_crop_faces = smart_crop_faces - cfg.queue_target_len = max(MIN_QUEUE_TARGET_LEN, min(MAX_QUEUE_TARGET_LEN, queue_target_len)) - config.save(cfg) + with config.locked(): + cfg = config.load() + if album_id != cfg.album_id: + # A newly selected album starts clean -- the old current photo and + # queue don't mean anything in the new album's context. + cfg.current_asset_id = "" + cfg.current_asset_set_at = 0.0 + cfg.queue = [] + cfg.queue_cursor = 0 + cfg.history = [] + cfg.album_id = album_id + cfg.order = order if order in ("sequential", "shuffle") else "sequential" + cfg.refresh_interval_s = max(MIN_REFRESH_INTERVAL_S, min(MAX_REFRESH_INTERVAL_S, refresh_interval_s)) + cfg.smart_crop_faces = smart_crop_faces + cfg.queue_target_len = max(MIN_QUEUE_TARGET_LEN, min(MAX_QUEUE_TARGET_LEN, queue_target_len)) + config.save(cfg) return {"status": "saved"} @@ -189,8 +190,10 @@ def frame_image(): client = ImmichClient(cfg.immich_url, cfg.immich_api_key) assets = _list_assets(client, cfg) - if photo_queue.get_current(cfg, assets): - config.save(cfg) + with config.locked(): + cfg = config.load() # re-read: state may have changed since the unlocked read above + if photo_queue.get_current(cfg, assets): + config.save(cfg) return Response(content=_render_asset(client, cfg, cfg.current_asset_id), media_type="application/octet-stream") @@ -206,8 +209,10 @@ def frame_advance(): client = ImmichClient(cfg.immich_url, cfg.immich_api_key) assets = _list_assets(client, cfg) - photo_queue.advance_forced(cfg, assets) - config.save(cfg) + with config.locked(): + cfg = config.load() # re-read: state may have changed since the unlocked read above + photo_queue.advance_forced(cfg, assets) + config.save(cfg) return Response(content=_render_asset(client, cfg, cfg.current_asset_id), media_type="application/octet-stream") @@ -226,8 +231,10 @@ def frame_back(): client = ImmichClient(cfg.immich_url, cfg.immich_api_key) assets = _list_assets(client, cfg) - photo_queue.back_forced(cfg, assets) - config.save(cfg) + with config.locked(): + cfg = config.load() # re-read: state may have changed since the unlocked read above + photo_queue.back_forced(cfg, assets) + config.save(cfg) return Response(content=_render_asset(client, cfg, cfg.current_asset_id), media_type="application/octet-stream") @@ -314,8 +321,10 @@ def frame_photo_info(): client = ImmichClient(cfg.immich_url, cfg.immich_api_key) assets = _list_assets(client, cfg) - if photo_queue.get_current(cfg, assets): - config.save(cfg) + with config.locked(): + cfg = config.load() # re-read: state may have changed since the unlocked read above + if photo_queue.get_current(cfg, assets): + config.save(cfg) if not cfg.current_asset_id: raise HTTPException(404, "No current photo") @@ -379,8 +388,10 @@ def frame_face_labels(): client = ImmichClient(cfg.immich_url, cfg.immich_api_key) assets = _list_assets(client, cfg) - if photo_queue.get_current(cfg, assets): - config.save(cfg) + with config.locked(): + cfg = config.load() # re-read: state may have changed since the unlocked read above + if photo_queue.get_current(cfg, assets): + config.save(cfg) if not cfg.current_asset_id: return {"count": 0} @@ -418,11 +429,13 @@ def api_queue(): client = ImmichClient(cfg.immich_url, cfg.immich_api_key) assets = _list_assets(client, cfg) - current_changed = photo_queue.get_current(cfg, assets) - queue_before = list(cfg.queue) - photo_queue.sync_queue_length(cfg, assets) - if current_changed or cfg.queue != queue_before: - config.save(cfg) + with config.locked(): + cfg = config.load() # re-read: state may have changed since the unlocked read above + current_changed = photo_queue.get_current(cfg, assets) + queue_before = list(cfg.queue) + photo_queue.sync_queue_length(cfg, assets) + if current_changed or cfg.queue != queue_before: + config.save(cfg) def entry(asset_id: str) -> dict: return {"id": asset_id, "thumbnail_url": f"/api/photo-thumbnail/{asset_id}"} @@ -445,12 +458,13 @@ def api_queue_reorder(body: QueueReorderRequest): the client sent that's no longer actually queued is dropped, and any ID the server has that the client didn't know about is appended rather than lost.""" - cfg = config.load() - current_set = set(cfg.queue) - reordered = [asset_id for asset_id in body.queue if asset_id in current_set] - reordered += [asset_id for asset_id in cfg.queue if asset_id not in set(reordered)] - cfg.queue = reordered - config.save(cfg) + with config.locked(): + cfg = config.load() + current_set = set(cfg.queue) + reordered = [asset_id for asset_id in body.queue if asset_id in current_set] + reordered += [asset_id for asset_id in cfg.queue if asset_id not in set(reordered)] + cfg.queue = reordered + config.save(cfg) return {"status": "saved"} @@ -465,11 +479,12 @@ def api_queue_promote(body: QueuePromoteRequest): client supplying a full, exactly-current snapshot of the queue at all, so it can't fail due to the queue having shifted server-side since the browser's last fetch.""" - cfg = config.load() - if body.asset_id not in cfg.queue: - raise HTTPException(400, "That photo is no longer in the upcoming queue") - cfg.queue = [body.asset_id] + [asset_id for asset_id in cfg.queue if asset_id != body.asset_id] - config.save(cfg) + with config.locked(): + cfg = config.load() + if body.asset_id not in cfg.queue: + raise HTTPException(400, "That photo is no longer in the upcoming queue") + cfg.queue = [body.asset_id] + [asset_id for asset_id in cfg.queue if asset_id != body.asset_id] + config.save(cfg) return {"status": "saved"}