From a2056e8e96dba2292f2b3a563931453efca6d730 Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Sat, 8 Aug 2026 18:51:59 +0200 Subject: [PATCH] Fixes --- Modules/app-module/source/app_scheduler.cpp | 7 +++-- .../lvgl-window-manager-module/CMakeLists.txt | 2 +- .../lvgl_window_manager/window_manager.h | 8 ++++++ .../source/window_manager.cpp | 2 +- Tactility/CMakeLists.txt | 2 +- .../include/tactility/properties_file.h | 4 +-- .../include/tactility/system_event.h | 19 ++++++++++++- TactilityKernel/source/bundle.cpp | 12 +------- TactilityKernel/source/properties_file.cpp | 14 +++++++--- TactilityKernel/source/system_event.cpp | 28 +++++++++++++++++-- .../Source/PreferencesTest.cpp | 1 + .../Source/PropertiesFileTest.cpp | 12 ++++++-- .../Source/SystemEventTest.cpp | 4 +-- 13 files changed, 85 insertions(+), 30 deletions(-) diff --git a/Modules/app-module/source/app_scheduler.cpp b/Modules/app-module/source/app_scheduler.cpp index 948222e04..fa7f76238 100644 --- a/Modules/app-module/source/app_scheduler.cpp +++ b/Modules/app-module/source/app_scheduler.cpp @@ -71,13 +71,16 @@ void set_completion(AppInstanceId app_instance_id, AppCompletionSignal* completi // Takes a reference on app_instance_id's completion signal (see AppCompletionSignal), for the // caller to wait on. @return the signal to wait on, or NULL if the instance has already fully // finished (its ledger entry - and so its reference to the signal - is already gone) and so -// there's nothing left to wait for. +// there's nothing left to wait for, or if the instance is still starting up (start_internal() +// in manager.cpp inserts the ledger entry before app_scheduler_start() has gotten as far as +// set_completion() - `completion` is NULL for that whole window) and so there's nothing to +// take a reference on yet. AppCompletionSignal* acquire_completion_signal(AppInstanceId app_instance_id) { auto& ledger = app_ledger(); mutex_lock(&ledger.mutex); auto iterator = ledger.instances.find(app_instance_id); AppCompletionSignal* completion = nullptr; - if (iterator != ledger.instances.end()) { + if (iterator != ledger.instances.end() && iterator->second.completion != nullptr) { completion = iterator->second.completion; completion->refcount++; } diff --git a/Modules/lvgl-window-manager-module/CMakeLists.txt b/Modules/lvgl-window-manager-module/CMakeLists.txt index d8e518d56..79c6d56ec 100644 --- a/Modules/lvgl-window-manager-module/CMakeLists.txt +++ b/Modules/lvgl-window-manager-module/CMakeLists.txt @@ -7,5 +7,5 @@ file(GLOB_RECURSE SOURCE_FILES "source/*.c*") tactility_add_module(lvgl-window-manager-module SRCS ${SOURCE_FILES} INCLUDE_DIRS include/ - REQUIRES TactilityKernel lvgl-module + REQUIRES TactilityKernel lvgl-module app-module ) diff --git a/Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h b/Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h index 7a5c243e0..957be5a5c 100644 --- a/Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h +++ b/Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h @@ -36,6 +36,10 @@ enum WindowState { * @return the widget windows should actually be placed into - @a root_widget itself, or a * child of it. Returning NULL falls back to @a root_widget. * @warning Called on the LVGL task with the LVGL lock already held. + * @warning Also called with window-manager's internal lifecycle_mutex held (non-recursive) - + * do NOT call window_manager_start()/window_manager_stop()/window_manager_create()/ + * window_manager_remove() or any other window-manager API from this callback, that would + * deadlock. */ typedef lv_obj_t* (*WindowManagerScreenInitFn)(lv_obj_t* root_widget); @@ -76,6 +80,10 @@ error_t window_manager_stop(void); * window_manager_remove() for the window that used to be on top (e.g. a dialog's own thread as * it closes). Do NOT rely on thread_local state set by this window's own app thread; use * @a user_data instead. + * @warning Also called with window-manager's internal lifecycle_mutex held (non-recursive) - + * do NOT call window_manager_start()/window_manager_stop()/window_manager_create()/ + * window_manager_remove() or any other window-manager API from this callback, that would + * deadlock. */ typedef void (*WindowCreateWidgetsFn)(lv_obj_t* root, void* user_data); diff --git a/Modules/lvgl-window-manager-module/source/window_manager.cpp b/Modules/lvgl-window-manager-module/source/window_manager.cpp index f182f2149..94da9f451 100644 --- a/Modules/lvgl-window-manager-module/source/window_manager.cpp +++ b/Modules/lvgl-window-manager-module/source/window_manager.cpp @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include "../../app-module/include/app/instance.h" +#include #include diff --git a/Tactility/CMakeLists.txt b/Tactility/CMakeLists.txt index 5a6897538..3de858ac9 100644 --- a/Tactility/CMakeLists.txt +++ b/Tactility/CMakeLists.txt @@ -10,7 +10,6 @@ list(APPEND REQUIRES_LIST lvgl-module lvgl-window-manager-module app-module - app-esp32-module crypt-module gps-module gps-generic-module @@ -23,6 +22,7 @@ list(APPEND REQUIRES_LIST if (DEFINED ENV{ESP_IDF_VERSION}) list(APPEND REQUIRES_LIST + app-esp32-module platform-esp32 driver elf_loader diff --git a/TactilityKernel/include/tactility/properties_file.h b/TactilityKernel/include/tactility/properties_file.h index 2df582f3a..3c65ebc98 100644 --- a/TactilityKernel/include/tactility/properties_file.h +++ b/TactilityKernel/include/tactility/properties_file.h @@ -27,8 +27,8 @@ typedef struct PropertiesFile PropertiesFile; * @param[in] path absolute or relative file path (e.g. "/data/settings.properties") - the * parent directory must already exist * @return the new instance, or NULL on allocation failure, or NULL if @a path exists but a - * genuine I/O error interrupted reading it (a missing file is not an error - the instance - * starts out empty in that case) + * genuine error prevented opening or reading it, e.g. a permissions error (a missing file is + * not an error - the instance starts out empty in that case) */ PropertiesFile* properties_file_open(const char* path); diff --git a/TactilityKernel/include/tactility/system_event.h b/TactilityKernel/include/tactility/system_event.h index 2d5356f1c..8ff46cd2f 100644 --- a/TactilityKernel/include/tactility/system_event.h +++ b/TactilityKernel/include/tactility/system_event.h @@ -168,8 +168,19 @@ struct SystemEventSubscription { int waiter_count; /** Set by system_event_unsubscribe() before it gives `semaphore` and waits, so a task * already blocked in system_event_await() bails out (ERROR_INVALID_STATE) instead of - * waiting out its full timeout. Reset on the next system_event_subscribe(). */ + * waiting out its full timeout. Reset once system_event_unsubscribe() finishes + * draining old awaiters (see unsubscribe_in_progress) - not simply "on the next + * system_event_subscribe()", so a fresh registration can never observe a stale `true` + * left over from an unsubscribe that hasn't returned yet. */ bool cancelled; + /** True from the moment system_event_unsubscribe() unlinks `sub` until it has finished + * draining old awaiters and deleted the old semaphore. system_event_subscribe() spins + * until this clears before reusing `sub` - otherwise a new registration could reset + * waiter_count/cancelled (both shared with the old registration, there being only one + * `sub`) out from under the old system_event_unsubscribe() call still relying on them, + * or hand out a new semaphore for that same call to then promptly delete instead of the + * old one, while an old awaiter is still blocked on the real old semaphore. */ + bool unsubscribe_in_progress; struct SystemEventSubscription* next; } internal; @@ -178,6 +189,10 @@ struct SystemEventSubscription { /** * Register a poll subscription for events of @a sub->type. * @warning Does not work in ISR context. + * @warning If @a sub was just passed to system_event_unsubscribe() (e.g. reusing a node for a + * new registration) and that call hasn't returned yet on another task, this call blocks + * (briefly - not for the full duration of anyone's timeout) until it does, before registering - + * see SystemEventSubscription::internal.unsubscribe_in_progress. * @param[in,out] sub subscription to register; caller sets @a sub->type beforehand, owns the * storage, and must keep it alive (and stationary) until unsubscribed * @retval ERROR_NONE on success @@ -194,6 +209,8 @@ error_t system_event_subscribe(struct SystemEventSubscription* sub); * currently blocked in system_event_await() on @a sub has woken up and left, so it's safe to * delete the subscription's semaphore before this call returns. A blocked awaiter is woken * (with ERROR_INVALID_STATE) as part of this call rather than left to time out on its own. + * A concurrent system_event_subscribe() reusing the same @a sub waits out this same window + * (see system_event_subscribe()'s @warning) rather than racing it. * @param[in] sub subscription to remove, as passed to system_event_subscribe() * @return ERROR_NONE on success, ERROR_NOT_FOUND if no matching subscription exists */ diff --git a/TactilityKernel/source/bundle.cpp b/TactilityKernel/source/bundle.cpp index 20f0eff82..9ca06175c 100644 --- a/TactilityKernel/source/bundle.cpp +++ b/TactilityKernel/source/bundle.cpp @@ -44,17 +44,7 @@ Bundle* bundle_clone(const Bundle* bundle) { if (clone == nullptr) { return nullptr; } - // The Bundle allocation above is nothrow, but copy-assigning `entries` (allocating a node - // and copying the key/value_string for every entry) is not - std::bad_alloc could still - // escape mid-copy. Callers only ever check for a NULL return, so convert that into the - // documented nullptr-on-failure contract instead of letting it propagate out of this - // extern "C" function (which would be undefined behavior). - try { - clone->entries = bundle->entries; - } catch (...) { - delete clone; - return nullptr; - } + clone->entries = bundle->entries; return clone; } diff --git a/TactilityKernel/source/properties_file.cpp b/TactilityKernel/source/properties_file.cpp index c2203bab5..5194b7d22 100644 --- a/TactilityKernel/source/properties_file.cpp +++ b/TactilityKernel/source/properties_file.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -48,9 +49,9 @@ namespace { // close(). Mirrors Tactility's loadPropertiesFile(): "#"-prefixed and blank lines are skipped; // a "[section]" line becomes a literal prefix (verbatim, brackets included) prepended to every // subsequent key, until the next "[section]" line replaces it. -// @return false if the file exists but a genuine I/O error interrupted reading it (fgetc()'s -// EOF return doesn't by itself distinguish clean end-of-file from a read error - ferror() after -// the loop does); true otherwise, including for a missing file. +// @return false if the file exists but a genuine I/O error interrupted opening or reading it +// (fgetc()'s EOF return doesn't by itself distinguish clean end-of-file from a read error - +// ferror() after the loop does); true otherwise, including for a missing file (ENOENT). bool load_from_file(PropertiesFile* file) { FileMutex mutex {}; file_mutex_get(&mutex, file->path.c_str()); @@ -58,8 +59,13 @@ bool load_from_file(PropertiesFile* file) { FILE* handle = std::fopen(file->path.c_str(), "r"); if (handle == nullptr) { + const int open_error = errno; file_mutex_unlock(&mutex); - return true; + if (open_error == ENOENT) { + return true; + } + LOG_E(TAG, "Failed to open %s", file->path.c_str()); + return false; } std::string key_prefix; diff --git a/TactilityKernel/source/system_event.cpp b/TactilityKernel/source/system_event.cpp index 4207482ac..73c07ffac 100644 --- a/TactilityKernel/source/system_event.cpp +++ b/TactilityKernel/source/system_event.cpp @@ -166,6 +166,23 @@ error_t system_event_emit( } error_t system_event_subscribe(SystemEventSubscription* sub) { + // Wait out any system_event_unsubscribe() call still draining old awaiters for this same + // `sub` on another task (see internal.unsubscribe_in_progress). waiter_count/cancelled + // belong to `sub` itself, not to a given registration - reusing `sub` before that call + // finishes would reset them out from under it, and could hand out a fresh semaphore for it + // to then promptly delete instead of the old one, while an old awaiter is still blocked on + // the real old semaphore. + while (true) { + mutex_lock(&poll_subscriptions_mutex.handle); + bool busy = sub->internal.unsubscribe_in_progress; + mutex_unlock(&poll_subscriptions_mutex.handle); + + if (!busy) { + break; + } + delay_ticks(pdMS_TO_TICKS(10)); + } + SemaphoreHandle_t semaphore = xSemaphoreCreateBinary(); if (semaphore == nullptr) { return ERROR_OUT_OF_MEMORY; @@ -218,6 +235,9 @@ error_t system_event_unsubscribe(SystemEventSubscription* sub) { // this point would overwrite that field with a freshly created semaphore, and we must // not delete the wrong (newly active) one. sub->internal.cancelled = true; + // Blocks a concurrent system_event_subscribe() from reusing `sub` until this whole + // call returns - see internal.unsubscribe_in_progress and system_event_subscribe(). + sub->internal.unsubscribe_in_progress = true; semaphore_to_delete = sub->internal.semaphore; sub->internal.semaphore = nullptr; } @@ -246,9 +266,13 @@ error_t system_event_unsubscribe(SystemEventSubscription* sub) { vSemaphoreDelete(semaphore_to_delete); - // Reset so a future system_event_subscribe() re-registering this same `sub` isn't left - // pre-cancelled (subscribe() also resets this itself, defensively). + // Reset under the lock, together, as the last step - only past this point is `sub` safe + // for system_event_subscribe() to reuse (see internal.unsubscribe_in_progress and the + // busy-wait at the top of system_event_subscribe()). + mutex_lock(&poll_subscriptions_mutex.handle); sub->internal.cancelled = false; + sub->internal.unsubscribe_in_progress = false; + mutex_unlock(&poll_subscriptions_mutex.handle); return ERROR_NONE; } diff --git a/Tests/TactilityKernel/Source/PreferencesTest.cpp b/Tests/TactilityKernel/Source/PreferencesTest.cpp index 635c7afd4..0478f440a 100644 --- a/Tests/TactilityKernel/Source/PreferencesTest.cpp +++ b/Tests/TactilityKernel/Source/PreferencesTest.cpp @@ -231,6 +231,7 @@ TEST_CASE("preferences_open creates missing parent directories (recursively) and CHECK(file_exists(nested_path)); Preferences* reopened = preferences_open(nested_path); + REQUIRE_NE(reopened, nullptr); int32_t out = 0; CHECK(preferences_opt_int32(reopened, "count", &out)); CHECK_EQ(out, 7); diff --git a/Tests/TactilityKernel/Source/PropertiesFileTest.cpp b/Tests/TactilityKernel/Source/PropertiesFileTest.cpp index 520b9b47d..e2b4fde40 100644 --- a/Tests/TactilityKernel/Source/PropertiesFileTest.cpp +++ b/Tests/TactilityKernel/Source/PropertiesFileTest.cpp @@ -193,11 +193,17 @@ TEST_CASE("properties_file_close reports ERROR_RESOURCE when the parent director } TEST_CASE("a failed close leaves previously-saved content on disk untouched") { + if (geteuid() == 0) { + // Root bypasses directory write permissions, so the read-only directory below would + // not make save_to_file() fail. + return; + } + const char* dir = "/tmp/tactility_kernel_properties_file_readonly_test"; const char* path = "/tmp/tactility_kernel_properties_file_readonly_test/settings.properties"; - mkdir(dir, 0777); - chmod(dir, 0777); + mkdir(dir, 0700); + chmod(dir, 0700); std::remove(path); { @@ -214,7 +220,7 @@ TEST_CASE("a failed close leaves previously-saved content on disk untouched") { properties_file_set(file, "key", "corrupted"); CHECK_EQ(properties_file_close(file), ERROR_RESOURCE); - chmod(dir, 0777); // restore write access for the check below and for cleanup + chmod(dir, 0700); // restore write access for the check below and for cleanup PropertiesFile* reloaded = properties_file_open(path); char buffer[32]; diff --git a/Tests/TactilityKernel/Source/SystemEventTest.cpp b/Tests/TactilityKernel/Source/SystemEventTest.cpp index 750c66cbc..e5a267fe1 100644 --- a/Tests/TactilityKernel/Source/SystemEventTest.cpp +++ b/Tests/TactilityKernel/Source/SystemEventTest.cpp @@ -399,7 +399,7 @@ TEST_CASE("system_event_unsubscribe wakes a task blocked in system_event_await w CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NONE); CHECK_LT(get_ticks() - before, pdMS_TO_TICKS(1000)); - CHECK_EQ(thread_join(thread, 2, 1), ERROR_NONE); + CHECK_EQ(thread_join(thread, pdMS_TO_TICKS(2000), pdMS_TO_TICKS(1)), ERROR_NONE); CHECK_EQ(thread_get_return_code(thread), ERROR_INVALID_STATE); thread_free(thread); @@ -432,7 +432,7 @@ TEST_CASE("a subscription node can be re-subscribed after system_event_unsubscri ); CHECK_EQ(thread_start(thread), ERROR_NONE); CHECK_EQ(system_event_await(&sub, pdMS_TO_TICKS(2000)), ERROR_NONE); - CHECK_EQ(thread_join(thread, 2, 1), ERROR_NONE); + CHECK_EQ(thread_join(thread, pdMS_TO_TICKS(2000), pdMS_TO_TICKS(1)), ERROR_NONE); thread_free(thread); CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NONE);