Skip to content

Remove possible panic paths#82

Merged
RobertZ2011 merged 2 commits intoOpenDevicePartnership:mainfrom
RobertZ2011:remove-panics
Dec 15, 2025
Merged

Remove possible panic paths#82
RobertZ2011 merged 2 commits intoOpenDevicePartnership:mainfrom
RobertZ2011:remove-panics

Conversation

@RobertZ2011
Copy link
Contributor

@RobertZ2011 RobertZ2011 commented Dec 9, 2025

This pull request focuses on improving safety and robustness throughout the codebase by replacing unchecked slice and array indexing with safe .get()/.get_mut() accessors and explicit error handling. It also simplifies the conversion of 4-byte string literals to u32 constants, and updates several function signatures and trait implementations for consistency and correctness. Additionally, new unit tests have been added to ensure coverage of enum conversions.

Safety and Error Handling Improvements:

  • Replaced all direct slice indexing (e.g., slice[start..end]) with .get() or .get_mut() calls followed by .ok_or(PdError::InvalidParams)? or similar error handling in fw_update.rs, internal/command.rs, and internal/mod.rs to prevent panics from out-of-bounds access. [1] [2] [3] [4] [5] [6] [7] [8]
  • Updated error propagation in interrupt control and port address access to use .get() with error handling instead of manual bounds checks. [1] [2]

API and Type Consistency:

  • Changed function signatures that previously used ExpectedPdo as an error type to use RxCapsError for more accurate error reporting in internal/mod.rs. [1] [2] [3]
  • Updated PDO extraction logic to use safe indexing with error propagation for both SPR and EPR PDOs.

String-to-u32 Conversion Simplification:

  • Refactored u32_from_str to accept a [u8; 4] array instead of a &str, simplifying usage and eliminating panics from incorrect string lengths. Updated all call sites accordingly in command/mod.rs and lib.rs. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]

Testing and Code Quality:

  • Added comprehensive unit tests for the PpsRequestInterval enum to ensure all possible u8 values are covered and no panics occur.
  • Simplified a match arm in PpsRequestInterval to clarify panic safety.

Minor Cleanups:

  • Removed unused imports in rx_caps.rs.

@RobertZ2011 RobertZ2011 self-assigned this Dec 9, 2025
@RobertZ2011 RobertZ2011 marked this pull request as ready for review December 9, 2025 23:51
@RobertZ2011 RobertZ2011 requested a review from a team as a code owner December 9, 2025 23:51
@RobertZ2011 RobertZ2011 requested review from asasine, Copilot, felipebalbi, gjpmsft and jerrysxie and removed request for Copilot December 9, 2025 23:51
felipebalbi
felipebalbi previously approved these changes Dec 10, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes potential panic paths from the codebase by replacing panic-inducing operations with checked alternatives that return errors instead.

Key changes:

  • Replaced Index/IndexMut trait implementations with safe get/get_mut methods that return Option
  • Changed u32_from_str to accept a byte array parameter instead of a string slice, eliminating runtime length panics
  • Replaced unchecked array indexing with checked alternatives using get/get_mut throughout async code paths

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/stream.rs Added panic safety comment and clippy allow directive for intentional zero-length slice indexing
src/registers/rx_caps.rs Removed Index/IndexMut implementations that could panic; replaced with safe get/get_mut methods; introduced RxCapsError enum for better error handling
src/registers/autonegotiate_sink.rs Updated unreachable comment and added comprehensive test coverage for PpsRequestInterval::from
src/lib.rs Modified u32_from_str to accept byte array parameter instead of string slice, eliminating potential panic on length check
src/command/mod.rs Removed duplicate u32_from_str function; updated all command constants to use byte array syntax
src/asynchronous/interrupt.rs Replaced unchecked array indexing with checked get_mut that returns error on out-of-bounds access
src/asynchronous/internal/mod.rs Added bounds checking to buffer operations in register read/write functions; updated error types to use RxCapsError
src/asynchronous/internal/command.rs Added bounds checking to buffer slice operations in command result reading
src/asynchronous/fw_update.rs Added comprehensive bounds checking to all buffer operations during firmware update process

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

asasine
asasine previously approved these changes Dec 12, 2025
Copy link

@asasine asasine left a comment

Choose a reason for hiding this comment

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

nb

@RobertZ2011 RobertZ2011 enabled auto-merge (squash) December 15, 2025 17:36
@RobertZ2011 RobertZ2011 merged commit dcc4d71 into OpenDevicePartnership:main Dec 15, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Embedded Controller Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

Comments