[Bug]panic in debug mode when invoking status_type() if status is VMStatus::Error{ StatusCode::OUT_OF_GAS }

[JerryKwan]

🐛 Bug

panic occurred when invoking status_type() in debug mode if status is VMStatus::Error{StatusCode::OUT_OF_GAS} The backtrace is something like the following

To reproduce

Code snippet to reproduce

use crate::{
    vm_status::{VMStatus, StatusCode},
};

#[test]
fn test_stats_code() {
    let vm_status = VMStatus::Error(StatusCode::OUT_OF_GAS);
    let code = vm_status.status_code();
    assert_eq!(code, StatusCode::OUT_OF_GAS);
}

Stack trace/error message

ERROR - details: panicked at 'assertion failed: code.status_type() != StatusType::Execution', /root/.cargo/git/checkouts/move-ae0d26a86327ae96/4735fa5/language/move-core/types/src/vm_status.rs:132:17
details: panicked at 'assertion failed: code.status_type() != StatusType::Execution', /root/.cargo/git/checkouts/move-ae0d26a86327ae96/4735fa5/language/move-core/types/src/vm_status.rs:132:17
2022-06-13T07:19:57.761619499-07:00 ERROR - backtrace:    0:     0x5603d1dbd07a - backtrace::backtrace::libunwind::trace::h75698078811e3944
                               at /root/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.65/src/backtrace/libunwind.rs:93:5
                           backtrace::backtrace::trace_unsynchronized::hd57af2b933e855e0
                               at /root/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.65/src/backtrace/mod.rs:66:5
   1:     0x5603d1dbcff2 - backtrace::backtrace::trace::hf602886d933bcd64
                               at /root/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.65/src/backtrace/mod.rs:53:14
   2:     0x5603d1e2a7c6 - backtrace::capture::Backtrace::create::h5d0bd96752a1217b
                               at /root/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.65/src/capture.rs:176:9
   3:     0x5603d1e2a6fd - backtrace::capture::Backtrace::new::h72069796affebd67
                               at /root/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.65/src/capture.rs:140:22
   4:     0x5603ccdcd039 - starcoin_node::crash_handler::handle_panic::hb3281e643e7aef22
                               at /home/workspace/starcoin/node/src/crash_handler.rs:21:38
   5:     0x5603cd0ae26e - starcoin_node::crash_handler::setup_panic_handler::{{closure}}::h2bd2376d594e3e12
                               at /home/workspace/starcoin/node/src/crash_handler.rs:14:9
   6:     0x5603d21449f4 - std::panicking::rust_panic_with_hook::h3b7380e99b825b63
                               at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/std/src/panicking.rs:702:17
   7:     0x5603d2144669 - std::panicking::begin_panic_handler::{{closure}}::h8e849d0710154ce0
                               at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/std/src/panicking.rs:586:13
   8:     0x5603d21420b4 - std::sys_common::backtrace::__rust_end_short_backtrace::hedcdaddbd4c46cc5

Expected Behavior

status_code return the status code normally, no panic occurred

System information

Please complete the following information:

Additional context

Add any other context about the problem here.

[tnowacki]

This isn't really a bug, in that you shouldn't be making an Error status with OUT_OF_GAS, it should be an execution failure

[JerryKwan]

@tnowacki Thanks for looking into this. You are right, OUT_OF_GAS should be an VMStatus::ExecutionFailure not VMStatus::Error, but in the current implementation a lot of StatusCode's status_type() will return StatusType::Execution https://github.com/move-language/move/blob/75fb97eee86966d2c8e6be96fe21da969250e36c/language/move-core/types/src/vm_status.rs#L673-L708 https://github.com/move-language/move/blob/75fb97eee86966d2c8e6be96fe21da969250e36c/language/move-core/types/src/vm_status.rs#L122-L137 And regarding the place where VMStatus::Error(StatusCode::OUT_OF_GAS) generated, GasStatus::deduct_gas() will return PartialVMError::new(StatusCode::OUT_OF_GAS), and PartialVMError::finish(self, location: Location) -> VMError, and VMError::into_vm_status will return VMStatus::Error(major_status) other than VMStatus::ExecutionFailure if location is Location::Undefined. But most of the time VMRuntime::execute_function_impl() use Location::Undefined https://github.com/move-language/move/blob/75fb97eee86966d2c8e6be96fe21da969250e36c/language/move-vm/types/src/gas_schedule.rs#L82-L97 https://github.com/move-language/move/blob/75fb97eee86966d2c8e6be96fe21da969250e36c/language/move-binary-format/src/errors.rs#L55-L123 https://github.com/move-language/move/blob/750903126278ae325461b8c5c9d24f1c708965f5/language/move-vm/runtime/src/runtime.rs#L312-L377

[tnowacki]

a lot of StatusCode's status_type() will return StatusType::Execution

Thats why there is an assert to make sure they aren't marked as errors in the wrong category

And regarding the place where VMStatus::Error(StatusCode::OUT_OF_GAS) generated, GasStatus::deduct_gas() will return PartialVMError::new(StatusCode::OUT_OF_GAS), and PartialVMError::finish(self, location: Location) -> VMError, and VMError::into_vm_status will return VMStatus::Error(major_status) other than VMStatus::ExecutionFailure if location is Location::Undefined. But most of the time VMRuntime::execute_function_impl() use Location::Undefined

Yeah I remember someone looking at this long ago. I would be okay with OUT_OF_GAS being special cased here. As it can happen at a specific point, but might not (if gas is being charged by the adapter or someone else outside of execution)

[JerryKwan]

@tnowacki Sometimes It's hard to distinguish VMStatus::ExecutionFailure and VMStatus::Execution. Especially when using PartialVMError::finish(self, location: Location) -> VMError, because it's not available to get the location all the time. BTW, Is there any way to get the location where error happens? such as from a Session object like session.runtime.loader.xx?

[tnowacki]

I'm not sure I fully understand the question. The location for the error is setting is all rather manual at each site, but is informed by contextual information

[JerryKwan]

@tnowacki Regarding this issue, it's better if we can remove or change the debug_assert logic And if there is an easy way to get the location of the error from the context, then it will be much easier to fix the panic such as VMStatus::Error{ StatusCode::OUT_OF_GAS }, that is to say we can set the location of error other than Location::Undefined in the logic like the following https://github.com/starcoinorg/starcoin/blob/9d7fad31575f20ae3ed0aa60b686663c7afef31c/vm/vm-runtime/src/starcoin_vm.rs#L1233-L1253

[JerryKwan]

@tnowacki In the current implementation of Move, the error result of cost_strategy .deduct_gas , that's to say PartialVMError { major_status: OUT_OF_GAS, sub_status: None, message: None, indices: [], offsets: [] } will never be converted to VMStatus::ExecutionFailure , it will be always VMStatus::Error, as you can see from the following code snippets https://github.com/move-language/move/blob/8958b40e97a4f94a588e5365fdb90ec18f00d888/language/move-binary-format/src/errors.rs#L86-L122

[tnowacki]

Gotcha, I'd be okay with saying then that its an execution error... or an out of gas error :)

[JerryKwan]

@tnowacki Any suggestions about how to correct the existing logic? We can remove or correct the assert_debug in https://github.com/move-language/move/blob/75fb97eee86966d2c8e6be96fe21da969250e36c/language/move-core/types/src/vm_status.rs#L133 or we can remove/correct the return statement in https://github.com/move-language/move/blob/8958b40e97a4f94a588e5365fdb90ec18f00d888/language/move-binary-format/src/errors.rs#L104 and https://github.com/move-language/move/blob/8958b40e97a4f94a588e5365fdb90ec18f00d888/language/move-binary-format/src/errors.rs#L109 Or we can add explicit StatusType named OutOfGas?

[JerryKwan]

@tnowacki Any suggestions about how we can handle this? thank you

[tnowacki]

Sorry yeah to be clear what I had meant. I'd be okay with with this debug_assert!(code.status_type() != StatusType::Execution || code == StatusCode::OUT_OF_GAS);

[tnowacki]

This would be consistent with the check in keep_or_discard

[JerryKwan]

@tnowacki Thanks for looking into this. That sounds reasonable.