GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Make `OpArgType::from_op_arg` to return a `Result<Self, MarshalError>` by ShaharNaveh · Pull Request #6914 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughA refactoring of error handling in bytecode instruction processing, converting return types from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
to_oparg to return a Result<Self, MarshalError>OpArgType::from_op_arg to return a Result<Self, MarshalError>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 640-645: from_op_arg for UnpackExArgs currently truncates high
bytes via to_le_bytes; instead validate that the upper two bytes of x are zero
before constructing UnpackExArgs to avoid accepting invalid bytecode: check (x
>> 16) == 0 (or inspect bytes[2..]) and return an appropriate MarshalError if
non-zero, otherwise proceed to extract before and after and return
Ok(Self{before, after}); reference UnpackExArgs, OpArgType::from_op_arg, and
MarshalError to locate where to add the check and error return.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| impl OpArgType for UnpackExArgs { | ||
| #[inline(always)] | ||
| fn from_op_arg(x: u32) -> Option<Self> { | ||
| fn from_op_arg(x: u32) -> Result<Self, MarshalError> { | ||
| let [before, after, ..] = x.to_le_bytes(); | ||
| Some(Self { before, after }) | ||
| Ok(Self { before, after }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate upper bytes for UnpackExArgs to avoid accepting invalid bytecode.
Right now values with non-zero high bytes are silently truncated, which defeats the new error reporting.
🛠️ Suggested fix
fn from_op_arg(x: u32) -> Result<Self, MarshalError> {
- let [before, after, ..] = x.to_le_bytes();
- Ok(Self { before, after })
+ let [before, after, hi1, hi2] = x.to_le_bytes();
+ if hi1 != 0 || hi2 != 0 {
+ return Err(MarshalError::InvalidBytecode);
+ }
+ Ok(Self { before, after })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl OpArgType for UnpackExArgs { | |
| #[inline(always)] | |
| fn from_op_arg(x: u32) -> Option<Self> { | |
| fn from_op_arg(x: u32) -> Result<Self, MarshalError> { | |
| let [before, after, ..] = x.to_le_bytes(); | |
| Some(Self { before, after }) | |
| Ok(Self { before, after }) | |
| } | |
| impl OpArgType for UnpackExArgs { | |
| #[inline(always)] | |
| fn from_op_arg(x: u32) -> Result<Self, MarshalError> { | |
| let [before, after, hi1, hi2] = x.to_le_bytes(); | |
| if hi1 != 0 || hi2 != 0 { | |
| return Err(MarshalError::InvalidBytecode); | |
| } | |
| Ok(Self { before, after }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 640 - 645,
from_op_arg for UnpackExArgs currently truncates high bytes via to_le_bytes;
instead validate that the upper two bytes of x are zero before constructing
UnpackExArgs to avoid accepting invalid bytecode: check (x >> 16) == 0 (or
inspect bytes[2..]) and return an appropriate MarshalError if non-zero,
otherwise proceed to extract before and after and return Ok(Self{before,
after}); reference UnpackExArgs, OpArgType::from_op_arg, and MarshalError to
locate where to add the check and error return.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.