From 53b97a8eeb9c0a206e57f26313d8c00cb92a1ba0 Mon Sep 17 00:00:00 2001 From: Wolfgang Grieskamp Date: Fri, 17 Jan 2025 19:02:00 -0800 Subject: [PATCH] Check version when deserializing closure opcodes. --- .../move-binary-format/src/deserializer.rs | 10 +++ .../move-binary-format/src/file_format.rs | 74 ++++++++----------- .../src/file_format_common.rs | 3 +- .../src/signature_v2.rs | 2 +- .../move-bytecode-verifier/src/type_safety.rs | 8 +- .../move/move-core/types/src/vm_status.rs | 2 +- .../src/stackless_bytecode_generator.rs | 1 + third_party/move/move-model/src/ty.rs | 2 +- .../move/move-vm/runtime/src/interpreter.rs | 2 +- .../move-vm/runtime/src/loader/type_loader.rs | 2 +- .../runtime/src/runtime_type_checks.rs | 4 +- .../move-disassembler/src/disassembler.rs | 3 +- .../tools/move-resource-viewer/src/lib.rs | 1 + 13 files changed, 60 insertions(+), 54 deletions(-) diff --git a/third_party/move/move-binary-format/src/deserializer.rs b/third_party/move/move-binary-format/src/deserializer.rs index 1f8712f994ba6a..ba8bf00ef5b790 100644 --- a/third_party/move/move-binary-format/src/deserializer.rs +++ b/third_party/move/move-binary-format/src/deserializer.rs @@ -1650,6 +1650,16 @@ fn load_code(cursor: &mut VersionedCursor, code: &mut Vec) -> BinaryLo )), ); }, + Opcodes::PACK_CLOSURE | Opcodes::PACK_CLOSURE_GENERIC | Opcodes::CALL_CLOSURE + if cursor.version() < VERSION_8 => + { + return Err( + PartialVMError::new(StatusCode::MALFORMED).with_message(format!( + "Closure operations not available before bytecode version {}", + VERSION_8 + )), + ); + }, _ => {}, }; diff --git a/third_party/move/move-binary-format/src/file_format.rs b/third_party/move/move-binary-format/src/file_format.rs index 5cd43f451b224d..84e0d8b7ae494d 100644 --- a/third_party/move/move-binary-format/src/file_format.rs +++ b/third_party/move/move-binary-format/src/file_format.rs @@ -49,7 +49,6 @@ use proptest::{collection::vec, prelude::*, strategy::BoxedStrategy}; use ref_cast::RefCast; use serde::{Deserialize, Serialize}; use std::{ - collections::BTreeMap, fmt::{self, Formatter}, ops::BitOr, }; @@ -1568,16 +1567,14 @@ impl ClosureMask { /// Apply a closure mask to a list of elements, returning only those /// where position `i` is set in the mask (if `collect_captured` is true) or not /// set (otherwise). - pub fn extract(&self, tys: &[T], collect_captured: bool) -> Vec { - tys.iter() - .enumerate() - .filter_map(|(pos, x)| { - let set = (1 << pos) & self.mask != 0; - if set && collect_captured || !set && !collect_captured { - Some(x.clone()) - } else { - None - } + pub fn extract<'a, T: Clone>(&self, values: &'a [T], collect_captured: bool) -> Vec<&'a T> { + let mut mask = self.mask; + values + .iter() + .filter(|_| { + let set = mask & 0x1 != 0; + mask >>= 1; + set && collect_captured || !set && !collect_captured }) .collect() } @@ -1590,39 +1587,29 @@ impl ClosureMask { /// This returns `None` if the provided lists are inconsistent w.r.t the mask /// and cannot be composed. This should not happen in verified code, but /// a caller should decide whether to crash or to error. - pub fn compose(&self, captured: &[T], provided: &[T]) -> Option> { - let mut result = BTreeMap::new(); // expect ordered enumeration - let mut cap_idx = 0; - let mut pro_idx = 0; - for i in 0..64 { - if cap_idx >= captured.len() && pro_idx >= provided.len() { - // all covered - break; - } - if (1 << i) & self.mask != 0 { - if cap_idx >= captured.len() { - // Inconsistency - return None; - } - result.insert(i, captured[cap_idx].clone()); - cap_idx += 1 + pub fn compose( + &self, + captured: impl IntoIterator, + provided: impl IntoIterator, + ) -> Option> { + let mut captured = captured.into_iter(); + let mut provided = provided.into_iter(); + let mut result = vec![]; + let mut mask = self.mask; + while mask != 0 { + if mask & 0x1 != 0 { + result.push(captured.next()?) } else { - if pro_idx >= provided.len() { - // Inconsistency - return None; - } - result.insert(i, provided[pro_idx].clone()); - pro_idx += 1 + result.push(provided.next()?) } + mask >>= 1; } - let map_len = result.len(); - let vec = result.into_values().collect::>(); - if vec.len() != map_len { - // Inconsistency: all indices must be contiguously covered - None - } else { - Some(vec) + if captured.next().is_some() { + // Not all captured arguments consumed + return None; } + result.extend(provided); + Some(result) } /// Return the max index of captured arguments @@ -3106,8 +3093,8 @@ pub enum Bytecode { #[group = "closure"] #[description = r#" - `ClosEval(|t1..tn|r has a)` evalutes a closure of the given function type, taking - the captured arguments and mixing in the provided ones on the stack. + `CallClosure(|t1..tn|r has a)` evalutes a closure of the given function type, + taking the captured arguments and mixing in the provided ones on the stack. On top of the stack is the closure being evaluated, underneath the arguments: `[c,vn,..,v1] + stack`. The type of the closure must match the type specified in @@ -3122,7 +3109,8 @@ pub enum Bytecode { The semantics of this instruction can be characterized by the following equation: ``` - CloseEval(ClosPack(f, mask, c1..cn), a1..am) = f(mask.compose(c1..cn, a1..am)) + CallClosure(PackClosure(f, mask, c1..cn), a1..am) == + f(mask.compose(c1..cn, a1..am)) ``` "#] #[static_operands = "[]"] diff --git a/third_party/move/move-binary-format/src/file_format_common.rs b/third_party/move/move-binary-format/src/file_format_common.rs index 853cf8a8235369..a320a891480c3f 100644 --- a/third_party/move/move-binary-format/src/file_format_common.rs +++ b/third_party/move/move-binary-format/src/file_format_common.rs @@ -299,6 +299,7 @@ pub enum Opcodes { UNPACK_VARIANT_GENERIC = 0x55, TEST_VARIANT = 0x56, TEST_VARIANT_GENERIC = 0x57, + // Since bytecode version 8 PACK_CLOSURE = 0x58, PACK_CLOSURE_GENERIC = 0x59, CALL_CLOSURE = 0x5A, @@ -513,7 +514,7 @@ pub const VERSION_6: u32 = 6; pub const VERSION_7: u32 = 7; /// Version 8: changes compared to version 7 -/// + TBD +/// + closure instructions pub const VERSION_8: u32 = 8; /// Mark which version is the latest version. Should be set to v8 once features diff --git a/third_party/move/move-bytecode-verifier/src/signature_v2.rs b/third_party/move/move-bytecode-verifier/src/signature_v2.rs index 77ce9de131fa81..503f87a0cbb5a5 100644 --- a/third_party/move/move-bytecode-verifier/src/signature_v2.rs +++ b/third_party/move/move-bytecode-verifier/src/signature_v2.rs @@ -902,7 +902,7 @@ impl<'a, const N: usize> SignatureChecker<'a, N> { let sgn = self.resolver.signature_at(*idx); if sgn.len() != 1 || !matches!(&sgn.0[0], SignatureToken::Function(..)) { return map_err(Err(PartialVMError::new( - StatusCode::CLOSURE_EVAL_REQUIRES_FUNCTION, + StatusCode::CLOSURE_CALL_REQUIRES_FUNCTION, ) .with_message("expected a function type for closure call".to_string()))); } diff --git a/third_party/move/move-bytecode-verifier/src/type_safety.rs b/third_party/move/move-bytecode-verifier/src/type_safety.rs index 30db544a6f8160..8c5e7fd206648b 100644 --- a/third_party/move/move-bytecode-verifier/src/type_safety.rs +++ b/third_party/move/move-bytecode-verifier/src/type_safety.rs @@ -360,7 +360,7 @@ fn clos_pack( let param_sgn = verifier.resolver.signature_at(func_handle.parameters); let captured_param_tys = mask.extract(¶m_sgn.0, true); let mut abilities = AbilitySet::ALL; - for ty in captured_param_tys.iter().rev() { + for ty in captured_param_tys.into_iter().rev() { abilities = abilities.intersect(verifier.abilities(ty)?); let arg = safe_unwrap!(verifier.stack.pop()); if (type_actuals.is_empty() && &arg != ty) @@ -402,7 +402,11 @@ fn clos_pack( abilities.remove(Ability::Key); // Construct the resulting function type - let not_captured_param_tys = mask.extract(¶m_sgn.0, false); + let not_captured_param_tys = mask + .extract(¶m_sgn.0, false) + .into_iter() + .cloned() + .collect(); let ret_sign = verifier.resolver.signature_at(func_handle.return_); verifier.push( meter, diff --git a/third_party/move/move-core/types/src/vm_status.rs b/third_party/move/move-core/types/src/vm_status.rs index 61781f736c4804..37ddf6d8ec9446 100644 --- a/third_party/move/move-core/types/src/vm_status.rs +++ b/third_party/move/move-core/types/src/vm_status.rs @@ -747,7 +747,7 @@ pub enum StatusCode { // Closure mask invalid INVALID_CLOSURE_MASK = 1132, // Closure eval type is not a function - CLOSURE_EVAL_REQUIRES_FUNCTION = 1133, + CLOSURE_CALL_REQUIRES_FUNCTION = 1133, // Reserved error code for future use RESERVED_VERIFICATION_ERROR_2 = 1134, diff --git a/third_party/move/move-model/bytecode/src/stackless_bytecode_generator.rs b/third_party/move/move-model/bytecode/src/stackless_bytecode_generator.rs index 18d59dcdf3eeac..05de57449c1dfd 100644 --- a/third_party/move/move-model/bytecode/src/stackless_bytecode_generator.rs +++ b/third_party/move/move-model/bytecode/src/stackless_bytecode_generator.rs @@ -296,6 +296,7 @@ impl<'a> StacklessBytecodeGenerator<'a> { MoveBytecode::PackClosure(..) | MoveBytecode::PackClosureGeneric(..) | MoveBytecode::CallClosure(..) => { + // TODO(#15664): implement for closures unimplemented!("stackless bytecode generation for closure opcodes") }, MoveBytecode::Pop => { diff --git a/third_party/move/move-model/src/ty.rs b/third_party/move/move-model/src/ty.rs index 6da025ea9c482e..9ab3ce35bbff7f 100644 --- a/third_party/move/move-model/src/ty.rs +++ b/third_party/move/move-model/src/ty.rs @@ -1395,7 +1395,7 @@ impl Type { ) }, SignatureToken::Function(..) => { - // TODO: implement function conversion + // TODO(#15664): implement function conversion unimplemented!("signature token to model type") }, } diff --git a/third_party/move/move-vm/runtime/src/interpreter.rs b/third_party/move/move-vm/runtime/src/interpreter.rs index 560112a45d266e..f20f3b3b0cd98e 100644 --- a/third_party/move/move-vm/runtime/src/interpreter.rs +++ b/third_party/move/move-vm/runtime/src/interpreter.rs @@ -1728,7 +1728,7 @@ impl Frame { )?; match instruction { - // TODO: implement closures + // TODO(#15664): implement closures Bytecode::PackClosure(..) | Bytecode::PackClosureGeneric(..) | Bytecode::CallClosure(..) => { diff --git a/third_party/move/move-vm/runtime/src/loader/type_loader.rs b/third_party/move/move-vm/runtime/src/loader/type_loader.rs index 1aa31dcd377ea7..7f978379681f30 100644 --- a/third_party/move/move-vm/runtime/src/loader/type_loader.rs +++ b/third_party/move/move-vm/runtime/src/loader/type_loader.rs @@ -32,7 +32,7 @@ pub fn intern_type( Type::Vector(TriompheArc::new(inner_type)) }, SignatureToken::Function(..) => { - // TODO: implement closures + // TODO(#15664): implement closures return Err(PartialVMError::new(StatusCode::UNIMPLEMENTED_FEATURE) .with_message("function types in the type loader".to_owned())); }, diff --git a/third_party/move/move-vm/runtime/src/runtime_type_checks.rs b/third_party/move/move-vm/runtime/src/runtime_type_checks.rs index 7fc950cde81497..fdb8b93bd4b2b1 100644 --- a/third_party/move/move-vm/runtime/src/runtime_type_checks.rs +++ b/third_party/move/move-vm/runtime/src/runtime_type_checks.rs @@ -120,7 +120,7 @@ impl RuntimeTypeCheck for FullRuntimeTypeCheck { instruction: &Bytecode, ) -> PartialVMResult<()> { match instruction { - // TODO: implement closures + // TODO(#15664): implement closures Bytecode::PackClosure(..) | Bytecode::PackClosureGeneric(..) | Bytecode::CallClosure(..) => { @@ -254,7 +254,7 @@ impl RuntimeTypeCheck for FullRuntimeTypeCheck { let ty_builder = resolver.loader().ty_builder(); match instruction { - // TODO: implement closures + // TODO(#15664): implement closures Bytecode::PackClosure(..) | Bytecode::PackClosureGeneric(..) | Bytecode::CallClosure(..) => { diff --git a/third_party/move/tools/move-disassembler/src/disassembler.rs b/third_party/move/tools/move-disassembler/src/disassembler.rs index eee4338aa9459e..d2a8030423905c 100644 --- a/third_party/move/tools/move-disassembler/src/disassembler.rs +++ b/third_party/move/tools/move-disassembler/src/disassembler.rs @@ -570,7 +570,7 @@ impl<'a> Disassembler<'a> { type_param_context: &[SourceName], ) -> Result { Ok(match sig_tok { - // TODO: function types + // TODO(#15664): function types SignatureToken::Function(..) => unimplemented!("disassembling function sig tokens"), SignatureToken::Bool => "bool".to_string(), @@ -647,6 +647,7 @@ impl<'a> Disassembler<'a> { Bytecode::PackClosure(..) | Bytecode::PackClosureGeneric(..) | Bytecode::CallClosure(..) => { + // TODO(#15664): implement bail!("closure opcodes not implemented") }, Bytecode::LdConst(idx) => { diff --git a/third_party/move/tools/move-resource-viewer/src/lib.rs b/third_party/move/tools/move-resource-viewer/src/lib.rs index ba6d7f3eca5151..4fb38b0ae6eab3 100644 --- a/third_party/move/tools/move-resource-viewer/src/lib.rs +++ b/third_party/move/tools/move-resource-viewer/src/lib.rs @@ -376,6 +376,7 @@ impl MoveValueAnnotator { FatType::Vector(Box::new(self.resolve_signature(module, ty, limit)?)) }, SignatureToken::Function(..) => { + // TODO(#15664): implement bail!("function types NYI by fat types") }, SignatureToken::Struct(idx) => {