Refactor app loading and window management - #609
Conversation
📝 WalkthroughWalkthroughAdded application modules for manifest parsing, archive installation, runtime loading, scheduling, lifecycle management, and app events. Added an LVGL window manager with stacked windows and state notifications. Extended system events with polling subscriptions and renamed callback APIs. Updated system-event callers and tests. Added module build configuration and public module declarations. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
Modules/lvgl-window-manager/source/window_manager.cpp-79-84 (1)
79-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor the
window_manager_configure()lifecycle contract.The header states that this call has no effect after startup. This implementation replaces
s.screen_initwhile started. The replacement becomes observable after the next stop/start cycle.Return without changing
s.screen_initwhen the manager is started. Coordinate this with a starting lifecycle state so a concurrent start cannot bypass the pre-start requirement.TactilityKernel/include/tactility/system_event.h-139-140 (1)
139-140: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPermit callers to read event output fields.
The warning prohibits reading
timestamp,data, anddata_len. These fields contain the result ofsystem_event_await(). The tests also readdataanddata_len.Document
task,sequence,consumed_sequence, andnextas internal. Permit reads of the event output fields after a successful await.Modules/app-module/source/app_scheduler.cpp-119-164 (1)
119-164: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
tskNO_AFFINITYfor the app thread affinity.
thread_set_affinity(thread, affinity)stores the raw affinity value, so this passes-1toxTaskCreatePinnedToCoreon ESP-IDF instead of the port’s documented no-affinity value. Keep the-1as the default/compatibility comment if needed, but passtskNO_AFFINITYwhen creating the task.Modules/app-module/source/event.cpp-96-114 (1)
96-114: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRoute app event notifications to a reserved notification index.
Current usages include polling subscriptions, lvTask, libTask waiting tasks, and the USB host task. They call the same non-indexed notification APIs or
ulTaskNotifyTake(pdTRUE, 0)and would have their notification credit cleared byapp_event_awaiton a shared index 0 slot. UsexTaskNotifyGiveIndexed/ulTaskNotifyTakeIndexedorulTaskNotifyTakeIndexed(pdFALSE, 0)with an app-only index.Modules/app-module/source/app_install.cpp-237-238 (1)
237-238: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe lock comment states the inverse of the real contract.
uninstall_locked()never takesinstall_registry().mutex. Both callers take it first:app_install()at line 329 andapp_uninstall()at line 379. The_lockedsuffix also implies the caller holds the lock.A future caller that follows this comment either deadlocks or mutates the registry unlocked.
📝 Proposed fix
-// Takes install_registry().mutex - caller must not already hold it. +// Caller must already hold install_registry().mutex. error_t uninstall_locked(const std::string& app_id) {Modules/app-module/include/app/manager.h-36-42 (1)
36-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the malformed sentence and widen the documented constraint.
Lines 38-39 read "Safe to call app_manager_add()/_remove() from within
@avisitor is NOT guaranteed", which is not a grammatical sentence.The constraint is also narrower than the implementation allows.
app_manager_for_each_manifest()inmanager.cpprunsvisitorwhile it holds the ledger mutex. Anyapp_manager_*call from the visitor can deadlock, not onlyapp_manager_add()/app_manager_remove().📝 Proposed doc fix
- * Calls `@a` visitor once for every registered manifest (e.g. for AppList/Settings to enumerate - * apps to show). Iteration order is unspecified. Safe to call app_manager_add()/_remove() from - * within `@a` visitor is NOT guaranteed - do not mutate the registry from inside the callback. + * Calls `@a` visitor once for every registered manifest (e.g. for AppList/Settings to enumerate + * apps to show). Iteration order is unspecified. + * `@warning` `@a` visitor runs with app-module's internal registry lock held. Do not call any + * app_manager_*() function from inside `@a` visitor - copy out what you need and act on it after + * this call returns.Modules/app-module/source/manager.cpp-124-126 (1)
124-126: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse a kernel-provided tick conversion for the stop timeout.
app_scheduler_stop()expectsTickType_t;TactilityKernel/include/tactility/freertos/task.hdefines FreeRTOS task primitives but does not definepdMS_TO_TICKS. If the FreeRTOS include chain changes, this call can fail. Define the 2000 ms value in milliseconds and convert it with FreeRTOS’s tick conversion helper or the local constant used by the FreeRTOS wrapper.Modules/app-module/include/app/module.h-4-8 (1)
4-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude the Module definition before exposing
app_module.
TactilityKernel/include/tactility/module.hdefinesstruct Module.Modules/app-module/include/app/module.h,Modules/app-esp32-module/include/app_esp32/module.h,Modules/gps-module/include/gps/module.h, andModules/lvgl-module/include/lvgl/module.hdeclareextern struct Module foo_module;without including it, whileModules/crypt-module/include/crypt/module.hincludestactility/module.hbefore its declaration. IncludeTactilityKernel/include/tactility/module.hin the headers that need the fullModuletype, or change this forward declaration into the same style used by the other leaf module headers if the incomplete type is intentional.
🧹 Nitpick comments (7)
Modules/app-module/private/app/private/app_scheduler.h (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
<stdint.h>in a header that declares C linkage.The file guards declarations with
extern "C", which signals C consumers.<cstdint>compiles only in C++. Use<stdint.h>to keep the header usable from C, and to matchinclude/app/event.h.♻️ Proposed change
-#include <cstdint> +#include <stdint.h>Modules/app-module/source/app_internal_loader.cpp (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
externonServiceManifestdefinitions in both loader services. Anexterndeclaration with an initializer is a definition, so the keyword has no effect and compilers can warn about it. The same pattern was copied into both modules. Declare each manifest in a header and define it withoutextern.
Modules/app-module/source/app_internal_loader.cpp#L42-L48: removeexternfrom theapp_internal_loader_service_manifestdefinition.Modules/app-esp32-module/source/app_esp32_loader_service.cpp#L135-L141: removeexternfrom theloader_service_manifestdefinition.Modules/app-module/include/app/location.h (1)
1-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the SPDX header and document the enum values.
Two small gaps in this new header:
- Line 1 has no
// SPDX-License-Identifier: Apache-2.0. Every other new header in this module has it.- Line 14 points readers to
AppLocationType, but that enum documents nothing. State whatlocationholds for each type.📝 Proposed fix
+// SPDX-License-Identifier: Apache-2.0 `#pragma` once `#ifdef` __cplusplus extern "C" { `#endif` enum AppLocationType { + /** `location` points at an in-memory app image. */ APP_LOCATION_MEMORY, + /** `location` is a NULL-terminated path to the app's install directory. */ APP_LOCATION_PATH, };Modules/app-module/include/app/metadata.h (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the SPDX license identifier.
Every other new file in this change set starts with
// SPDX-License-Identifier: Apache-2.0. This header omits it.📄 Proposed fix
+// SPDX-License-Identifier: Apache-2.0 `#pragma` onceModules/app-module/private/app/private/app_metadata_parsing_internal.h (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude
<cstddef>and qualifystd::size_t.
size_tappears unqualified in the global namespace.<map>and<string>only guaranteestd::size_t. Mainstream implementations also declare::size_t, so this compiles today, but the include is not guaranteed.♻️ Proposed change
+#include <cstddef> `#include` <map> `#include` <string>-bool app_metadata_copy_bounded(char* dest, size_t dest_size, const std::string& value); +bool app_metadata_copy_bounded(char* dest, std::size_t dest_size, const std::string& value);Modules/app-module/source/app_metadata_parsing_v2.cpp (1)
10-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared parse routine driven by a key table.
app_metadata_parse_v2andapp_metadata_parse_v1differ only in the six property key strings. Every future metadata field needs the same edit in both files. A single routine that accepts a struct of key names removes the duplication.The internal header documents that the per-format split mirrors the previous
tt::appparser, so this is optional.Modules/app-esp32-module/source/module.cpp (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefix the exported manifest symbol with the module name.
loader_service_manifestis a global symbol with C linkage. Other modules in the image can export the same generic name, which causes a duplicate-symbol link error.Modules/app-module/source/module.cppuses the prefixed nameapp_internal_loader_service_manifest. Rename this symbol and its definition toapp_esp32_loader_service_manifest.♻️ Proposed change
-extern ServiceManifest loader_service_manifest; +extern ServiceManifest app_esp32_loader_service_manifest; static error_t start() { - return service_manager_add(&loader_service_manifest, /*auto_start=*/true); + return service_manager_add(&app_esp32_loader_service_manifest, /*auto_start=*/true); } static error_t stop() { - return service_manager_remove(loader_service_manifest.id); + return service_manager_remove(app_esp32_loader_service_manifest.id); }Rename the definition in
Modules/app-esp32-module/source/app_esp32_loader_service.cppto match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d42aa53d-a21f-4126-9334-befd7c40d4df
📒 Files selected for processing (42)
Devices/lilygo-tlora-pager/source/module.cppDocumentation/ideas.mdModules/app-esp32-module/CMakeLists.txtModules/app-esp32-module/devicetree.yamlModules/app-esp32-module/include/app_esp32/module.hModules/app-esp32-module/source/app_esp32_loader_service.cppModules/app-esp32-module/source/module.cppModules/app-module/CMakeLists.txtModules/app-module/devicetree.yamlModules/app-module/include/app/event.hModules/app-module/include/app/install.hModules/app-module/include/app/instance.hModules/app-module/include/app/loader.hModules/app-module/include/app/location.hModules/app-module/include/app/manager.hModules/app-module/include/app/manifest.hModules/app-module/include/app/metadata.hModules/app-module/include/app/module.hModules/app-module/private/app/private/app_ledger.hModules/app-module/private/app/private/app_metadata_parsing_internal.hModules/app-module/private/app/private/app_scheduler.hModules/app-module/source/app_install.cppModules/app-module/source/app_internal_loader.cppModules/app-module/source/app_metadata_parsing.cppModules/app-module/source/app_metadata_parsing_v1.cppModules/app-module/source/app_metadata_parsing_v2.cppModules/app-module/source/app_scheduler.cppModules/app-module/source/event.cppModules/app-module/source/manager.cppModules/app-module/source/module.cppModules/lvgl-window-manager/CMakeLists.txtModules/lvgl-window-manager/devicetree.yamlModules/lvgl-window-manager/include/lvgl_window_manager/module.hModules/lvgl-window-manager/include/lvgl_window_manager/window_manager.hModules/lvgl-window-manager/source/module.cppModules/lvgl-window-manager/source/window_manager.cppTactility/Source/lvgl/Statusbar.cppTactility/Source/service/rtctime/RtcTimeService.cppTactility/Source/service/wifi/Wifi.cppTactilityKernel/include/tactility/system_event.hTactilityKernel/source/system_event.cppTests/TactilityKernel/Source/SystemEventTest.cpp
| #include "../../../TactilityKernel/include/tactility/error.h" | ||
| #include "../../../TactilityKernel/include/tactility/filesystem/file_mutex.h" | ||
| #include "../../app-module/include/app/loader.h" | ||
| #include "../../app-module/include/app/location.h" | ||
|
|
||
|
|
||
| #include <app/loader.h> | ||
| #include <app/manifest.h> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the deep relative includes.
Lines 2-5 reach into sibling modules and into TactilityKernel by relative path. Lines 8-9 already include the same app-module headers through the module include paths. The relative form bypasses the module dependency declarations and breaks if the directory layout changes. Keep only the angle-bracket includes, and declare the required modules in CMakeLists.txt and devicetree.yaml.
♻️ Proposed change
-#include "../../../TactilityKernel/include/tactility/error.h"
-#include "../../../TactilityKernel/include/tactility/filesystem/file_mutex.h"
-#include "../../app-module/include/app/loader.h"
-#include "../../app-module/include/app/location.h"
-
-
`#include` <app/loader.h>
+#include <app/location.h>
`#include` <app/manifest.h>
+
+#include <tactility/error.h>
+#include <tactility/filesystem/file_mutex.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include "../../../TactilityKernel/include/tactility/error.h" | |
| #include "../../../TactilityKernel/include/tactility/filesystem/file_mutex.h" | |
| #include "../../app-module/include/app/loader.h" | |
| #include "../../app-module/include/app/location.h" | |
| #include <app/loader.h> | |
| #include <app/manifest.h> | |
| `#include` <app/loader.h> | |
| `#include` <app/location.h> | |
| `#include` <app/manifest.h> | |
| `#include` <tactility/error.h> | |
| `#include` <tactility/filesystem/file_mutex.h> |
| error_t api_load(AppLocation location, AppRuntime* out_runtime) { | ||
| auto* runtime = new (std::nothrow) Esp32AppRuntime(); | ||
| if (runtime == nullptr) { | ||
| return ERROR_OUT_OF_MEMORY; | ||
| } | ||
|
|
||
| if (location.type != APP_LOCATION_PATH) { | ||
| return ERROR_NOT_SUPPORTED; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix the memory leak on the unsupported-location path.
api_load allocates runtime at Line 72 and then returns at Line 78 without deleting it. Every call with a non-path location leaks an Esp32AppRuntime. Validate location.type before the allocation.
🐛 Proposed fix
error_t api_load(AppLocation location, AppRuntime* out_runtime) {
+ if (location.type != APP_LOCATION_PATH) {
+ return ERROR_NOT_SUPPORTED;
+ }
+
auto* runtime = new (std::nothrow) Esp32AppRuntime();
if (runtime == nullptr) {
return ERROR_OUT_OF_MEMORY;
}
- if (location.type != APP_LOCATION_PATH) {
- return ERROR_NOT_SUPPORTED;
- }
-
size_t size = 0;| struct AppMetadata { | ||
|
|
||
| /** | ||
| * The SDK version that was used to compile this app. (e.g. "0.6.0") | ||
| * Must be NULL-terminated. | ||
| */ | ||
| char target_sdk[APP_METADATA_TARGET_SDK_LENGTH + 1]; | ||
|
|
||
| /** | ||
| * The identifier by which the app is launched by the system and other apps. | ||
| * Must be NULL-terminated. | ||
| */ | ||
| char app_id[APP_METADATA_APP_ID_LENGTH + 1]; | ||
|
|
||
| /** | ||
| * The user-readable name of the app. Used in UI. | ||
| * Must be NULL-terminated. | ||
| */ | ||
| char app_name[APP_METADATA_APP_NAME_LENGTH + 1]; | ||
|
|
||
| /** | ||
| * The version as it is displayed to the user (e.g. "1.2.0") | ||
| * Must be NULL-terminated. | ||
| */ | ||
| char app_version_name[APP_METADATA_APP_VERSION_NAME_LENGTH + 1]; | ||
|
|
||
| /** The technical version (must be incremented with new releases of the app */ | ||
| uint64_t app_version_code = 0; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find C translation units and headers that include app/metadata.h
rg -n --glob '*.c' --glob '*.h' 'app/metadata\.h' .
# List C sources in the module directories that may pull it in transitively
fd -e c . Modules | head -50Repository: TactilityProject/Tactility
Length of output: 1621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'Modules/app-module/include/app/metadata.h' 'Modules/app-module/private/app/private/app_metadata_parsing_internal.h' 'Modules/app-module/source/app_install.cpp' 'Modules/lvgl-module/source/arch/lvgl_esp32.c' 'Modules/lvgl-module/source/arch/lvgl_posix.c' | sort
echo
echo "== app/metadata.h around struct and guards =="
cat -n Modules/app-module/include/app/metadata.h | sed -n '1,90p'
echo
echo "== app_metadata_parsing_internal.h =="
cat -n Modules/app-module/private/app/private/app_metadata_parsing_internal.h | sed -n '1,80p'
echo
echo "== manifest header for comparison =="
fd -e h -t f 'manifest\.h$' Modules | while read -r f; do echo "--- $f"; cat -n "$f" | sed -n '1,80p'; done
echo
echo "== install.h relevant include/use hints =="
cat -n Modules/app-module/include/app/install.h | sed -n '1,80p'
echo
echo "== app_install metadata usages =="
rg -n "AppMetadata|metadata " Modules/app-module/source/app_install.cpp || true
echo
echo "== C files including metadata.h or parsing internal header =="
rg -n --glob '*.c' 'app/metadata\.h|private/app/private/app_metadata_parsing_internal\.h' .
echo
echo "== all includes from lvgl C sources of headers mentioning AppMetadata or app metadata =="
rg -n --glob '*.c' 'metadata|AppMetadata|manifest' Modules/lvgl-module/source Modules/lvgl-module/source-fonts || true
echo
echo "== structural parser guard/header check =="
python3 - <<'PY'
from pathlib import Path
p=Path('Modules/app-module/include/app/metadata.h')
text=p.read_text()
print("contains_extern_C=", '" C"' in text or 'extern "C"' in text)
print("contains_default_initializer=", '= 0' in text)
print("uses_fixed_width_uint=", 'uint64_t' in text)
print("lines= ", len(text.splitlines()))
PYRepository: TactilityProject/Tactility
Length of output: 11264
Remove the default member initializer to keep AppMetadata C-compatible.
app/metadata.h exposes struct AppMetadata through extern "C" declarations, so C translations must accept the struct definition. uint64_t app_version_code = 0; is a C++ default member initializer and makes this header non-C. AppMetadata callers already use aggregate zero-initialization, so the initializer is not needed.
🔧 Proposed fix
/** The technical version (must be incremented with new releases of the app */
- uint64_t app_version_code = 0;
+ uint64_t app_version_code;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct AppMetadata { | |
| /** | |
| * The SDK version that was used to compile this app. (e.g. "0.6.0") | |
| * Must be NULL-terminated. | |
| */ | |
| char target_sdk[APP_METADATA_TARGET_SDK_LENGTH + 1]; | |
| /** | |
| * The identifier by which the app is launched by the system and other apps. | |
| * Must be NULL-terminated. | |
| */ | |
| char app_id[APP_METADATA_APP_ID_LENGTH + 1]; | |
| /** | |
| * The user-readable name of the app. Used in UI. | |
| * Must be NULL-terminated. | |
| */ | |
| char app_name[APP_METADATA_APP_NAME_LENGTH + 1]; | |
| /** | |
| * The version as it is displayed to the user (e.g. "1.2.0") | |
| * Must be NULL-terminated. | |
| */ | |
| char app_version_name[APP_METADATA_APP_VERSION_NAME_LENGTH + 1]; | |
| /** The technical version (must be incremented with new releases of the app */ | |
| uint64_t app_version_code = 0; | |
| }; | |
| struct AppMetadata { | |
| /** | |
| * The SDK version that was used to compile this app. (e.g. "0.6.0") | |
| * Must be NULL-terminated. | |
| */ | |
| char target_sdk[APP_METADATA_TARGET_SDK_LENGTH + 1]; | |
| /** | |
| * The identifier by which the app is launched by the system and other apps. | |
| * Must be NULL-terminated. | |
| */ | |
| char app_id[APP_METADATA_APP_ID_LENGTH + 1]; | |
| /** | |
| * The user-readable name of the app. Used in UI. | |
| * Must be NULL-terminated. | |
| */ | |
| char app_name[APP_METADATA_APP_NAME_LENGTH + 1]; | |
| /** | |
| * The version as it is displayed to the user (e.g. "1.2.0") | |
| * Must be NULL-terminated. | |
| */ | |
| char app_version_name[APP_METADATA_APP_VERSION_NAME_LENGTH + 1]; | |
| /** The technical version (must be incremented with new releases of the app */ | |
| uint64_t app_version_code; | |
| }; |
| bool untar_file(minitar* archive, const minitar_entry* entry, const std::string& destination_path) { | ||
| auto absolute_path = destination_path + "/" + entry->metadata.path; | ||
| if (!ensure_directory_recursive(destination_path)) { | ||
| LOG_E(TAG, "Can't find or create directory %s", destination_path.c_str()); | ||
| return false; | ||
| } | ||
|
|
||
| if (!minitar_read_contents_to_file(archive, entry, absolute_path.c_str())) { | ||
| LOG_E(TAG, "Failed to write data to %s", absolute_path.c_str()); | ||
| return false; | ||
| } | ||
|
|
||
| // Note: fchmod() doesn't exist on ESP-IDF and chmod() does nothing on that platform. | ||
| chmod(absolute_path.c_str(), entry->metadata.mode); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
app_install() treats archive-supplied strings as trusted filesystem paths. app_install() accepts an arbitrary tarball, so both the tar entry paths and the manifest's app_id are untrusted input. Both reach the filesystem without validation, so a crafted archive can write to and delete paths outside the app install directory. Add one validation rule and apply it at both sinks.
Modules/app-module/source/app_install.cpp#L137-L153: reject anyentry->metadata.paththat is absolute or contains a..component before you buildabsolute_path, and apply the same check inuntar_directory(). Also create the parent directory ofabsolute_path, not justdestination_path.Modules/app-module/source/app_install.cpp#L333-L336: reject an emptymetadata.app_id, or one that contains a path separator or.., beforeuninstall_locked()and before you buildfinal_pathfordelete_recursively()andrename().
📍 Affects 1 file
Modules/app-module/source/app_install.cpp#L137-L153(this comment)Modules/app-module/source/app_install.cpp#L333-L336
| uninstall_locked(metadata.app_id); | ||
|
|
||
| auto final_path = app_parent_path + "/" + metadata.app_id; | ||
| delete_recursively(final_path); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate metadata.app_id before you use it as a directory name.
metadata.app_id comes from manifest.properties inside the untrusted tarball. Line 335 concatenates it straight into final_path, and line 336 then runs delete_recursively(final_path).
If app_id contains / or .., final_path resolves outside the app install directory. delete_recursively() then deletes an arbitrary directory tree, and rename() on line 339 installs the app outside its sandbox.
Reject any app_id that is empty or contains a path separator, .., or a NUL-adjacent surprise, before line 333.
🔒️ Proposed fix
+ // app_id is untrusted (it comes from the archive's manifest) and becomes a directory name.
+ if (metadata.app_id[0] == '\0' || std::strpbrk(metadata.app_id, "/\\") != nullptr || std::strstr(metadata.app_id, "..") != nullptr) {
+ LOG_E(TAG, "Invalid app id in manifest");
+ delete_recursively(staging_path);
+ return ERROR_INVALID_ARGUMENT;
+ }
+
auto& registry = install_registry();
mutex_lock(®istry.mutex);
// Replace any previous install of this app id (mirrors the old install()'s "already
// running/present" handling).
uninstall_locked(metadata.app_id);| s.waiting_task = xTaskGetCurrentTaskHandle(); | ||
| mutex_unlock(&s.mutex); | ||
|
|
||
| ulTaskNotifyTake(pdTRUE, timeout); | ||
|
|
||
| return window_manager_get_state(id); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear the waiter registration after a timeout.
When ulTaskNotifyTake() times out, s.waiting_task still points to the caller. A later create or remove can notify a task that is no longer waiting. If that task has exited, the manager uses a stale task handle. If it waits again, the stale notification can cause an immediate return.
After the wait, clear s.waiting_task under s.mutex only when it still equals the current task handle. Handle the notification-versus-timeout race before returning the state.
| std::memcpy(sub->data, data, std::min(data_len, static_cast<size_t>(SYSTEM_EVENT_MAX_DATA_SIZE))); | ||
| } | ||
| sub->data_len = data_len; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Store the copied payload length.
Line 85 truncates the copy to SYSTEM_EVENT_MAX_DATA_SIZE. Line 87 stores the original data_len. A consumer can then read past sub->data when it trusts sub->data_len.
Store the bounded copy length, or reject oversized payloads before notification.
Proposed fix
- if (data_len > 0) {
- std::memcpy(sub->data, data, std::min(data_len, static_cast<size_t>(SYSTEM_EVENT_MAX_DATA_SIZE)));
+ const size_t copied_len = std::min(data_len, static_cast<size_t>(SYSTEM_EVENT_MAX_DATA_SIZE));
+ if (copied_len > 0) {
+ std::memcpy(sub->data, data, copied_len);
}
- sub->data_len = data_len;
+ sub->data_len = copied_len;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::memcpy(sub->data, data, std::min(data_len, static_cast<size_t>(SYSTEM_EVENT_MAX_DATA_SIZE))); | |
| } | |
| sub->data_len = data_len; | |
| const size_t copied_len = std::min(data_len, static_cast<size_t>(SYSTEM_EVENT_MAX_DATA_SIZE)); | |
| if (copied_len > 0) { | |
| std::memcpy(sub->data, data, copied_len); | |
| } | |
| sub->data_len = copied_len; |
| } | ||
| sub->data_len = data_len; | ||
| sub->sequence++; | ||
| xTaskNotifyGive(sub->task); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect direct-notification usage and FreeRTOS notification-array configuration.
rg -n -C 3 'xTaskNotifyGive|ulTaskNotifyTake|xTaskNotifyGiveIndexed|ulTaskNotifyTakeIndexed' .
fd -i 'FreeRTOSConfig.*' . -x rg -n 'configTASK_NOTIFICATION_ARRAY_ENTRIES' {}
rg -n -C 3 '\bsystem_event_(subscribe|await|unsubscribe)\s*\(' .Repository: TactilityProject/Tactility
Length of output: 32797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== system_event.cpp relevant section =="
sed -n '1,230p' TactilityKernel/source/system_event.cpp | cat -n
echo
echo "== configTASK_NOTIFICATION_ARRAY_ENTRIES occurrences =="
rg -n -C 2 'configTASK_NOTIFICATION_ARRAY_ENTRIES' .
echo
echo "== notify_poll_subscribers callers =="
rg -n -C 4 'notify_poll_subscribers|system_event_notify' TactilityKernel/source/system_event.cpp .Repository: TactilityProject/Tactility
Length of output: 26712
Isolate wakeups per poll subscription.
All poll subscriptions registered by one task share the default FreeRTOS direct-notification index. xTaskNotifyGive(sub->task) wakes index 0, so an event for subscription A can wake system_event_await(sub) for subscription B. ulTaskNotifyTake(pdTRUE, timeout) consumes that shared notification before the per-subscription sequence check.
Use a per-subscription synchronization object, or document and enforce one poll subscription per task.
| mutex_lock(&poll_subscriptions_mutex.handle); | ||
| sub->next = poll_subscriptions; | ||
| poll_subscriptions = sub; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject duplicate registration of the same subscription.
Registering sub a second time links the node into a list that already contains it. This creates a cycle. The next notify_poll_subscribers() traversal then does not terminate while holding poll_subscriptions_mutex.
Check whether sub is already linked before insertion. Return an error when it is already registered.
| TEST_CASE("system_event_emit passes the data pointer and length through unchanged") { | ||
| reset_calls(); | ||
| int context_a = 1; | ||
| struct Payload { int value; } payload { 42 }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate Payload declaration.
This scope declares struct Payload { int value; } payload { 42 }; twice. The test target does not compile because Payload and payload are redeclared.
1 participant
Summary by CodeRabbit