From c9c8a924c98456e28a68301e9cf0d35911a9e5dc Mon Sep 17 00:00:00 2001 From: Wolfgang Grieskamp Date: Wed, 22 Jan 2025 19:42:19 -0800 Subject: [PATCH] Addressing reviewer comments --- .../move-binary-format/src/deserializer.rs | 66 +++++++++++++++++-- .../move-binary-format/src/file_format.rs | 28 ++++---- .../src/file_format_common.rs | 1 + .../move/move-binary-format/src/serializer.rs | 9 ++- .../src/acquires_list_verifier.rs | 3 + .../src/reference_safety/abstract_state.rs | 2 +- .../src/reference_safety/mod.rs | 6 +- .../src/signature_v2.rs | 15 ++--- .../move-bytecode-verifier/src/type_safety.rs | 4 +- 9 files changed, 99 insertions(+), 35 deletions(-) diff --git a/third_party/move/move-binary-format/src/deserializer.rs b/third_party/move/move-binary-format/src/deserializer.rs index ba8bf00ef5b790..0946cac7ab7d34 100644 --- a/third_party/move/move-binary-format/src/deserializer.rs +++ b/third_party/move/move-binary-format/src/deserializer.rs @@ -1135,6 +1135,13 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult, }, + Function { + abilities: AbilitySet, + arg_count: usize, + result_count: usize, + args: Vec, + results: Vec, + }, } impl TypeBuilder { @@ -1161,6 +1168,30 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult { + if args.len() < arg_count { + args.push(tok) + } else { + results.push(tok) + } + if args.len() == arg_count && results.len() == result_count { + T::Saturated(SignatureToken::Function(args, results, abilities)) + } else { + T::Function { + abilities, + arg_count, + result_count, + args, + results, + } + } + }, _ => unreachable!("invalid type constructor application"), } } @@ -1181,8 +1212,9 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult { + let ser_type = S::from_u8(byte)?; + match ser_type { + S::U16 | S::U32 | S::U256 if cursor.version() < VERSION_6 => { return Err( PartialVMError::new(StatusCode::MALFORMED).with_message(format!( "u16, u32, u256 integers not supported in bytecode version {}", @@ -1190,10 +1222,17 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult { + return Err( + PartialVMError::new(StatusCode::MALFORMED).with_message(format!( + "function types not supported in bytecode version {}", + cursor.version() + )), + ); + }, _ => (), }; - - Ok(match S::from_u8(byte)? { + Ok(match ser_type { S::BOOL => T::Saturated(SignatureToken::Bool), S::U8 => T::Saturated(SignatureToken::U8), S::U16 => T::Saturated(SignatureToken::U16), @@ -1227,6 +1266,25 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult { + // The legacy ability set position is only for older bytecode versions, + // still choosing StructTypeParameters matches what functions can have. + let abilities = + load_ability_set(cursor, AbilitySetPosition::StructTypeParameters)?; + let arg_count = load_type_parameter_count(cursor)?; + let result_count = load_type_parameter_count(cursor)?; + if arg_count + result_count == 0 { + T::Saturated(SignatureToken::Function(vec![], vec![], abilities)) + } else { + T::Function { + abilities, + arg_count, + result_count, + args: vec![], + results: vec![], + } + } + }, }) } else { Err(PartialVMError::new(StatusCode::MALFORMED) 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 21f76ea300f9b5..64a6e35b6c7b21 100644 --- a/third_party/move/move-binary-format/src/file_format.rs +++ b/third_party/move/move-binary-format/src/file_format.rs @@ -1550,28 +1550,34 @@ impl SignatureToken { derive(arbitrary::Arbitrary), derive(dearbitrary::Dearbitrary) )] -pub struct ClosureMask { - pub mask: u64, -} +pub struct ClosureMask(u64); impl fmt::Display for ClosureMask { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{:b}", self.mask) + write!(f, "{:b}", self.0) } } impl ClosureMask { - pub fn new(mask: u64) -> Self { - Self { mask } + pub fn new(bits: u64) -> Self { + Self(bits) + } + + pub fn bits(&self) -> u64 { + self.0 } /// 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<'a, T: Clone>(&self, values: &'a [T], collect_captured: bool) -> Vec<&'a T> { - let mut mask = self.mask; + pub fn extract<'a, T: Clone>( + &self, + values: impl IntoIterator, + collect_captured: bool, + ) -> Vec<&'a T> { + let mut mask = self.0; values - .iter() + .into_iter() .filter(|_| { let set = mask & 0x1 != 0; mask >>= 1; @@ -1596,7 +1602,7 @@ impl ClosureMask { let mut captured = captured.into_iter(); let mut provided = provided.into_iter(); let mut result = vec![]; - let mut mask = self.mask; + let mut mask = self.0; while mask != 0 { if mask & 0x1 != 0 { result.push(captured.next()?) @@ -1616,7 +1622,7 @@ impl ClosureMask { /// Return the max index of captured arguments pub fn max_captured(&self) -> usize { let mut i = 0; - let mut mask = self.mask; + let mut mask = self.0; while mask != 0 { mask >>= 1; i += 1 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 c40f1e19986741..d6e6ec725ba4bd 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 @@ -140,6 +140,7 @@ pub enum SerializedType { U16 = 0xD, U32 = 0xE, U256 = 0xF, + FUNCTION = 0x10 } /// A marker for an option in the serialized output. diff --git a/third_party/move/move-binary-format/src/serializer.rs b/third_party/move/move-binary-format/src/serializer.rs index fc412a6f0046a2..e74ed1be8b916f 100644 --- a/third_party/move/move-binary-format/src/serializer.rs +++ b/third_party/move/move-binary-format/src/serializer.rs @@ -161,7 +161,7 @@ fn serialize_struct_def_inst_index( } fn serialize_closure_mask(binary: &mut BinaryData, mask: &ClosureMask) -> Result<()> { - write_as_uleb128(binary, mask.mask, u64::MAX) + write_as_uleb128(binary, mask.bits(), u64::MAX) } fn seiralize_table_offset(binary: &mut BinaryData, offset: u32) -> Result<()> { @@ -804,8 +804,11 @@ fn serialize_signature_token_single_node_impl( binary.push(SerializedType::TYPE_PARAMETER as u8)?; serialize_type_parameter_index(binary, *idx)?; }, - SignatureToken::Function(..) => { - unimplemented!("serialization of function types") + SignatureToken::Function(args, results, abilities) => { + binary.push(SerializedType::FUNCTION as u8)?; + serialize_ability_set(binary, *abilities)?; + serialize_signature_size(binary, args.len())?; + serialize_signature_size(binary, results.len())?; }, } Ok(()) diff --git a/third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs b/third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs index d0fc627a63e57c..0c9f7daf119222 100644 --- a/third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs +++ b/third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs @@ -102,6 +102,9 @@ impl<'a> AcquiresVerifier<'a> { self.struct_acquire(si.def, offset) }, + // Note that closure pack operation do not acquire resources; these are acquired + // when the function is called later, and acquires check at this point of time + // happen dynamically at runtime. Bytecode::PackClosure(..) | Bytecode::PackClosureGeneric(..) | Bytecode::CallClosure(_) diff --git a/third_party/move/move-bytecode-verifier/src/reference_safety/abstract_state.rs b/third_party/move/move-bytecode-verifier/src/reference_safety/abstract_state.rs index 83cd057332a22b..c07ecfa8ba564e 100644 --- a/third_party/move/move-bytecode-verifier/src/reference_safety/abstract_state.rs +++ b/third_party/move/move-bytecode-verifier/src/reference_safety/abstract_state.rs @@ -570,7 +570,7 @@ impl AbstractState { /// Records the evaluation of a closure in the abstract state. This is currently the /// same as calling the function. - pub fn clos_eval( + pub fn call_closure( &mut self, offset: CodeOffset, arguments: Vec, diff --git a/third_party/move/move-bytecode-verifier/src/reference_safety/mod.rs b/third_party/move/move-bytecode-verifier/src/reference_safety/mod.rs index cead8a230807ab..272d8908704a35 100644 --- a/third_party/move/move-bytecode-verifier/src/reference_safety/mod.rs +++ b/third_party/move/move-bytecode-verifier/src/reference_safety/mod.rs @@ -118,7 +118,7 @@ fn clos_pack( Ok(()) } -fn clos_eval( +fn call_closure( verifier: &mut ReferenceSafetyAnalysis, state: &mut AbstractState, offset: CodeOffset, @@ -131,7 +131,7 @@ fn clos_eval( .map(|_| Ok(safe_unwrap!(verifier.stack.pop()))) .rev() .collect::>>()?; - let values = state.clos_eval(offset, arguments, &result_tys, meter)?; + let values = state.call_closure(offset, arguments, &result_tys, meter)?; for value in values { verifier.stack.push(value) } @@ -558,7 +558,7 @@ fn execute_inner( }, Bytecode::CallClosure(idx) => { let (arg_tys, result_tys) = fun_type(verifier, *idx)?; - clos_eval(verifier, state, offset, arg_tys, result_tys, meter)? + call_closure(verifier, state, offset, arg_tys, result_tys, meter)? }, Bytecode::VecPack(idx, num) => { 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 503f87a0cbb5a5..2186311763a198 100644 --- a/third_party/move/move-bytecode-verifier/src/signature_v2.rs +++ b/third_party/move/move-bytecode-verifier/src/signature_v2.rs @@ -173,17 +173,10 @@ fn check_ty( param_constraints, )?; }, - Function(args, result, abilities) => { + Function(_, _, abilities) => { assert_abilities(*abilities, required_abilities)?; - for ty in args.iter().chain(result.iter()) { - check_ty( - struct_handles, - ty, - false, - required_abilities.requires(), - param_constraints, - )?; - } + // Note we do not need to check abilities of argument or result types, + // they do not matter for the `required_abilities`. }, Struct(sh_idx) => { let handle = &struct_handles[sh_idx.0 as usize]; @@ -798,7 +791,7 @@ impl<'a, const N: usize> SignatureChecker<'a, N> { Ok(()) } - /// Checks if a code unit is well-formed. + /// Checks if a code unit is well-formed. This expects signature checker to be run. /// /// A code unit is well-formed if /// - The locals are well-formed within the context. (References are allowed.) 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 ca0ff40790a44d..418967a21942e3 100644 --- a/third_party/move/move-bytecode-verifier/src/type_safety.rs +++ b/third_party/move/move-bytecode-verifier/src/type_safety.rs @@ -301,7 +301,7 @@ fn call( Ok(()) } -fn clos_eval( +fn call_closure( verifier: &mut TypeSafetyChecker, meter: &mut impl Meter, offset: CodeOffset, @@ -853,7 +853,7 @@ fn verify_instr( Bytecode::CallClosure(idx) => { // The signature checker has verified this is a function type. let expected_ty = safe_unwrap!(verifier.resolver.signature_at(*idx).0.first()); - clos_eval(verifier, meter, offset, expected_ty)? + call_closure(verifier, meter, offset, expected_ty)? }, Bytecode::Pack(idx) => {