App loading improvements and more

[KenVanHoeylandt]

• External apps can opt into automatic cleanup when they exit, including tracked tasks, threads, files, directories, and memory.
• App-relative file paths resolve against the app’s working directory, with . and .. normalized.
• The shell identifies ELF binaries that cannot be executed instead of treating them as scripts.
• Shared icon fonts now offer distinct default and large sizes, with updated icons across the interface.
• ESP32 apps can load additional ELF formats when address mirroring is enabled.
• Terminal colors render clearly on monochrome displays, and toolbar buttons have cleaner styling.
• GPS settings safely refresh their device list when their window is hidden or its widgets are destroyed.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes add app-scoped resource cleanup and app-relative path resolution, with ESP32 and POSIX wrappers for resource tracking and filesystem operations. ELF checks use accepted-type masks and provide ELF-magic detection for shell dispatch. Shared icon fonts gain default and large variants. Toolbar styling and monochrome terminal rendering change. The changes also update simulator task hooks, GPS settings widget teardown, C symbols, and tests for cleanup, paths, and ELF behavior.

Priority: ⬇️ Low

Merge Risk: 🟠 High · up to ab54a

App cleanup can cause crashes, invalid execution, or resource corruption on ESP32 and POSIX when app tasks outlive shutdown.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to ab54a

Automatic cleanup can release memory and files while app threads remain active. Resources released from another execution context can also remain registered for a second release. These failures can affect the surrounding runtime, although they require an executing native app that opts into cleanup; no new remote access or privilege grant was established.

Retained concerns

• High · security · inferred: The new final cleanup phase can reclaim resources before all app pthreads have stopped. On ESP32, a thread can pass admission but remain unpublished in the deletion map; cleanup then misses it and proceeds without a join. On POSIX, cancellation followed by an unchecked timed join permits a non-terminating or detached thread to survive. The previous unload hazard predates this PR, but automatic freeing and closing of surviving threads' resources newly expands the memory-safety and failure-containment consequences.
• Medium · security · inferred: Cleanup ownership is selected from the releasing task rather than from the resource. POSIX release wrappers always perform the underlying free or close, but untracking depends on that task's app-image context. If an app-retained resource is released from a foreign callback context, its original tracker can remain stale and final cleanup can release it again, potentially corrupting memory or closing a reused descriptor. The ownership-transfer exclusion does not cover this retained-ownership case. This is an inferred supported-API risk, not an observed failure in a shipped cleanup-enabled app.

Security review details

Security Blast Radius

• inferred — The demonstrated trigger requires executing native app code with cleanup enabled. Cleanup acts on raw allocations and runtime handles, while loaders execute the app in the surrounding runtime. Memory corruption or stale-handle release can therefore affect the POSIX process or ESP32 runtime, not merely the exiting app. Remote, tenant-wide, or fleet-wide reachability was not established.

Security Findings and Attack Paths

• inferred — An app-supplied pthread routine can outlive the cleanup barrier through the ESP32 publication race or a POSIX unsuccessful join. Final cleanup then frees or closes resources it may still access. This establishes a lifecycle failure path, not a demonstrated privilege-escalation exploit.
• inferred — Foreign-task callback execution supplies a possible stale-registration route: app code releases an app-retained resource without its original task context, and later cleanup releases the stale entry. The repository exposes event callbacks and a separate LVGL worker, but its library dispatcher internals and a cleanup-enabled shipped reproduction were not inspected.

Trust Boundaries and Controls

• observed — Resource identity is keyed by AppInstanceId and selected from the current task or thread. App-created POSIX work inherits its owner's image context, and tracker closure rejects further admissions. This protects ordinary tracked work but does not automatically restore ownership for callbacks on independently created service tasks.

Resilience and Maintainability Implications

• observed — The platform join interface returns no status, and task records are discarded before joins complete. Consequently the scheduler cannot distinguish proven termination from timeout or join failure before authorizing unload and reclamation.

Hardening Proposals

• proposed — Make successful task termination an explicit prerequisite for unload and reclamation. Publish ESP32 deletion identity atomically with admission, propagate POSIX join outcomes, and retain ownership plus a defined recovery state when termination cannot be proven.
• proposed — Use resource-origin ownership for release bookkeeping, or establish and enforce an explicit callback-context contract. Validate retained ownership across foreign-task release and descriptor reuse, separately from intentionally transferred resources.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 218 functions across 51 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title refers to app-loading changes in the pull request, but “and more” makes it broad and does not identify the primary change.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 6

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 46a7c6d8-e20a-4ee0-aa41-2b38d2fdf758

📥 Commits

Reviewing files that changed from the base of the PR and between 04ada9d and f07eb64.

📒 Files selected for processing (53)

• CMakeLists.txt
• Documentation/ideas.md
• Modules/app-esp32-module/source/app_esp32_loader_service.cpp
• Modules/app-esp32-module/source/app_symbols.cpp
• Modules/app-esp32-module/source/path_wrap.cpp
• Modules/app-esp32-module/source/root_dir.cpp
• Modules/app-module/include/app/elf_check.h
• Modules/app-module/include/app/libc.h
• Modules/app-module/include/app/manifest.h
• Modules/app-module/include/app/resources.h
• Modules/app-module/private/app/private/resources.h
• Modules/app-module/private/app/private/scheduler.h
• Modules/app-module/source/elf_check.cpp
• Modules/app-module/source/libc.cpp
• Modules/app-module/source/package_manifest_parsing_v3.cpp
• Modules/app-module/source/resources.cpp
• Modules/app-module/source/scheduler.cpp
• Modules/app-module/tests/source/elf_check_test.cpp
• Modules/app-module/tests/source/package_manifest_test.cpp
• Modules/app-module/tests/source/resources_test.cpp
• Modules/app-posix-module/private/app_posix/malloc_wrap.h
• Modules/app-posix-module/private/app_posix/stdio_wrap.h
• Modules/app-posix-module/private/app_posix/task_wrap.h
• Modules/app-posix-module/source/app_posix_loader_service.cpp
• Modules/app-posix-module/source/malloc_wrap.cpp
• Modules/app-posix-module/source/stdio_wrap.cpp
• Modules/app-posix-module/source/stdio_wrap_apple.cpp
• Modules/app-posix-module/source/stdio_wrap_elf.cpp
• Modules/app-posix-module/source/task_wrap.cpp
• Modules/app-posix-module/tests/CMakeLists.txt
• Modules/app-posix-module/tests/fixtures/leak_fixture.cpp
• Modules/app-posix-module/tests/source/cleanup_test.cpp
• Modules/app-posix-module/tests/source/libc_test.cpp
• Modules/c-symbols-module/source/module.cpp
• Modules/lvgl-module/include/lvgl/fonts.h
• Modules/lvgl-module/source/fonts.c
• Modules/lvgl-module/source/symbols.c
• Modules/lvgl-module/source/widgets/toolbar.cpp
• Platforms/platform-esp32/source/mkdir.cpp
• Platforms/platform-esp32/source/vfs_null_path.cpp
• Platforms/platform-posix/freertos/freertos_task_hooks.c
• Platforms/platform-posix/freertos/freertos_task_hooks.h
• Tactility/Private/Tactility/app/terminal/TerminalRenderer.h
• Tactility/Source/app/AppGrid.cpp
• Tactility/Source/app/applist/AppList.cpp
• Tactility/Source/app/apppackagelist/AppPackageList.cpp
• Tactility/Source/app/shell/Shell.cpp
• Tactility/Source/app/shell/main.cpp
• Tactility/Source/app/terminal/TerminalRenderer.cpp
• Tactility/Source/app/timezone/TimeZone.cpp
• Tactility/Source/lvgl/FontSizes.cpp
• Tactility/Source/lvgl/Fonts.cpp
• Tactility/Tests/Source/FontsTest.cpp

💤 Files with no reviewable changes (2)

• Platforms/platform-esp32/source/mkdir.cpp
• Platforms/platform-esp32/source/vfs_null_path.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (3)

🟠 Major · Gate the trampoline until its FreeRTOS handle is registered. · app_symbols.cpp:400-425

Modules/app-esp32-module/source/app_symbols.cpp:400-425
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Gate the trampoline until its FreeRTOS handle is registered.

thread_trampoline() calls app_resources_enter_task() before inserting its handle into thread_tasks. If it is preempted after admission, cleanup can miss the mapping in delete_thread(), clear the task record, and unload the app binary. When the trampoline resumes, it can call copy.function from the unloaded app. Add a start gate so create_thread() publishes the mapping before the trampoline can enter the app.

🟠 Major · Keep a task tracked until cross-core deletion completes. · app_symbols.cpp:365-368

Modules/app-esp32-module/source/app_symbols.cpp:365-368
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Keep a task tracked until cross-core deletion completes.

app_vTaskDelete removes the target from Tracker::tasks before calling vTaskDelete. An app can create a task and delete it from its main task through the exported APIs. On dual-core ESP-IDF, deleting a task running on the other core only triggers a yield there. The target can still execute app code while the caller continues. app_resources_release_tasks then does not wait for that target, and cleanup can unload the app and release its resources while the target is still using them.

Add a completion or join mechanism for cross-task deletion. Remove the tracker entry only after the target can no longer execute. Do not fix this only by moving app_resources_untrack_task after the raw vTaskDelete call, because that call does not provide the required cross-core completion guarantee.

🟠 Major · Do not unload the app while a tracked pthread may still be running. · task_wrap.cpp:162-181

Modules/app-posix-module/source/task_wrap.cpp:162-181
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Do not unload the app while a tracked pthread may still be running.

When an app-created pthread runs code without deferred-cancellation points, thread_delete() only requests cancellation. thread_join() can time out after 500 ms and discards ETIMEDOUT. app_resources_release_tasks() then returns, so finish_app_task() can unload the app while the pthread still executes its code.

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9947bf9e-785d-4f94-b474-729d4d581345

📥 Commits

Reviewing files that changed from the base of the PR and between 831b23f and ab54a5d.

📒 Files selected for processing (1)

• Tactility/Source/app/gpssettings/GpsSettings.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.