Rework implementation of StorageOperation command - #3502
Conversation
- Now using storage file system to allow storing at any available storage. - Moved from platform to HAL level as this can now use a commom implementation. - Slight refactor to use file sytem API. - Update CMake accordingly. - Drop NameLength in Monitor_StorageOperation_Command.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAccessible-storage operations now use a shared nanoHAL implementation. The wire command and HAL signature omit ChangesStorage operation migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/HAL/nanoHAL_StorageOperation.cpp`:
- Around line 18-20: Update HAL_StorageOperation to honor offset for Write and
other offset-based storage updates instead of discarding it; ensure writes begin
at the requested position while preserving Append’s EOF behavior. If offset
writes are unsupported, remove Offset consistently from the storage-operation
contract and tooling rather than ignoring it.
- Around line 143-146: Update HAL_StorageOperation so storageName is treated as
non-owned and is never freed when it aliases the command payload data. Remove
the platform_free(storageName) cleanup block, unless replacing it with an
explicit filename allocation/copy whose owned allocation alone is freed.
- Line 22: Remove the platform_free(storageName) cleanup from the storage
operation flow, where storageName references cmd->Data and SplitFilePath-derived
pointers. Keep payload-buffer ownership with the normal WireProtocol command
lifetime and do not free this borrowed pointer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5534772c-8d27-4f5b-afda-1a78a9d9da17
📒 Files selected for processing (17)
CMake/binutils.ESP32.cmakeCMake/binutils.common.cmakesrc/CLR/Debugger/Debugger.cppsrc/CLR/Debugger/Debugger.hsrc/CLR/Include/WireProtocol_MonitorCommands.hsrc/HAL/Include/nanoHAL_StorageOperation.hsrc/HAL/nanoHAL_StorageOperation.cpptargets/ChibiOS/_common/CMakeLists.txttargets/ChibiOS/_common/targetHAL_StorageOperation.cpptargets/ChibiOS/_include/targetHAL_StorageOperation.htargets/ESP32/_common/CMakeLists.txttargets/ESP32/_common/targetHAL_StorageOperation.cpptargets/ESP32/_include/targetHAL_StorageOperation.htargets/FreeRTOS/NXP/_common/CMakeLists.txttargets/ThreadX/Maxim/_common/CMakeLists.txttargets/ThreadX/ST/_common/CMakeLists.txttargets/ThreadX/SiliconLabs/_common/CMakeLists.txt
💤 Files with no reviewable changes (11)
- targets/ChibiOS/_include/targetHAL_StorageOperation.h
- targets/ESP32/_common/CMakeLists.txt
- targets/ESP32/_common/targetHAL_StorageOperation.cpp
- targets/ThreadX/Maxim/_common/CMakeLists.txt
- targets/ESP32/_include/targetHAL_StorageOperation.h
- src/CLR/Include/WireProtocol_MonitorCommands.h
- targets/ThreadX/ST/_common/CMakeLists.txt
- targets/ThreadX/SiliconLabs/_common/CMakeLists.txt
- targets/ChibiOS/_common/targetHAL_StorageOperation.cpp
- targets/ChibiOS/_common/CMakeLists.txt
- targets/FreeRTOS/NXP/_common/CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/HAL/nanoHAL_StorageOperation.cpp (1)
49-126: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-125): Out-of-bounds Read
Reachability: External
Bound
Write/Appendcopies to the remaining command payload
HAL_StorageOperationcomputesfileDatato start afterstorageName, then passes the wireDataLengthdirectly tovolume->Write; both paths also accept longerrelativePathstrings without bounding the pointer search. Reject commands whereDataLengthis larger than the bytes after the path terminator, and bound the offset before usingfileDatainWrite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/HAL/nanoHAL_StorageOperation.cpp` around lines 49 - 126, Validate the command payload in HAL_StorageOperation before deriving fileData: ensure relativePath is NUL-terminated within the available payload and reject any DataLength exceeding the bytes remaining after that terminator. Bound the computed offset and use the validated remaining length for volume->Write in both StorageOperation_Write and StorageOperation_Append, preserving existing WriteError handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/HAL/nanoHAL_StorageOperation.cpp`:
- Around line 88-93: Preserve the existing Offset wire contract and use it in
the storage write path: update the HAL storage-operation implementation around
HAL_StorageOperation and seek fileHandle to the requested offset before
volume->Write, propagating seek failure through the existing error handling.
Keep Debugger.cpp’s cmd->Offset flow and the HAL declaration consistent, and do
not discard offset.
---
Outside diff comments:
In `@src/HAL/nanoHAL_StorageOperation.cpp`:
- Around line 49-126: Validate the command payload in HAL_StorageOperation
before deriving fileData: ensure relativePath is NUL-terminated within the
available payload and reject any DataLength exceeding the bytes remaining after
that terminator. Bound the computed offset and use the validated remaining
length for volume->Write in both StorageOperation_Write and
StorageOperation_Append, preserving existing WriteError handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b03af3c1-df1f-4d19-a543-336f38c727bf
📒 Files selected for processing (1)
src/HAL/nanoHAL_StorageOperation.cpp
3 participants
Description
v2022.14.1.13v2019.14.1.13nanoff>2.5.158with support for this new command packet.Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist