fix verifier panic and interpreter overflow #137
Conversation
Signed-off-by: Echo8377 <[email protected]>
Echo8377
force-pushed
the
fix-verifier-panic-interpreter-overflow
branch
from
bef13b0 to
763b582
Compare
src/interpreter.rs
Outdated
| // Use checked arithmetic to prevent integer overflow | ||
| let offset = insn.imm as isize; | ||
| if offset < 0 { | ||
| let abs_offset = (-offset) as usize; | ||
| if abs_offset > insn_ptr { | ||
| Err(Error::other(format!( | ||
| "Error: call offset underflow (insn #{})", | ||
| insn_ptr - 1 | ||
| )))?; | ||
| } | ||
| insn_ptr -= abs_offset; | ||
| } else { | ||
| insn_ptr = insn_ptr.checked_add(offset as usize).ok_or_else(|| { | ||
| Error::other(format!( | ||
| "Error: call offset overflow (insn #{})", | ||
| insn_ptr - 1 | ||
| )) | ||
| })?; | ||
| } |
There was a problem hiding this comment.
Given that we work with immediate values, we know the value at verification time, and we shouldn't be doing these checks at runtime, we should do it in src/verifier.rs instead.
There was a problem hiding this comment.
Given that we work with immediate values, we know the value at verification time, and we shouldn't be doing these checks at runtime, we should do it in
src/verifier.rsinstead.
You're right about avoiding runtime checks. I checked verifier.rs and realized your original code was already solid and handles these bounds correctly. So I've removed the redundant check from interpreter.rs to restore the efficient behavior.
There was a problem hiding this comment.
I checked verifier.rs and realized your original code was already solid and handles these bounds correctly
I don't think it does, I tried your reproducer yesterday and the code in the verifier doesn't prevent the overflow in debug mode. We should probably reject the program in that case. But I can look into it at some point as well, I didn't have time yesterday.
Signed-off-by: Echo8377 <[email protected]>
2 participants
No description provided.