LVGL terminal rendering, elf memleak fixes and posix support - #669
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe app module adds signal delivery, signal-aware libc calls, and per-app allocation accounting. POSIX and ESP32 wrappers connect these features to app binaries, and tests cover signal handling and interrupted calls. The terminal gains an LVGL canvas renderer, renderer visibility handling, and window-mode selection. The ideas document updates a TODO item and an app installation example. Other changes update shell shutdown handling, a T-Deck key mapping, and an ESP32 relocation-failure log. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Apps may not respond promptly to signals, and terminal shutdown can race with window updates. Correct the signal paths, renderer cleanup, API documentation, and test synchronization before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new windowed terminal has a shutdown/rebuild race that can access freed memory and disrupt the shared UI. Signal delivery also changes cross-app behavior, while some blocking calls remain outside its promised interruption coverage. Remote exploitation or privilege escalation has not been established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 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: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
dbc50197-5480-4b15-a8c3-9491e1b67d62
📒 Files selected for processing (33)
Documentation/ideas.mdModules/app-esp32-module/source/app_esp32_loader_service.cppModules/app-esp32-module/source/app_symbols.cppModules/app-esp32-module/source/module.cppModules/app-module/include/app/event.hModules/app-module/include/app/libc.hModules/app-module/include/app/memory.hModules/app-module/include/app/signal.hModules/app-module/private/app/private/ledger.hModules/app-module/source/libc.cppModules/app-module/source/module.cppModules/app-module/source/scheduler.cppModules/app-module/source/signal.cppModules/app-module/source/stream.cppModules/app-posix-module/private/app_posix/malloc_wrap.hModules/app-posix-module/private/app_posix/stdio_wrap.hModules/app-posix-module/source/app_posix_loader_service.cppModules/app-posix-module/source/malloc_wrap.cppModules/app-posix-module/source/stdio_wrap.cppModules/app-posix-module/source/stdio_wrap_apple.cppModules/app-posix-module/source/stdio_wrap_elf.cppModules/app-posix-module/tests/source/libc_test.cppTactility/Private/Tactility/app/terminal/KeyboardInput.hTactility/Private/Tactility/app/terminal/Terminal.hTactility/Private/Tactility/app/terminal/TerminalRenderer.hTactility/Private/Tactility/app/terminal/TerminalRendererLvgl.hTactility/Source/app/shell/Run.cppTactility/Source/app/terminal/KeyboardInput.cppTactility/Source/app/terminal/Shell.cppTactility/Source/app/terminal/Terminal.cppTactility/Source/app/terminal/TerminalRenderer.cppTactility/Source/app/terminal/TerminalRendererLvgl.cppTactility/Source/app/terminal/main.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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Deliver pending signals in getpid and getppid. · stdio_wrap.cpp:223-235
Modules/app-posix-module/source/stdio_wrap.cpp:223-235
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDeliver pending signals in
getpidandgetppid.When another app sends
SIGTERMto a non-event app, the signal is queued. The target’s nextgetpid()orgetppid()can return without delivering it, so the default termination action is delayed past the documented next libc call. Callapp_signal_deliver_pending()at the entry of both wrappers.Suggested fix
pid_t __wrap_getpid() { + app_signal_deliver_pending(); int result; if (app_libc_try_getpid(&result)) { return result; @@ pid_t __wrap_getppid() { + app_signal_deliver_pending(); int result; if (app_libc_try_getppid(&result)) { return result;
🟡 Minor · Check pending app signals during the real-poll fallback. · stdio_wrap.cpp:175-185
Modules/app-posix-module/source/stdio_wrap.cpp:175-185
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck pending app signals during the real-poll fallback.
When
app_libc_try_poll()finds no app-owned descriptor, it returnsfalse, and this branch calls__real_poll()with the full timeout.app_signal_send()only sets the unsubscribed app’s pending bit; it does not wake this wait. With an infinite timeout, the app can remain blocked after a signal arrives instead of returning-1/EINTRwithin the documented ~100 ms. Bound this fallback’s waits and check and deliver pending signals between them.
🟡 Minor · Document sleep()'s remaining-seconds return value. · signal.h:20-22
Modules/app-module/include/app/signal.h:20-22
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument
sleep()'s remaining-seconds return value.When a pending signal interrupts
sleep(), the app libc returns the whole seconds not slept, not-1witherrnoset toEINTR. Replacesleepwithusleepin the-1/EINTRstatement and documentsleep()'s separate return value.Suggested fix
- * app_signal_deliver_pending()). A blocking read, write, poll or sleep it is waiting in returns -1 + * app_signal_deliver_pending()). A blocking read, write, poll or usleep call returns -1 * with errno EINTR within about 100 ms. + * A blocked sleep() call returns the number of whole seconds not slept.
🟡 Minor · Clear canvasBuffer_ before releasing the LVGL lock. · TerminalRendererLvgl.cpp:54-74
Tactility/Source/app/terminal/TerminalRendererLvgl.cpp:54-74
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClear
canvasBuffer_before releasing the LVGL lock.If
app_manager_stop()ends the terminal while an upper window is being removed, the window manager can rebuild the hidden terminal’s widgets on the remover thread and callattachCanvas().end()releases the LVGL lock before freeing and nulling the plaincanvasBuffer_, whileattachCanvas()reads it. These accesses can race. Capture and clear the pointer under the lock, then free the captured buffer after unlocking.Suggested fix
lvgl_lock(); + auto* buffer = canvasBuffer_; + canvasBuffer_ = nullptr; lv_obj_t* canvas = canvas_; if (canvas != nullptr) { lv_obj_delete(canvas); canvas_ = nullptr; } lvgl_unlock(); - pixel_buffer_free(canvasBuffer_); - canvasBuffer_ = nullptr; + pixel_buffer_free(buffer);
🟡 Minor · Synchronize on entry to the blocking wait. · libc_test.cpp:543-557
Modules/app-posix-module/tests/source/libc_test.cpp:543-557
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSynchronize on entry to the blocking wait.
The app sets
g_signal_app_blockedbefore callingread,usleep, orpoll. The 50 ms delay does not prove that the call has started. If the task is delayed after setting the flag, the test can sendSIGUSR1before wrapper entry. The wrapper can then deliver and clear the handled signal before the underlying wait, which may block normally. The subsequentwait_for_state(..., APP_INSTANCE_STATE_STOPPED, 1000)can time out and fail the test. Replace the fixed delay with a handshake that observes entry into the underlying wait.
🟡 Minor · Deliver pending signals in both ESP32 PID bindings. · app_symbols.cpp:81-89
Modules/app-esp32-module/source/app_symbols.cpp:81-89
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDeliver pending signals in both ESP32 PID bindings.
When an unsubscribed app has SIGTERM pending, these bindings return without calling
app_signal_deliver_pending().getpid()andgetppid()can therefore return without delivering SIGTERM, so the app can continue past its next libc call with SIGTERM still pending. Callapp_signal_deliver_pending()at the entry of both ESP32 bindings.Suggested fix
pid_t app_getpid() { + app_signal_deliver_pending(); int result; return app_libc_try_getpid(&result) ? result : 0; } pid_t app_getppid() { + app_signal_deliver_pending(); int result; return app_libc_try_getppid(&result) ? result : 0; }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
9e9e0146-323c-4358-b9b1-b75a1523e1d9
📒 Files selected for processing (5)
Drivers/lilygo-module/source/tdeck_keyboard.cppModules/app-module/source/libc.cppModules/app-module/source/signal.cppModules/app-posix-module/tests/source/libc_test.cppTactility/Source/app/terminal/TerminalRendererLvgl.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- Tactility/Source/app/terminal/TerminalRendererLvgl.cpp
- Modules/app-module/source/signal.cpp
- Modules/app-posix-module/tests/source/libc_test.cpp
- Modules/app-module/source/libc.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.
1 participant
Summary by CodeRabbit
tactility.py installsyntax.