Fixes

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++;
     }

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
 )

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);
 

Modules/lvgl-window-manager-module/source/window_manager.cpp

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 #include <lvgl_window_manager/window_manager.h>
 
-#include "../../app-module/include/app/instance.h"
+#include <app/instance.h>
 
 #include <lvgl/lvgl.h>
 

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

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
+ * genuine error prevented opening or reading it, e.g. a permissions error (a missing file is
- * starts out empty in that case)
+ * not an error - the instance starts out empty in that case)
  */
 PropertiesFile* properties_file_open(const char* path);
 

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
  */

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
+    clone->entries = bundle->entries;
-    // 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;
-    }
     return clone;
 }
 

TactilityKernel/source/properties_file.cpp

@@ -3,6 +3,7 @@
 #include <tactility/filesystem/file_mutex.h>
 #include <tactility/log.h>
 
+#include <cerrno>
 #include <cstdint>
 #include <cstdio>
 #include <cstring>
@@ -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
+// @return false if the file exists but a genuine I/O error interrupted opening or reading it
-// EOF return doesn't by itself distinguish clean end-of-file from a read error - ferror() after
+// (fgetc()'s EOF return doesn't by itself distinguish clean end-of-file from a read error -
-// the loop does); true otherwise, including for a missing file.
+// 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;

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
+    // Reset under the lock, together, as the last step - only past this point is `sub` safe
-    // pre-cancelled (subscribe() also resets this itself, defensively).
+    // 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;
 }

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);

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);
+    mkdir(dir, 0700);
-    chmod(dir, 0777);
+    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];

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);