Skip to content

fix(recovery): validate UART write bounds with helper - #74

Merged
srpatcha merged 2 commits into
embeddedos-org:masterfrom
furqan72672:fix/recovery-write-bounds
Aug 31, 2026
Merged

fix(recovery): validate UART write bounds with helper#74
srpatcha merged 2 commits into
embeddedos-org:masterfrom
furqan72672:fix/recovery-write-bounds

Conversation

@furqan72672

@furqan72672 furqan72672 commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix recovery UART WRITE bounds validation by routing the handler through the existing eos_recovery_write_in_range() helper before acknowledging or writing data to flash.

Type of Change

  • fix - Bug fix
  • test - Add or fix tests

Changes

  • Removed the duplicate slot_size declaration in recovery_handle_write().
  • Updated recovery_handle_write() to call eos_recovery_write_in_range() before sending the ready ACK.
  • Strengthened the helper to reject write-end uint32_t address wrap.
  • Added unit coverage for valid writes, slot-boundary rejection, zero base/slot/length, slot overflow, and address-wrap cases.

Testing

  • Manual/static validation performed
  • New tests added for recovery WRITE bounds behavior
  • Unit tests pass (ctest --test-dir build --output-on-failure)
  • Integration tests pass

Validation performed locally:

  • git diff --check passed with only CRLF normalization warnings.
  • Static search confirmed only one slot_size declaration remains.
  • Static search confirmed recovery_handle_write() calls eos_recovery_write_in_range().

Not run locally:

  • cmake -B build -DEBLDR_BUILD_TESTS=ON
  • cmake --build build --parallel
  • ctest --test-dir build --output-on-failure -R test_recovery

Reason: cmake, ctest, and pytest are not installed in my local environment.

Pre-Submission Checklist

  • Code compiles without warnings (-Wall -Wextra -Werror for C)
  • All existing tests pass
  • New tests added for new functionality
  • Documentation updated if API changed
  • Commit messages follow <type>(<scope>): <description> convention
  • Branch is rebased on latest master

Related Issues

None.

Screenshots / Logs

Not applicable.

Additional Notes

A separate review pass found no issues in the diff. The remaining limitation is that the C build and CTest suite still need to be run in CI or on a machine with the required toolchain installed.

Copilot AI lite review requested due to automatic review settings August 30, 2026 21:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

TASKS.md verification commands use Windows-style path separators that are likely not runnable in the repo’s POSIX-oriented workflows, reducing verifiability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Introduces and adopts a shared bounds-validation helper for UART recovery WRITE packets to prevent out-of-slot and address-wrap flash writes.

Changes:

  • Add eos_recovery_write_in_range() helper to validate (base, slot_size, offset, len) including overflow cases.
  • Update recovery_handle_write() to use the helper and remove the duplicate slot_size declaration.
  • Add unit tests covering helper boundary/overflow behavior and keep the existing out-of-bounds WRITE regression test.
File summaries
File Description
core/recovery.c Adds the bounds helper and uses it in the UART WRITE handler before ACK/flash write.
tests/unit/test_recovery.c Adds a unit test for the new bounds helper and wires it into main().
TASKS.md Adds/updates task T-007 with acceptance criteria and verification notes for the change.
Review details

Suppressed comments (1)

TASKS.md:50

  • The Python verification command uses Windows-style path separators (tests\unit\...), which is likely to fail on POSIX shells. Use forward slashes so the command is portable and matches the repository's documented commands.
  | Build | `cmake -B build -DEBLDR_BUILD_TESTS=ON` | `UNKNOWN` - `cmake` is not installed in this environment |
  | Unit test | `ctest --test-dir build --output-on-failure -R test_recovery` | `UNKNOWN` - `ctest` is not installed in this environment |
  | Python unit tests | `pytest tests\unit\test_uart_recovery.py tests\unit\test_sign_image.py` | `UNKNOWN` - `pytest` is not installed in this environment |
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread TASKS.md
Comment on lines +45 to +46
| Static check - duplicate slot size | `rg "uint32_t slot_size = eos_hal_slot_size" core\recovery.c` | `PASS` - one declaration remains |
| Static check - helper use | `rg "eos_recovery_write_in_range" core\recovery.c tests\unit\test_recovery.c` | `PASS` - helper is defined, called by `recovery_handle_write()`, and covered by tests |
srpatcha
srpatcha previously approved these changes Aug 31, 2026
Copilot AI review requested due to automatic review settings August 31, 2026 08:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

@srpatcha
srpatcha self-requested a review August 31, 2026 08:02
@srpatcha
srpatcha merged commit 9feca1b into embeddedos-org:master Aug 31, 2026
21 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants