Improvements & fixes (#589)

Tactility/Source/service/ServiceRegistration.cpp

@@ -1,5 +1,6 @@
 #include <Tactility/service/ServiceRegistration.h>
 
+#include <Tactility/Mutex.h>
 #include <Tactility/service/ServiceContext.h>
 #include <Tactility/service/ServiceManifest.h>
 
@@ -9,11 +10,35 @@
 
 #include <cassert>
 #include <memory>
+#include <unordered_map>
 
 namespace tt::service {
 
 constexpr auto* TAG = "ServiceRegistration";
 
+namespace {
+
+// Tracks the heap allocations addService() makes per registered id, so removeService()
+// can free them once the kernel confirms the manifest is unregistered. The kernel only
+// ever stores the raw pointer it's handed (see service_manager_add/_remove) - it never
+// takes ownership - so the registering side (us) is responsible for the lifetime.
+struct AllocatedManifest {
+    std::shared_ptr<const ServiceManifest>* persistentManifest;
+    ::ServiceManifest* cManifest;
+};
+
+Mutex& allocatedManifestsMutex() {
+    static Mutex mutex;
+    return mutex;
+}
+
+std::unordered_map<std::string, AllocatedManifest>& allocatedManifests() {
+    static std::unordered_map<std::string, AllocatedManifest> map;
+    return map;
+}
+
+} // namespace
+
 // Bridges the kernel's C ServiceManifest/Service callbacks to the C++ Service
 // instances they wrap. Declared extern "C" to match the linkage of the C
 // function-pointer types they're assigned to (see e.g. gpio_controller.cpp).
@@ -48,9 +73,8 @@ void addService(std::shared_ptr<const ServiceManifest> manifest, bool autoStart)
         return;
     }
 
-    // Intentionally never freed: removeService() only unregisters the manifest
-    // from the kernel, it doesn't own this allocation. Keeps id's backing
-    // string alive for cManifest.id below.
+    // Freed by removeService() once the kernel confirms the manifest is unregistered.
+    // Keeps id's backing string alive for cManifest.id below in the meantime.
     auto* persistentManifest = new std::shared_ptr(manifest);
     auto* cManifest = new ::ServiceManifest {
         .id = (*persistentManifest)->id.c_str(),
@@ -60,6 +84,12 @@ void addService(std::shared_ptr<const ServiceManifest> manifest, bool autoStart)
         .on_stop = cppOnStopTrampoline,
     };
 
+    {
+        auto lock = allocatedManifestsMutex().asScopedLock();
+        lock.lock();
+        allocatedManifests()[id] = AllocatedManifest { persistentManifest, cManifest };
+    }
+
     error_t error = service_manager_add(cManifest, autoStart);
     if (error != ERROR_NONE) {
         LOG_E(TAG, "Failed to add service %s: %s", id.c_str(), error_to_string(error));
@@ -85,6 +115,20 @@ bool removeService(const std::string& id) {
         LOG_E(TAG, "Failed to remove service %s: %s", id.c_str(), error_to_string(error));
         return false;
     }
+
+    // The kernel has confirmed the manifest is unregistered, so id (which points into
+    // persistentManifest's string) is no longer needed by anything - safe to free now.
+    {
+        auto lock = allocatedManifestsMutex().asScopedLock();
+        lock.lock();
+        auto iterator = allocatedManifests().find(id);
+        if (iterator != allocatedManifests().end()) {
+            delete iterator->second.cManifest;
+            delete iterator->second.persistentManifest;
+            allocatedManifests().erase(iterator);
+        }
+    }
+
     LOG_I(TAG, "Removed %s", id.c_str());
     return true;
 }

Tactility/Source/service/development/DevelopmentService.cpp

@@ -157,13 +157,20 @@ esp_err_t DevelopmentService::handleAppInstall(httpd_req_t* request) {
     // Create tmp directory
     const std::string tmp_path = getTempPath();
     if (!file::findOrCreateDirectory(tmp_path, 0777)) {
-        httpd_resp_send_err(request, HTTPD_500_INTERNAL_SERVER_ERROR, "Failed to save file");
+        httpd_resp_send_err(request, HTTPD_500_INTERNAL_SERVER_ERROR, "Failed to create temp path");
         return ESP_FAIL;
     }
 
-    auto file_path = std::format("{}/{}", tmp_path, filename_entry->second);
+    std::string safe_name = file::getLastPathSegment(filename_entry->second);
+    if (safe_name.empty() || safe_name.find("..") != std::string::npos ||
+        safe_name.find('/') != std::string::npos || safe_name.find('\\') != std::string::npos) {
+        httpd_resp_send_err(request, HTTPD_400_BAD_REQUEST, "invalid filename");
+        return ESP_FAIL;
+    }
+    auto file_path = std::format("{}/{}", tmp_path, safe_name);
     if (network::receiveFile(request, file_size, file_path) != file_size) {
-        httpd_resp_send_err(request, HTTPD_500_INTERNAL_SERVER_ERROR, "Failed to save file");
+        file::deleteFile(file_path);
+        httpd_resp_send_err(request, HTTPD_500_INTERNAL_SERVER_ERROR, "Failed to receive file");
         return ESP_FAIL;
     }
 

Tactility/Source/service/gui/GuiService.cpp

@@ -200,6 +200,13 @@ void GuiService::redraw() {
         // Create a default group which adds all objects automatically,
         // and assign all indevs to it.
         // This enables navigation with limited input, such as encoder wheels.
+        // The previous default group (if any) is no longer referenced by anything
+        // after lv_obj_clean() above, so it must be freed here or it leaks.
+        auto* previous_group = lv_group_get_default();
+        if (previous_group != nullptr) {
+            lv_group_delete(previous_group);
+        }
+
         lv_group_t* group = lv_group_create();
         auto* indev = lv_indev_get_next(nullptr);
         while (indev) {
@@ -279,6 +286,17 @@ void GuiService::onStop(ServiceContext& service) {
         lv_group_delete(keyboardGroup);
         keyboardGroup = nullptr;
     }
+
+    auto* default_group = lv_group_get_default();
+    if (default_group != nullptr) {
+        lv_group_delete(default_group);
+        lv_group_set_default(nullptr);
+    }
+
+    auto* screen_root = lv_screen_active();
+    if (screen_root != nullptr) {
+        lv_obj_clean(screen_root);
+    }
     lvgl_unlock();
 
     delete thread;

Tactility/Source/service/loader/Loader.cpp

@@ -10,12 +10,8 @@
 
 #include <vector>
 
-#ifdef ESP_PLATFORM
-#include <esp_heap_caps.h>
-#include <utility>
-#endif
-
 #include <tactility/log.h>
+#include <tactility/memory.h>
 
 namespace tt::service::loader {
 
@@ -72,6 +68,8 @@ void LoaderService::onStartAppMessage(const std::string& id, app::LaunchId launc
     appStack.push_back(new_app);
     transitionAppToState(new_app, app::State::Created);
     transitionAppToState(new_app, app::State::Showing);
+
+    memory_trace();
 }
 
 void LoaderService::onStopTopAppMessage(const std::string& id) {
@@ -125,10 +123,6 @@ void LoaderService::onStopTopAppMessage(const std::string& id) {
         LOG_W(TAG, "Memory leak: Stopped %s, but use count is %d", app_to_stop->getManifest().appId.c_str(), (int)(app_to_stop->getApp().use_count() - 2));
     }
 
-#ifdef ESP_PLATFORM
-    LOG_I(TAG, "Free heap: %d", (int)heap_caps_get_free_size(MALLOC_CAP_INTERNAL));
-#endif
-
     std::shared_ptr<app::AppInstance> instance_to_resume;
     // If there's a previous app, resume it
     if (!appStack.empty()) {
@@ -167,6 +161,8 @@ void LoaderService::onStopTopAppMessage(const std::string& id) {
             );
         }
     }
+
+    memory_trace();
 }
 
 int LoaderService::findAppInStack(const std::string& id) const {