stdio, keyboard, compact UI, package manifest, and more - #667
KenVanHoeylandt wants to merge 3 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe changes add app-aware libc adapters and platform wrappers, package compatibility checks based on device and RAM requirements, and ESP ELF loader wrappers. They also implement terminal scroll regions, change Enter mappings to line feed, update T-Lora Pager configuration, and adjust selected UI layouts. Priority: ➖ Normal Merge Risk: 🔵 Low · up to Out-of-range App Hub RAM metadata can reach an undefined numeric conversion before validation. Add the localized bounds check; the established merge risk is otherwise low. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected application-call paths preserve important descriptor and signal controls. No material security regression was established, but native-call fallback and concurrent teardown remain incompletely resolved, so the changes should not be treated as risk-free. 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: 6f53bc68-097f-4166-bfa0-27b0776d3984
📒 Files selected for processing (69)
Buildscripts/TactilitySDK/TactilitySDK.esp32.cmakeBuildscripts/TactilitySDK/TactilitySDK.posix.cmakeCMakeLists.txtDevices/cl32/source/cl32_v2_keyboard.cppDevices/lilygo-tdeck-max/lilygo,tdeck-max.dtsDevices/lilygo-tdeck-pro/lilygo,tdeck-pro.dtsDevices/lilygo-tlora-pager/CMakeLists.txtDevices/lilygo-tlora-pager/device.propertiesDevices/lilygo-tlora-pager/lilygo,tlora-pager.dtsDevices/lilygo-tlora-pager/module.yamlDevices/lilygo-tlora-pager/source/module.cppDocumentation/ideas.mdDrivers/m5stack-module/source/cardputer_keyboard.cppModules/app-esp32-module/CMakeLists.txtModules/app-esp32-module/source/app_esp32_loader_service.cppModules/app-esp32-module/source/elf_cache.cppModules/app-esp32-module/source/elf_relocate.cppModules/app-esp32-module/source/stdio_wrap.cppModules/app-module/CMakeLists.txtModules/app-module/include/app/dir.hModules/app-module/include/app/file.hModules/app-module/include/app/io.hModules/app-module/include/app/libc.hModules/app-module/include/app/package_manifest.hModules/app-module/include/poll.hModules/app-module/include/sys/ioctl.hModules/app-module/private/app/private/fd_table.hModules/app-module/private/app/private/ledger.hModules/app-module/private/app/private/stdio_wrap.hModules/app-module/source/fd_table.cppModules/app-module/source/io.cppModules/app-module/source/libc.cppModules/app-module/source/module.cppModules/app-module/source/package_compatibility.cppModules/app-module/source/package_manifest_parsing_v3.cppModules/app-module/source/scheduler.cppModules/app-module/source/stdio_wrap.cppModules/app-module/source/stream.cppModules/app-module/tests/CMakeLists.txtModules/app-module/tests/source/execute_test.cppModules/app-module/tests/source/io_test.cppModules/app-module/tests/source/package_manifest_test.cppModules/app-posix-module/CMakeLists.txtModules/app-posix-module/private/app_posix/stdio_wrap.hModules/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/CMakeLists.txtModules/app-posix-module/tests/source/libc_test.cppModules/c-symbols-module/source/module.cppModules/posix-symbols-module/source/module.cppTactility/Private/Tactility/app/apphub/AppHubEntry.hTactility/Source/app/apphub/AppHubApp.cppTactility/Source/app/apphub/AppHubEntry.cppTactility/Source/app/applist/AppList.cppTactility/Source/app/apppackagelist/AppPackageList.cppTactility/Source/app/fileselection/FileSelection.cppTactility/Source/app/inputdialog/InputDialog.cppTactility/Source/app/poweroff/PowerOff.cppTactility/Source/app/shell/LineEditor.cppTactility/Source/app/terminal/vterm/vterm.cTactility/Source/lvgl/wrappers/obj.cppTactility/Tests/Source/AppHubEntryTest.cppTactility/Tests/Source/VtermTest.cppTactilityKernel/include/tactility/drivers/keyboard.hTactilityKernel/include/tactility/memory.hTactilityKernel/source/drivers/keyboard.cppTactilityKernel/source/memory.cppTactilityKernel/source/symbols.c
💤 Files with no reviewable changes (8)
- Modules/app-module/tests/source/execute_test.cpp
- Devices/lilygo-tlora-pager/CMakeLists.txt
- Devices/lilygo-tlora-pager/source/module.cpp
- Modules/app-module/include/app/file.h
- Modules/app-module/include/app/io.h
- Modules/app-module/source/stdio_wrap.cpp
- Modules/app-module/private/app/private/stdio_wrap.h
- Modules/app-module/tests/source/io_test.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 · Handle closed app descriptors before the real ioctl() fallback. · libc.cpp:51-64
Modules/app-module/source/libc.cpp:51-64
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle closed app descriptors before the real
ioctl()fallback.
app_io_ioctl()returnsERROR_NOT_FOUNDfor a closed descriptor, soapp_libc_try_window_size()returnsfalse. Both platform wrappers then call the realioctl(). A reused real descriptor can produce a window size instead of the requiredEBADF.Use
get_app_fd_state()in the shared adapter. Returningtruefor the closed state prevents the fallback in both wrappers.Suggested fix
- if (request != TIOCGWINSZ || arg == nullptr) { + if (request != TIOCGWINSZ) { return false; } + const AppFdState state = get_app_fd_state(fd); + if (state == AppFdState::Closed) { + errno = EBADF; + return true; + } + if (arg == nullptr || state == AppFdState::NotAppFd) { + return false; + } AppWindowSize size {};
🟡 Minor · Route ESP32 kill() through the app-scoped helper. · stdio_wrap.cpp:112-120
Modules/app-esp32-module/source/stdio_wrap.cpp:112-120
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRoute ESP32
kill()through the app-scoped helper.
kill()is exported to apps, but the ESP32 boundary defines nokill()wrapper and the linker adds no--wrap=killoption. An app call can therefore reach the platformkill()symbol without callingapp_libc_try_kill(). This violates the app contract, which requires-1witherrno == ENOSYS.Suggested fix
#include <signal.h> +#include <sys/types.h> #include <sys/poll.h> #include <sys/stat.h> #include <termios.h> @@ _sig_func_ptr signal(int sig, _sig_func_ptr handler) { AppLibcSignalHandler previous; if (app_libc_try_signal(sig, handler, &previous)) { return previous; } errno = ENOSYS; return SIG_ERR; } +int kill(pid_t pid, int sig) { + int result; + if (app_libc_try_kill(static_cast<int>(pid), sig, &result)) { + return result; + } + errno = ENOSYS; + return -1; +} + // Called by an app, newlib's exit() would reach _exit(), which aborts the whole device
🟡 Minor · Reject fractional requiresRam values before integer conversion. · AppHubEntry.cpp:104-109
Tactility/Source/app/apphub/AppHubEntry.cpp:104-109
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject fractional
requiresRamvalues before integer conversion.
AppHubEntry::requiresRamis an integer megabyte count.readInt32currently accepts any JSON number and truncates it beforeisCompatiblevalidates the range. Therefore,1.5becomes1and can pass compatibility on a device with less than 1.5 MiB available.-0.5becomes0, bypasses the negative-value check, and is accepted as no RAM requirement.Reject non-integral values in
readInt32.parseEntryalready rejects the entry when this reader returnsfalse.Suggested fix
#include <cJSON.h> +#include <cmath> #include <string> #include <vector> @@ - output = static_cast<int32_t>(buffer); + if (buffer != std::trunc(buffer)) { + LOG_E(TAG, "%s is not an integer", key); + return false; + } + output = static_cast<int32_t>(buffer);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: dd0d4a9f-b8af-49c1-a960-add4a84b3d59
📒 Files selected for processing (4)
Tactility/Source/app/apphub/AppHubEntry.cppTactility/Source/app/terminal/vterm/vterm.cTactility/Tests/Source/AppHubEntryTest.cppTactility/Tests/Source/VtermTest.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- Tactility/Tests/Source/AppHubEntryTest.cpp
- Tactility/Tests/Source/VtermTest.cpp
- Tactility/Source/app/apphub/AppHubEntry.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.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: f4a119a4-a504-449a-9ab3-9f93a3c149a1
📒 Files selected for processing (7)
Modules/app-esp32-module/source/stdio_wrap.cppModules/app-module/include/app/libc.hModules/app-module/source/libc.cppModules/app-posix-module/source/stdio_wrap.cppModules/app-posix-module/tests/source/libc_test.cppTactility/Private/Tactility/json/Reader.hTactility/Tests/Source/AppHubEntryTest.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
New Features
Bug Fixes
Style