Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18658
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (2 Unrelated Failures)As of commit 4a49d17 with merge base c39a0ff ( BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
Adds load-time validation for FlatBuffer instruction value indices and centralizes the bounds-check logic, preventing out-of-range access during execution.
Changes:
- Introduces
ET_CHECK_VALID_VALUE_INDEXmacro to validate instruction value indices againstn_value_. - Applies the validation to
JumpFalseCall,MoveCall(bothmove_from/move_to), andFreeCall. - Changes the default instruction-args handler to log and fail fast on invalid instruction argument types.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
runtime/executor/method.cpp
Outdated
| default: { | ||
| chain_instruction_arg_lists[instr_idx] = InstructionArgs(); | ||
| } break; | ||
| ET_LOG(Error, "Invalid instruction type %hhu", static_cast<uint8_t>(instruction->instr_args_type())); |
There was a problem hiding this comment.
The log message doesn’t include enough context to diagnose which instruction caused the failure (e.g., instr_idx and/or the chain index i). Including those identifiers in the error log materially improves debuggability for malformed/corrupt programs.
| ET_LOG(Error, "Invalid instruction type %hhu", static_cast<uint8_t>(instruction->instr_args_type())); | |
| ET_LOG( | |
| Error, | |
| "Invalid instruction type %hhu at chain index %" ET_PRIsize_t | |
| " instruction index %" ET_PRIsize_t, | |
| static_cast<uint8_t>(instruction->instr_args_type()), | |
| i, | |
| instr_idx); |
JacobSzwejbka
left a comment
There was a problem hiding this comment.
Looks good but with this general class of error checking should we be centralizing it at program load rather then method init?
I feel like we might need a layered approach. The EValues are parsed at method load time, so it makes sense to check index validity of Free/JumpFalse/Move instructions here .. |
Summary
Check MoveCall indices are within range (0 <= index < n_values_)
Use a macro to wrap the calls in MoveCall, JumpFalseCall, FreeCall