diff --git a/core/src/plc_app/plc_retain.cpp b/core/src/plc_app/plc_retain.cpp index 281a6eab..173146e4 100644 --- a/core/src/plc_app/plc_retain.cpp +++ b/core/src/plc_app/plc_retain.cpp @@ -59,7 +59,18 @@ std::atomic g_active{false}; */ uint8_t retain_write_leaf(uint8_t arr, uint16_t elem, const uint8_t *bytes, uint16_t len) { - return runtime_external_write(arr, elem, (uint8_t)DBGW_OP_WRITE, bytes, len) == 0 ? 0x7E : 0x82; + const int rc = runtime_external_write(arr, elem, (uint8_t)DBGW_OP_WRITE, bytes, len); + + /* Restoring happens once, at program load, before any task is released — + * so apply the write now instead of leaving it for the dispatcher's + * cycle-end drain. Left queued, scan 1 would run on the initial values and + * the restore would then overwrite what scan 1 wrote; and a program with + * more retained leaves than the queue holds would lose the rest. */ + image_lock(); + debug_write_journal_drain(); + image_unlock(); + + return rc == 0 ? 0x7E : 0x82; } /** diff --git a/core/src/plc_app/plc_retain.h b/core/src/plc_app/plc_retain.h index 565f2b99..b6ef3c9a 100644 --- a/core/src/plc_app/plc_retain.h +++ b/core/src/plc_app/plc_retain.h @@ -115,8 +115,12 @@ void plc_retain_init(void); * a machine starting from its defaults is recoverable, one starting from * plausible-looking garbage is not. * + * The values are applied before this returns (not left for the dispatcher's + * cycle-end drain), so scan 1 sees them. + * * Safe and cheap when nothing is retained or no driver claimed the store, and - * idempotent: call once per program start, after plc_retain_init(). + * idempotent: call once per program start, after plc_retain_init() and + * journal_init(), before any task is released. */ void plc_retain_read(void); diff --git a/core/src/plc_app/plc_retain_file_store.cpp b/core/src/plc_app/plc_retain_file_store.cpp index 1e9f1446..3144bea1 100644 --- a/core/src/plc_app/plc_retain_file_store.cpp +++ b/core/src/plc_app/plc_retain_file_store.cpp @@ -11,6 +11,7 @@ #include "plc_retain.h" // PLC_RETAIN_PROGRAM_ID_LEN — one definition for both sides #include +#include #include #include #include @@ -55,7 +56,15 @@ bool g_dirty = false; std::string g_program_md5; std::string g_path; -int g_flush_seconds = 5; +int g_flush_seconds = 10; + +/* When the last commit happened, and whether one has happened since start. A + * change after a quiet period is committed at once; changes that follow within + * g_flush_seconds are held and committed, latest values only, when that period + * ends. So a setpoint is on disk straight away, and a value that keeps changing + * costs at most one write per period. Touched by the flusher thread only. */ +std::chrono::steady_clock::time_point g_last_commit; +bool g_committed_once = false; std::atomic g_enabled{false}; std::atomic g_running{false}; std::thread g_flusher; @@ -77,7 +86,7 @@ void read_config(const char *config_path) { g_enabled.store(false); g_path.clear(); - g_flush_seconds = 5; + g_flush_seconds = 10; FILE *f = fopen(config_path, "r"); if (!f) return; @@ -207,14 +216,17 @@ void discard_stored() void flush_loop() { + g_committed_once = false; while (g_running.load()) { - for (int i = 0; i < g_flush_seconds && g_running.load(); i++) - { - std::this_thread::sleep_for(std::chrono::seconds(1)); - } + /* A tenth of a second: how long a change after a quiet period waits, + * and how promptly a stop is noticed. */ + std::this_thread::sleep_for(std::chrono::milliseconds(100)); if (!g_running.load()) break; + const auto now = std::chrono::steady_clock::now(); + if (g_committed_once && now - g_last_commit < std::chrono::seconds(g_flush_seconds)) continue; + std::vector snapshot; std::string snapshot_id; { @@ -228,7 +240,12 @@ void flush_loop() snapshot_id = g_program_md5; g_dirty = false; } - if (!snapshot.empty()) commit(snapshot.data(), (uint16_t)snapshot.size(), snapshot_id); + if (!snapshot.empty()) + { + commit(snapshot.data(), (uint16_t)snapshot.size(), snapshot_id); + g_last_commit = std::chrono::steady_clock::now(); + g_committed_once = true; + } } } @@ -242,7 +259,8 @@ bool plc_retain_file_store_start(const char *config_path) g_running.store(true); g_flusher = std::thread(flush_loop); - log_info("Retain: built-in file store enabled — %s, flushing every %ds", + log_info("Retain: built-in file store enabled — %s, a change is saved at once, " + "then at most every %ds while values keep changing", g_path.c_str(), g_flush_seconds); return true; } @@ -322,11 +340,40 @@ int plc_retain_file_store_load(const char *program_md5, uint16_t md5_len, uint8_ if (memcmp(stored_id, program_md5, PROGRAM_ID_LEN) != 0) { - fclose(f); - discard_stored(); - log_info("Retain: stored values belong to a different program — storage cleared, " - "retained variables start at their initial values"); - return 0; + /* NOT discarded. Offered upward, and let the layout decide. + * + * PROGRAM_ID is the MD5 of program.st, so it changes on ANY edit — + * move a rung, rename a comment — and discarding here meant every + * retained value in a commissioned plant reset on every upload. That + * is the one guarantee the RETAIN qualifier exists to give: + * IEC 61131-3 §6.5.6.1 rule 1 defines a retained value as "the values + * the variables had when the resource or configuration was stopped", + * conditioned on the STARTING OPERATION being a warm restart and on + * nothing else. The words "download", "reload" and "online change" + * appear nowhere in Part 3. + * + * The check that belongs here already exists one layer up and was + * built for exactly this: ext_strucpp_retain_unpack() validates magic, + * format, crc, TRUNCATION and the LAYOUT HASH before a byte reaches a + * variable, and plc_retain.cpp names which of those failed. STruC++'s + * own debug-table-gen.ts says why it hashes the layout rather than the + * program: "a body edit leaves this unchanged and retained values + * survive, while adding, removing, retyping or reordering a retained + * variable changes it and the stored blob is refused. The project MD5 + * would have discarded retained state on every unrelated edit." + * + * So this gate was not reinforcing that design, it was defeating it — + * and ModBee's ESP32 store (NodeUioRetain.cpp) keeps the identity for + * saves and never compares it on read, which is why retain has been + * seen surviving program changes on real hardware and not here. + * + * WHAT IS GIVEN UP, stated plainly for the review: two DIFFERENT + * projects sharing one retain.bin path whose layout hashes happen to + * collide would now read each other's values. That is a 32-bit + * collision on a path nobody takes, weighed against a certainty on the + * path everybody takes. */ + log_info("Retain: stored values were written by a different program — " + "keeping them; the layout check decides whether they fit"); } const size_t n = fread(out, 1, cap, f); diff --git a/core/src/plc_app/plc_retain_file_store.h b/core/src/plc_app/plc_retain_file_store.h index f79a3814..8daaac79 100644 --- a/core/src/plc_app/plc_retain_file_store.h +++ b/core/src/plc_app/plc_retain_file_store.h @@ -34,11 +34,14 @@ * * enabled=1 * path=/var/lib/openplc-runtime/retain.bin - * flush_seconds=5 + * flush_seconds=10 * - * `flush_seconds` bounds how much retained state a power cut costs, against how - * hard the storage is worked. It is a real trade and it belongs to whoever - * installs the machine, which is why it is configuration and not a constant. + * Nothing is written while nothing changes. A change after a quiet period is + * committed at once; changes that follow within `flush_seconds` are held and + * committed, latest values only, when that period ends. So a setpoint is kept + * straight away and `flush_seconds` bounds how hard a value that keeps changing + * works the storage — and how much of such a value a power cut can cost. It + * belongs to whoever installs the machine, which is why it is configuration. */ #ifndef PLC_RETAIN_FILE_STORE_H diff --git a/core/src/plc_app/plc_state_manager.cpp b/core/src/plc_app/plc_state_manager.cpp index 86e01e16..b9523692 100644 --- a/core/src/plc_app/plc_state_manager.cpp +++ b/core/src/plc_app/plc_state_manager.cpp @@ -443,18 +443,6 @@ void *plc_cycle_thread(void *arg) image_tables_fill_null_pointers(); pthread_mutex_unlock(itm); - /* Retained variables. init() decides once whether retain can run here — - * does the .so export the entry points, does the program retain anything, - * which driver will hold the bytes — and read() asks that driver for what it - * has for THIS program, which is also where a driver discards a previous - * program's values. Both must follow the located-variable binding above: a - * retained variable may also be located, and its image slot has to exist - * before anything writes through it. Both are no-ops when retain is not in - * play, and read() lands before the first task is released, so a new - * program never runs a scan on the old one's state. */ - plc_retain_init(); - plc_retain_read(); - journal_buffer_ptrs_t journal_ptrs = { .bool_input = bool_input, .bool_output = bool_output, @@ -482,6 +470,18 @@ void *plc_cycle_thread(void *arg) log_info("Journal buffer initialized"); } + /* Retained variables. init() decides once whether retain can run here — + * does the .so export the entry points, does the program retain anything, + * which driver will hold the bytes — and read() asks that driver for what it + * has for THIS program, which is also where a driver discards a previous + * program's values. Both must follow the located-variable binding and + * journal_init() above: a retained variable may also be located, its image + * slot has to exist, and its restore goes through the image journal. Both + * are no-ops when retain is not in play. read() applies every value before + * the first task is released, so scan 1 already sees the retained state. */ + plc_retain_init(); + plc_retain_read(); + if (plugin_driver) { plugin_driver_start(plugin_driver); diff --git a/scripts/Makefile.strucpp b/scripts/Makefile.strucpp index 8420111f..f4d66d63 100644 --- a/scripts/Makefile.strucpp +++ b/scripts/Makefile.strucpp @@ -5,11 +5,15 @@ # # - Per-file compilation rules let `make -j` saturate every core on # the target platform (Pi 4 = 4×, x86 servers = 8–32×). -# - `wildcard $(GENERATED_DIR)/*.cpp` discovers however many .cpp -# files STruC++ split into. The codegen emits one TU per POU plus -# a shared configuration.cpp, so the file set varies per project -# — keeping the list out of this file means no Makefile churn -# when POUs are added or removed. +# - Sources are discovered by searching $(GENERATED_DIR), so the +# file set varies per project with no Makefile churn when POUs +# are added or removed. The codegen emits one TU per POU plus a +# shared configuration.cpp. +# - A project can also carry C++ libraries under +# $(GENERATED_DIR)/libraries//, each an ordinary library +# folder with its sources under src/. Every such src/ goes on the +# include path and its sources are compiled, so a block resolves +# `#include ` the same way it does on Arduino. # - `ccache` is picked up automatically when present. Re-running a # build where only one POU's body changed reuses every other .o # from the cache, so incremental builds drop from minutes to a @@ -79,9 +83,48 @@ CXX_BIN := g++ # preserve. Verified on an SLM-RP4: with the flag the .so leaves # /proc//maps on every stop and re-maps fresh on every start; without it, # it never leaves -- one build's image stays pinned for the life of the process. +comma := , + +# Libraries the editor materialised from enabled .stlib archives. Each is an +# ordinary library folder, so `src/` is its include root; a folder without one +# is taken as its own root. +RESOURCE_LIB_DIRS := $(wildcard $(GENERATED_DIR)/libraries/*) +RESOURCE_LIB_INC := $(foreach d,$(RESOURCE_LIB_DIRS),$(if $(wildcard $d/src),$d/src,$d)) + +# A LIBRARY MAY NAME THE DEFINES ITS OWN SOURCES NEED. +# +# `defines=A,B,C` in a library's library.properties becomes -DA -DB -DC. The +# key is ours; arduino-cli ignores keys it does not know, so the same file +# stays valid as an Arduino library descriptor. +# +# WHY IT IS NEEDED, with the case that found it. A vendored C library is very +# often configured by a header it ships - wolfSSL is the example, and its own +# documentation says "the WOLFSSL_USER_SETTINGS macro must be defined +# project-wide", because its wolfcrypt/settings.h reads user_settings.h only +# when that macro is set. Without it every source compiles with DEFAULT +# features instead of the ones the library was vendored for. Nothing fails at +# compile time: the objects are produced, the .so links (a shared object may +# carry undefined symbols), and the failure arrives when the runtime dlopens +# the program - `undefined symbol: wolfSSL_CTX_get_cert_store`, naming a +# function whose absence has no obvious connection to a missing -D. +# +# This is the same gap as the .c discovery above, one layer along: the branch +# taught the build where a vendored library's HEADERS are and then how to +# compile its SOURCES, and a library that needs a define to configure those +# sources still could not say so. +# +# Only from libraries/, and only as -D: a library cannot inject an include +# path, a warning flag or an optimisation level into somebody else's build. +RESOURCE_LIB_DEFS := $(strip $(foreach d,$(RESOURCE_LIB_DIRS),\ + $(if $(wildcard $d/library.properties),\ + $(addprefix -D,$(subst $(comma), ,\ + $(patsubst defines=%,%,\ + $(filter defines=%,$(shell tr -d '\r' < $d/library.properties))))),))) + CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -fno-gnu-unique -MMD -MP \ -Wno-unknown-pragmas -Wno-deprecated-declarations \ - -I $(GENERATED_DIR) -I $(RUNTIME_INC) -I $(PYTHON_INC) + -I $(GENERATED_DIR) -I $(RUNTIME_INC) -I $(PYTHON_INC) \ + $(addprefix -I ,$(RESOURCE_LIB_INC)) $(RESOURCE_LIB_DEFS) # Python POU stubs (`{external …}` blocks emitted by the editor) call # `getpid()`, `create_shm_name()`, `python_block_loader()`. Those @@ -91,6 +134,23 @@ CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -fno-gnu-unique # any Python POU pay nothing; the header is declarations only. GENERATED_CXXFLAGS := $(CXXFLAGS) -include iec_python.h +# And the same for a C source in a vendored library. +# +# Mirrored from CXXFLAGS rather than derived from it, deliberately. The C++ +# flags carry -std=c++17, -fno-gnu-unique, -DSTRUCPP_THREADED, the Python +# include root and a force-include of a C++ header, none of which mean anything +# to a C compiler - and subtracting them would break quietly the next time one +# was added. What has to stay in step is the INCLUDE ROOTS, and those are +# written out here so the next person editing either list can see both. +# +# No -std: the existing python_loader.c rule below does not set one either, and +# a vendored C library that needs a particular dialect says so in its own +# headers rather than having one imposed on it from here. +GENERATED_CFLAGS := -O1 -pipe -fPIC -Wall -MMD -MP \ + -Wno-unknown-pragmas -Wno-deprecated-declarations \ + -I $(GENERATED_DIR) -I $(RUNTIME_INC) \ + $(addprefix -I ,$(RESOURCE_LIB_INC)) $(RESOURCE_LIB_DEFS) + # --------------------------------------------------------------------------- # Retain capability probe. # @@ -133,11 +193,37 @@ SHIM_HAS_RETAIN := $(strip $(if $(wildcard $(RUNTIME_INC)/iec_retain.hpp),\ # the probe just measured, so they are already internally consistent. SHIM_CXXFLAGS := $(CXXFLAGS) $(if $(SHIM_HAS_RETAIN),-DSTRUCPP_SHIM_HAS_RETAIN,) -# Discover every .cpp emitted into core/generated/. STruC++ splits -# across configuration.cpp + one pou_.cpp per POU, but this -# Makefile doesn't care — wildcard adapts. -GEN_CPP := $(wildcard $(GENERATED_DIR)/*.cpp) -GEN_OBJ := $(patsubst $(GENERATED_DIR)/%.cpp,$(BUILD_DIR)/%.o,$(GEN_CPP)) +# Discover every source under core/generated/, at any depth: the top-level TUs +# STruC++ emitted, plus whatever each resource library carries below its own +# root. `sort` makes the order reproducible across filesystems and drops any +# duplicate. +# +# BOTH LANGUAGES, and the .c half was missing. This search found only *.cpp and +# the only pattern rule was for *.cpp, so every C source inside a vendored +# resource library compiled to nothing - and a C library is what a library +# vendors: TLS, compression, codecs and drivers are C almost without exception. +# AND IT DID NOT FAIL LOUDLY, which is the worst part. A shared object is +# allowed to carry undefined symbols, so the link SUCCEEDED and printed "Build +# complete"; the editor reported a clean compile and the failure surfaced later +# as `dlopen: undefined symbol` when the runtime tried to load the program. The +# file had not failed to compile — it had never been offered to a compiler, and +# nothing anywhere said so. +# +# This branch is where that gap came from. RESOURCE_LIB_DIRS and +# RESOURCE_LIB_INC above taught the build where a vendored library's HEADERS +# are; its sources were never added beside them. A vendored C library therefore +# got its headers found and its sources ignored - the same feature, half +# finished. +GEN_CPP := $(sort $(shell find $(GENERATED_DIR) -name '*.cpp')) +GEN_C := $(sort $(shell find $(GENERATED_DIR) -name '*.c')) + +# One `sort` over the union as well as over each half: a library that shipped +# foo.c and foo.cpp side by side would otherwise name $(BUILD_DIR)/foo.o twice. +# Make still has to choose a rule for it, and chooses the .cpp one because it +# is written first - deterministic, and a file layout to avoid rather than one +# to support. +GEN_OBJ := $(sort $(patsubst $(GENERATED_DIR)/%.cpp,$(BUILD_DIR)/%.o,$(GEN_CPP)) \ + $(patsubst $(GENERATED_DIR)/%.c,$(BUILD_DIR)/%.o,$(GEN_C))) SHIM_OBJ := $(BUILD_DIR)/runtime_v4_entry.o PYTHON_OBJ := $(if $(wildcard $(PYTHON_LOADER)),$(BUILD_DIR)/python_loader.o,) @@ -174,10 +260,24 @@ $(LIBPLC): $(GEN_OBJ) $(SHIM_OBJ) $(EXTRA_OBJS) | $(BUILD_DIR) # Generated TUs from the editor's upload. Force-include iec_python.h # so unqualified Python loader symbols in {external} blocks resolve. +# `$(@D)` because a nested source maps to a nested object: a library source at +# libraries/foo/src/transport/bar.cpp builds to +# $(BUILD_DIR)/libraries/foo/src/transport/bar.o. $(BUILD_DIR)/%.o: $(GENERATED_DIR)/%.cpp | $(BUILD_DIR) @echo "[INFO] Compiling $<..." + @mkdir -p $(@D) $(CXX) $(GENERATED_CXXFLAGS) -c $< -o $@ +# The same, for a C source a resource library brought with it. Not the C++ +# compiler with a different flag set: a C library compiled as C++ fails on +# things that are legal C and not legal C++ - an implicit void* conversion, a +# variable called `new`, a designated initialiser out of order - and it fails +# deep inside somebody else's code where it reads as their bug. +$(BUILD_DIR)/%.o: $(GENERATED_DIR)/%.c | $(BUILD_DIR) + @echo "[INFO] Compiling $<..." + @mkdir -p $(@D) + $(CC) $(GENERATED_CFLAGS) -c $< -o $@ + # Static runtime shim — lives in the runtime repo, not in the user # upload. Doesn't get the iec_python.h force-include. # diff --git a/tests/host/test_plc_retain_file_store.cpp b/tests/host/test_plc_retain_file_store.cpp index cd4b3eb9..614a2614 100644 --- a/tests/host/test_plc_retain_file_store.cpp +++ b/tests/host/test_plc_retain_file_store.cpp @@ -16,14 +16,16 @@ * (Baremetal's flash driver DOES carry an explicit length, because its * region is fixed-size and trailing erased bytes read as 0xFF — the two * formats are deliberately not the same, and this test pins this one.); - * - discarding the payload when the identity does not match; + * - KEEPING the payload when the identity does not match, because the + * identity is the program's MD5 and so changes on any edit at all — the + * layout hash one layer up is what decides whether the bytes still fit; * - treating a file too short to carry the header as unattributable; * - holding the identity from load() so the next save() can label its bytes. * * Until this file existed, that path's only evidence was a by-hand run on an - * SLM-RP4 recorded in a PR body. The case it proves — a program's values are - * refused for a DIFFERENT program even when the retain layout is identical — is - * exactly the one a layout hash cannot catch, so it is worth being able to + * SLM-RP4 recorded in a PR body. The case it now proves — that an ordinary + * logic edit does NOT cost a commissioned plant its retained values — is the + * one that used to fail on every single upload, so it is worth being able to * re-run without hardware. * * WHY NOT CEEDLING, AND WHY NOT THE LIFECYCLE HARNESS @@ -49,6 +51,8 @@ #include #include #include +#include +#include #include #include @@ -218,14 +222,37 @@ static void case_same_program_restores() CHECK(memcmp(out, blob, sizeof(blob)) == 0, "the payload should come back unchanged"); } -/* THE CASE A LAYOUT HASH CANNOT CATCH. +/* A PROGRAM EDIT MUST NOT COST THE PLANT ITS RETAINED VALUES. * - * Both programs here retain the same shape — same length, same bytes would pack - * identically — and differ only in identity. Without this check the second - * program silently inherits the first one's state. */ -static void case_different_program_is_discarded() + * This case asserted the opposite until the fix this replaces it for, and the + * reversal is the whole point, so it is worth saying why rather than leaving a + * diff that looks like a check being weakened. + * + * The identity stored here is PROGRAM_ID — the MD5 of program.st — so it + * changes on ANY edit: move a rung, rename a comment, add a line anywhere. + * Discarding on a mismatch therefore meant every retained value in a + * commissioned plant reset on every upload, which is precisely the guarantee + * the RETAIN qualifier exists to give. IEC 61131-3 §6.5.6.1 rule 1 defines a + * retained value as "the values the variables had when the resource or + * configuration was stopped", conditioned on the starting operation being a + * warm restart and on nothing else; "download", "reload" and "online change" + * appear nowhere in Part 3. + * + * And the check that SHOULD refuse a genuinely incompatible blob already + * exists one layer up, in ext_strucpp_retain_unpack(): magic, format, crc, + * truncation and the LAYOUT HASH, with plc_retain.cpp naming which failed. + * STruC++ hashes the layout instead of the program deliberately — a body edit + * leaves it unchanged, while adding, removing, retyping or reordering a + * retained variable changes it and the blob is refused. This store sitting in + * front of that with a project MD5 defeated it rather than reinforcing it. + * + * WHAT IS GIVEN UP: two DIFFERENT projects sharing one retain.bin path whose + * layout hashes happen to collide would read each other's values — a 32-bit + * collision on a path nobody takes, against a certainty on the path everybody + * takes. */ +static void case_different_program_keeps_its_values() { - g_case = "a different program's values are discarded, not inherited"; + g_case = "an edited program still gets its retained values"; reset(); const uint8_t blob[] = {0xDE, 0xAD, 0xBE, 0xEF}; @@ -240,27 +267,30 @@ static void case_different_program_is_discarded() const int rc = plc_retain_file_store_load(MD5_B, PLC_RETAIN_PROGRAM_ID_LEN, out, sizeof(out), &got); - CHECK(rc == 0, "a stale store is not an error — it is an empty one"); - CHECK(got == 0, "NOTHING may be handed back to a different program"); - CHECK(!store_file_exists(), "the stale file should be removed, not left to be re-read"); + CHECK(rc == 0, "a store written by an earlier build is not an error"); + CHECK(got == sizeof(blob), "the bytes must be handed up, for the layout check to judge"); + CHECK(got == sizeof(blob) && memcmp(out, blob, sizeof(blob)) == 0, + "and handed up UNCHANGED — this is the plant's commissioned state"); + CHECK(store_file_exists(), "the file must not be removed on an identity mismatch"); CHECK(g_log.find("different program") != std::string::npos, - "the operator must be told storage was cleared, and why"); + "the operator should still be told the bytes predate this build"); + CHECK(g_log.find("storage cleared") == std::string::npos, + "and must NOT be told they were cleared, because they were not"); - /* And the new program's first commit must be labelled with ITS identity, - * from the id the load above held — otherwise the next start discards it - * too and retention never works for this program. */ + /* The identity still has to be taken from load(), so the next commit + * labels the bytes with the program that is actually running. */ const uint8_t fresh[] = {0x11, 0x22}; plc_retain_file_store_save(fresh, sizeof(fresh)); plc_retain_file_store_flush(); plc_retain_file_store_stop(); FILE *f = fopen(g_store_path.c_str(), "rb"); - CHECK(f != nullptr, "the new program should be able to store"); + CHECK(f != nullptr, "the running program should be able to store"); if (f) { char id[PLC_RETAIN_PROGRAM_ID_LEN]; CHECK(fread(id, 1, sizeof(id), f) == sizeof(id), "header readable"); CHECK(memcmp(id, MD5_B, sizeof(id)) == 0, - "the commit after a discard must carry the NEW program's id"); + "the next commit must carry the RUNNING program's id"); fclose(f); } } @@ -365,6 +395,45 @@ static void case_unchanged_blob_is_not_rewritten() "an unchanged blob should not republish the file (write-and-rename changes the inode)"); } +/* The last `n` bytes of the store file: the blob, which the header precedes. */ +static bool stored_tail_is(const uint8_t *want, size_t n) +{ + FILE *f = fopen(g_store_path.c_str(), "rb"); + if (!f) return false; + uint8_t got[16] = {0}; + fseek(f, -(long)n, SEEK_END); + const size_t r = fread(got, 1, n, f); + fclose(f); + return r == n && memcmp(got, want, n) == 0; +} + +static void case_change_saved_at_once_then_held() +{ + g_case = "a change is saved at once; the next within the period waits for it to end"; + reset(); + write_conf(2); + plc_retain_file_store_start(g_conf_path.c_str()); + uint8_t out[512]; + uint16_t got = 0; + plc_retain_file_store_load(MD5_A, PLC_RETAIN_PROGRAM_ID_LEN, out, sizeof(out), &got); + + const uint8_t a[] = {1, 1, 1, 1}; + const uint8_t b[] = {2, 2, 2, 2}; + + plc_retain_file_store_save(a, sizeof(a)); + std::this_thread::sleep_for(std::chrono::milliseconds(400)); + CHECK(stored_tail_is(a, sizeof(a)), "the first change should be on disk within a moment, not a period"); + + plc_retain_file_store_save(b, sizeof(b)); + std::this_thread::sleep_for(std::chrono::milliseconds(400)); + CHECK(stored_tail_is(a, sizeof(a)), "a second change inside the period should be held"); + + std::this_thread::sleep_for(std::chrono::milliseconds(2000)); + CHECK(stored_tail_is(b, sizeof(b)), "the held change should be saved when the period ends"); + + plc_retain_file_store_stop(); +} + int main() { char tmpl[] = "/tmp/retain-store-test-XXXXXX"; @@ -380,11 +449,12 @@ int main() printf("plc_retain_file_store — identity handling\n"); case_header_layout(); case_same_program_restores(); - case_different_program_is_discarded(); + case_different_program_keeps_its_values(); case_short_file_is_unattributable(); case_empty_store_is_not_an_error(); case_wrong_identity_length_is_refused(); case_unchanged_blob_is_not_rewritten(); + case_change_saved_at_once_then_held(); reset(); rmdir(g_dir.c_str()); diff --git a/tests/pytest/compile/test_analyze_zip_reasons.py b/tests/pytest/compile/test_analyze_zip_reasons.py new file mode 100644 index 00000000..43b5e3b4 --- /dev/null +++ b/tests/pytest/compile/test_analyze_zip_reasons.py @@ -0,0 +1,42 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 Autonomy® + +"""A refused program bundle says why in the build log. + +analyze_zip sent its reasons only to the server logger, so the editor and +the CLI saw the build fail with empty logs and "Compilation failed". Each +refusal now also goes to the build log the status endpoint returns. +""" + +import zipfile + +from webserver import plcapp_management as pm + + +def _zip(tmp_path, name, data): + path = tmp_path / "program.zip" + with zipfile.ZipFile(path, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr(name, data) + return str(path) + + +def test_file_over_the_limit_is_logged(tmp_path, monkeypatch): + monkeypatch.setattr(pm, "MAX_FILE_SIZE", 100) + pm.build_state.logs.clear() + ok, _ = pm.analyze_zip(_zip(tmp_path, "debug-map.json", "x" * 101)) + assert not ok + assert any("debug-map.json" in line and "limit" in line for line in pm.build_state.logs) + + +def test_unsafe_path_is_logged(tmp_path): + pm.build_state.logs.clear() + ok, _ = pm.analyze_zip(_zip(tmp_path, "../evil.c", "int x;")) + assert not ok + assert any("Unsafe path" in line for line in pm.build_state.logs) + + +def test_a_normal_bundle_logs_no_error(tmp_path): + pm.build_state.logs.clear() + ok, _ = pm.analyze_zip(_zip(tmp_path, "program.st", "PROGRAM P END_PROGRAM")) + assert ok + assert not any("[ERROR]" in line for line in pm.build_state.logs) diff --git a/webserver/plcapp_management.py b/webserver/plcapp_management.py index 1b58e351..55cbcb58 100644 --- a/webserver/plcapp_management.py +++ b/webserver/plcapp_management.py @@ -30,8 +30,8 @@ logger, _ = get_logger("runtime", use_buffer=True) -MAX_FILE_SIZE: Final[int] = 10 * 1024 * 1024 # 10 MB per file -MAX_TOTAL_SIZE: Final[int] = 50 * 1024 * 1024 # 50 MB total +MAX_FILE_SIZE: Final[int] = 25 * 1024 * 1024 # 25 MB per file (a large program's debug map passes 10 MB) +MAX_TOTAL_SIZE: Final[int] = 100 * 1024 * 1024 # 100 MB total (debug map + debug table source) DISALLOWED_EXT = (".exe", ".dll", ".sh", ".bat", ".js", ".vbs", ".scr") class BuildStatus(Enum): @@ -81,25 +81,29 @@ def analyze_zip(zip_path) -> tuple[bool, list]: # Check for path traversal or absolute paths if filename.startswith("/") or ".." in filename or ":" in filename: - # logger.warning("Dangerous path: %s", filename) + build_state.log(f"[ERROR] Unsafe path in program file: {filename}\n") safe = False # Check uncompressed size if uncompressed_size > MAX_FILE_SIZE: logger.warning("File too large: %s (%d bytes)", filename, uncompressed_size) + build_state.log( + f"[ERROR] {filename} is {uncompressed_size} bytes; " + f"the limit per file is {MAX_FILE_SIZE} bytes.\n") safe = False # Check compression ratio (ZIP bomb detection) if compressed_size > 0 and uncompressed_size / compressed_size > 1000: - # logger.warning("Suspicious compression ratio in %s", - # filename) + build_state.log( + f"[ERROR] {filename} compresses more than 1000:1; refused.\n") safe = False # Check disallowed extensions if ext in DISALLOWED_EXT: logger.warning("Disallowed extension: %s", filename) + build_state.log(f"[ERROR] File type not allowed: {filename}\n") safe = False total_size += uncompressed_size @@ -107,8 +111,9 @@ def analyze_zip(zip_path) -> tuple[bool, list]: # Check total size if total_size > MAX_TOTAL_SIZE: - # logger.warning("Total uncompressed size too large: %d bytes", - # total_size) + build_state.log( + f"[ERROR] The program files total {total_size} bytes; " + f"the limit is {MAX_TOTAL_SIZE} bytes.\n") safe = False if safe: