-
-
Notifications
You must be signed in to change notification settings - Fork 467
fix(vm): guard VM stack against underflow #860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Add explicit stack length checks in pop and current helpers. Extend VM tests to assert safe underflow error reporting. Signed-off-by: Ville Vesilehto <[email protected]>
|
But what is the case? Those opcode seq will not be generated? Only manually. |
|
Yes, the compiler output doesn't generate those sequences. But |
| if len(vm.Stack) == 0 { | ||
| panic("stack underflow") | ||
| } | ||
| return vm.Stack[len(vm.Stack)-1] |
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.
But this will also panic (with differnt message obviosly)
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.
Yeah, it's then easier to assert to that consistent error msg (if wanted). Maybe "robust" would have been a better choice than "safer".
Also just realised that this might become handy if the project fuzzer would do bytecode fuzzing as well. Distinguishing stack underflow from other errors.
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.
I wrote a small bytecode fuzzer and found other similar control flow edge cases. While the compiler never emits negative offsets for jump opcodes like OpJump and OpJumpIfTrue, the VM will happily take those in if defined as program bytecode. I was able to generate an infinite loop quite easily.
Example:
OpInt // arg: ...
OpInt // arg: ...
OpJump // arg: -N (negative offset)
OpInt
OpJump
Fix is simple:
func (vm *VM) Run(program *Program, env any) (_ any, err error) {
...
for vm.ip < len(program.Bytecode) {
case OpJump:
if arg < 0 {
panic("negative jump offset is invalid")
}
vm.ip += arg
...I can certainly open PRs to fix these, if we find this type of VM hardening feasible.
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.
I’m thinking of what benefits other than more readable error messages those checks can bring
Add explicit stack length checks in
popandcurrentVM helpers. Extend VM tests to assert safe underflow error reporting.The test fails against
masteras follows:Bench results from
go test -run=^$ -bench=. -count=10 .below. Geomean at roughly +2%.