From 967fd30ddb84360ac12dea50aceeecf926023031 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Thu, 27 Aug 2026 17:26:49 -0400 Subject: [PATCH] =?UTF-8?q?fix(retain):=20review=20fixes=20=E2=80=94=20buf?= =?UTF-8?q?fer=20boundary,=20clear/save=20race,=20diagnostics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #174. THE BUFFER CAP ADMITTED A SIZE THE API CANNOT EXPRESS ---------------------------------------------------- `RETAIN_BUFFER_MAX` was 64 * 1024 = 65536 while the plugin retain API takes `uint16_t len` / `cap`, whose maximum is 65535. So `needed == 65536` passed the check, `g_active` went true, and then every `(uint16_t)` cast of the length wrapped to 0: the plugin was handed cap/len 0 and retain silently neither saved nor restored — at exactly the one size the error message above it claims is supported. Capped at `UINT16_MAX`. CLEAR COULD BE UNDONE BY AN IN-FLIGHT FLUSH ------------------------------------------- `plc_retain_save()` runs on the PLC cycle thread every scan; `RETAIN:CLEAR` runs on the control-socket thread, and the upload path fires it unconditionally without waiting for the PLC to stop. Two distinct problems came out of that: * In the built-in file store, `g_lock` covers the staging buffer and is released before the write — deliberately, so the scan thread never waits on disk I/O. That left the FILE unguarded: a clear could `remove()` it while a commit sat between its write and its rename, and the rename then republished the blob a moment after it was meant to be gone. The next start restored the PREVIOUS program's values, which is the exact thing clear-on-upload exists to prevent. A separate `g_store_lock` now serialises the file operations, still without holding anything across the scan path. * For a plugin store there was no lock at all, so a vendor's `retain_save` and `retain_clear` could be entered concurrently. Nothing in the plugin contract says they must be reentrant with each other, and for the expected backends — a file, an FRAM page, an NVS partition — that overlap is how a store ends up torn. The driver now serialises its three retain entry points. Held only across the plugin call, which the contract already requires to return promptly, so an uncontended mutex acquire per scan is the whole cost. HALF A STORE IS A MISTAKE, NOT A CONFIGURATION ---------------------------------------------- A plugin exporting `retain_save` but not `retain_load` (or the reverse) was silently treated as "not a store" — indistinguishable from a plugin that never meant to provide one. The two-stores case already logs; this is the same courtesy for the more likely error, which is a typo or an unfinished driver. TESTS ----- 14 pytest cases for `clear_retained()` and `retain_status()`: that a clear never propagates a failure into the upload, that a failure is reported as ERROR rather than OK (the caller logs "cleared"), and that an unreadable or truncated status reply comes back `unknown` rather than being guessed as "no retention configured" — the Persistent Storage screen would otherwise state something it does not know. The review also asked for cases in `tests/test_plugin_driver.c`. I did not add them, and the reason is worth recording: that harness has no build target in CMakeLists, no CI wiring, and `unity.h` is not vendored anywhere in the repo — it cannot compile or run today. Cases added there would look like coverage and be none. The C-side selection logic (first-store-wins, disabled-is-not-a-store, half-a-store-is-not-a-store) is currently proven only on hardware; giving that harness a build is its own piece of work. Verified on an SLM-RP4 after rebuilding: retain still restores across a program reload (a retained TON back at Q=True ET=10s STATE=2 while its un-retained twin restarts at ET=3s200ms), and an upload against a running PLC now leaves the store absent — the clear is honoured and no in-flight flush resurrects it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012FNp61926UEggtmsQPj3A3 --- core/src/drivers/plugin_driver.c | 56 +++++++- core/src/plc_app/plc_retain.cpp | 13 +- core/src/plc_app/plc_retain_file_store.cpp | 22 +++- .../runtimemanager/test_clear_retained.py | 120 ++++++++++++++++++ 4 files changed, 205 insertions(+), 6 deletions(-) create mode 100644 tests/pytest/runtimemanager/test_clear_retained.py diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index c315670..1b511b2 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -25,6 +25,7 @@ #include "plugin_driver.h" #include "vpp_plugin_seal.h" #include +#include #include #include #include @@ -1500,11 +1501,31 @@ static bool plugin_provides_retain_store(const plugin_instance_t *p) // storage plugin, and retain went on claiming to work. if (!p->config.enabled) return false; + if (!p->native_plugin) return false; + // BOTH halves required. A store that can save and not load is worse than // none: it would accept values every scan and silently never give them // back, which looks like working retention right up until the reboot that // matters. - return p->native_plugin && p->native_plugin->retain_save && p->native_plugin->retain_load; + // + // Half a store is a MISTAKE, not a configuration, so say so. Exporting one + // hook and not the other is a typo or an unfinished driver, and treating it + // as "not a store" without a word looks identical to a plugin that never + // meant to provide one — the vendor sees retain quietly not happening and + // has nothing to go on. The two-stores case below already logs; this is the + // same courtesy for the more likely error. + const bool has_save = p->native_plugin->retain_save != NULL; + const bool has_load = p->native_plugin->retain_load != NULL; + if (has_save != has_load) + { + log_warn("Retain: plugin '%s' exports %s but not %s, so it cannot be a retain store — " + "a store must implement both", + p->config.name, + has_save ? "retain_save" : "retain_load", + has_save ? "retain_load" : "retain_save"); + return false; + } + return has_save && has_load; } plugin_instance_t *plugin_driver_find_retain_store(plugin_driver_t *driver) @@ -1532,17 +1553,40 @@ plugin_instance_t *plugin_driver_find_retain_store(plugin_driver_t *driver) return chosen; } +/* + * Serialises the plugin's retain callbacks. + * + * `retain_save` runs on the PLC cycle thread, every scan. `retain_clear` runs + * on the control-socket thread whenever a program is uploaded — and the upload + * path clears unconditionally, without waiting for the PLC to stop. Without + * this lock the two can enter the SAME plugin's C entry points at once, and + * nothing in the plugin contract says a vendor's `retain_save` has to be + * reentrant with its own `retain_clear`. For the expected backends — a file, an + * FRAM page, an NVS partition — that overlap is how a store ends up torn. + * + * Held only across the plugin call, which the contract already requires to + * return promptly, so a per-scan acquire on an uncontended mutex is the whole + * cost. + */ +static pthread_mutex_t retain_call_lock = PTHREAD_MUTEX_INITIALIZER; + int plugin_driver_retain_save(plugin_instance_t *store, const uint8_t *blob, uint16_t len) { if (!plugin_provides_retain_store(store)) return -1; - return store->native_plugin->retain_save(blob, len); + pthread_mutex_lock(&retain_call_lock); + const int rc = store->native_plugin->retain_save(blob, len); + pthread_mutex_unlock(&retain_call_lock); + return rc; } int plugin_driver_retain_load(plugin_instance_t *store, uint8_t *out, uint16_t cap, uint16_t *out_len) { if (out_len) *out_len = 0; if (!plugin_provides_retain_store(store)) return -1; - return store->native_plugin->retain_load(out, cap, out_len); + pthread_mutex_lock(&retain_call_lock); + const int rc = store->native_plugin->retain_load(out, cap, out_len); + pthread_mutex_unlock(&retain_call_lock); + return rc; } int plugin_driver_retain_clear(plugin_instance_t *store) @@ -1551,7 +1595,11 @@ int plugin_driver_retain_clear(plugin_instance_t *store) // reporting that as failure would make the editor's post-upload clear look // broken on every such device. if (!store || !store->native_plugin || !store->native_plugin->retain_clear) return 0; - return store->native_plugin->retain_clear(); + // Same lock as save/load: this is the call that races the scan thread. + pthread_mutex_lock(&retain_call_lock); + const int rc = store->native_plugin->retain_clear(); + pthread_mutex_unlock(&retain_call_lock); + return rc; } void plugin_driver_cycle_start(plugin_driver_t *driver) diff --git a/core/src/plc_app/plc_retain.cpp b/core/src/plc_app/plc_retain.cpp index 059b0fa..22273ce 100644 --- a/core/src/plc_app/plc_retain.cpp +++ b/core/src/plc_app/plc_retain.cpp @@ -9,6 +9,7 @@ #include "plc_retain.h" #include +#include #include #include @@ -34,7 +35,17 @@ namespace { * something to discover on a running machine. A program needing more is * refused at init with a message naming both numbers. */ -constexpr size_t RETAIN_BUFFER_MAX = 64 * 1024; +/* + * UINT16_MAX, not 64 KB. + * + * The plugin retain API takes `uint16_t len` / `cap`, so 65535 is the largest + * blob it can describe. A cap of 65536 admitted exactly one size the API cannot + * express: `needed == 65536` passed the check below, `g_active` went true, and + * then every `(uint16_t)` cast of the length wrapped to 0 — the plugin was + * handed cap/len 0 and retain silently neither saved nor restored, at the one + * size the error message above claims is supported. + */ +constexpr size_t RETAIN_BUFFER_MAX = UINT16_MAX; std::vector g_buffer; std::atomic g_active{false}; diff --git a/core/src/plc_app/plc_retain_file_store.cpp b/core/src/plc_app/plc_retain_file_store.cpp index 1d0b2a8..2b4863c 100644 --- a/core/src/plc_app/plc_retain_file_store.cpp +++ b/core/src/plc_app/plc_retain_file_store.cpp @@ -97,8 +97,23 @@ void read_config(const char *config_path) * and fall back to initial values anyway, but losing the previous values as * well would be gratuitous. */ +/* + * Guards the STORE, not the staging buffer. + * + * `g_lock` covers `g_pending` and is deliberately released before the write, so + * the scan thread never waits on a disk I/O. That leaves the file itself + * unguarded, and two threads reach it: the flusher, and `clear()` on the + * control-socket thread when a program is uploaded. Without this, a clear could + * `remove()` the file while an in-flight commit was between its write and its + * rename — the rename then republished the blob a moment after it was supposed + * to be gone, and the next start restored values from the PREVIOUS program. + */ +std::mutex g_store_lock; + void commit(const uint8_t *buf, uint16_t len) { + std::lock_guard store(g_store_lock); + const std::string tmp = g_path + ".tmp"; FILE *f = fopen(tmp.c_str(), "wb"); @@ -257,9 +272,14 @@ int plc_retain_file_store_clear(void) g_dirty = false; } /* Not gated on `enabled`: a clear has to remove what a PREVIOUS - * configuration stored, which is the whole point of clearing on upload. */ + * configuration stored, which is the whole point of clearing on upload. + * + * Under `g_store_lock`, so a commit that is mid write-and-rename finishes + * first and this removes the file it published — rather than the rename + * landing after the remove and resurrecting the blob. */ if (!g_path.empty()) { + std::lock_guard store(g_store_lock); remove(g_path.c_str()); remove((g_path + ".tmp").c_str()); } diff --git a/tests/pytest/runtimemanager/test_clear_retained.py b/tests/pytest/runtimemanager/test_clear_retained.py new file mode 100644 index 0000000..7485d3c --- /dev/null +++ b/tests/pytest/runtimemanager/test_clear_retained.py @@ -0,0 +1,120 @@ +"""Behavioural tests for `RuntimeManager.clear_retained()` and `retain_status()`. + +`clear_retained` runs on every program upload, and it is the one thing standing +between a new program and the previous program's retained values. Two properties +matter and neither is obvious from reading it: + + * it must never break an upload — the upload is what the user asked for, and a + runtime that cannot be reached has nothing stored that this program will + read anyway; + * it must not report success when it did not happen, because "cleared" is what + the caller writes to the log. +""" + +import socket +import sys +import types +from pathlib import Path + +import pytest + +# `webserver.runtimemanager` pulls in the whole webserver package at import +# time; the socket is the only collaborator these tests care about. +sys.path.insert(0, str(Path(__file__).resolve().parents[3])) + +from webserver.runtimemanager import RuntimeManager # noqa: E402 + + +class FakeSocket: + """Stands in for the runtime control socket.""" + + def __init__(self, reply=None, raises=None): + self.reply = reply + self.raises = raises + self.sent = [] + + def send_and_receive(self, message): + self.sent.append(message) + if self.raises is not None: + raise self.raises + return self.reply + + +def manager(sock): + """A RuntimeManager with only its socket wired — no threads, no daemon.""" + mgr = RuntimeManager.__new__(RuntimeManager) + mgr.runtime_socket = sock + return mgr + + +# --- clear_retained ------------------------------------------------------- + + +def test_sends_the_clear_command(): + sock = FakeSocket(reply="RETAIN:OK\n") + assert manager(sock).clear_retained() == "RETAIN:OK\n" + assert sock.sent == ["RETAIN:CLEAR\n"] + + +@pytest.mark.parametrize( + "failure", + [ + OSError("no such socket"), + socket.error("connection reset"), + RuntimeError("something unexpected"), + ], +) +def test_a_failure_never_propagates_into_the_upload(failure): + # An upload must not fail because retained values could not be discarded. + sock = FakeSocket(raises=failure) + assert manager(sock).clear_retained() == "RETAIN:ERROR\n" + + +def test_a_failure_is_reported_as_error_not_as_ok(): + # The distinction the caller logs. Swallowing the exception AND answering OK + # would say "cleared" about a device that still holds the old values. + sock = FakeSocket(raises=OSError("down")) + assert "ERROR" in manager(sock).clear_retained() + + +# --- retain_status -------------------------------------------------------- + + +def test_reports_the_live_backend(): + sock = FakeSocket(reply="RETAIN:STATUS active plugin synergy\n") + assert manager(sock).retain_status() == { + "active": True, + "backend": "plugin", + "detail": "synergy", + } + + +def test_reports_the_file_backend_with_its_path(): + sock = FakeSocket(reply="RETAIN:STATUS active file /var/lib/openplc-runtime/retain.bin\n") + status = manager(sock).retain_status() + assert status["backend"] == "file" + assert status["detail"] == "/var/lib/openplc-runtime/retain.bin" + + +def test_reports_inactive_without_a_detail(): + sock = FakeSocket(reply="RETAIN:STATUS inactive none\n") + assert manager(sock).retain_status() == {"active": False, "backend": "none", "detail": ""} + + +@pytest.mark.parametrize( + "reply", + ["", "RETAIN:OK\n", "garbage\n", "RETAIN:STATUS active\n", None], +) +def test_an_unreadable_reply_is_unknown_rather_than_a_guess(reply): + # A runtime too old to answer, or a truncated reply, must not be reported as + # "no retention configured" — the Persistent Storage screen would then tell + # the operator something it does not know. + assert manager(FakeSocket(reply=reply)).retain_status()["backend"] == "unknown" + + +def test_an_unreachable_runtime_is_unknown(): + assert manager(FakeSocket(raises=OSError("down"))).retain_status() == { + "active": False, + "backend": "unknown", + "detail": "", + }