Addressing reviewer comments

third_party/move/move-binary-format/src/deserializer.rs

@@ -1135,6 +1135,13 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult<Sign
             arity: usize,
             ty_args: Vec<SignatureToken>,
         },
+        Function {
+            abilities: AbilitySet,
+            arg_count: usize,
+            result_count: usize,
+            args: Vec<SignatureToken>,
+            results: Vec<SignatureToken>,
+        },
     }
 
     impl TypeBuilder {
@@ -1161,6 +1168,30 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult<Sign
                         }
                     }
                 },
+                T::Function {
+                    abilities,
+                    arg_count,
+                    result_count,
+                    mut args,
+                    mut results,
+                } => {
+                    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,19 +1212,27 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult<Sign
 
     let mut read_next = || {
         if let Ok(byte) = cursor.read_u8() {
-            match S::from_u8(byte)? {
-                S::U16 | S::U32 | S::U256 if (cursor.version() < VERSION_6) => {
+            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 {}",
                             cursor.version()
                         )),
                     );
                 },
+                S::FUNCTION if cursor.version() < VERSION_8 => {
+                    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<Sign
                     let idx = load_type_parameter_index(cursor)?;
                     T::Saturated(SignatureToken::TypeParameter(idx))
                 },
+                S::FUNCTION => {
+                    // 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)

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<Item = &'a T>,
+        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

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.

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(())

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(_)

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<AbstractValue>,

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::<PartialVMResult<Vec<_>>>()?;
-    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) => {

third_party/move/move-bytecode-verifier/src/signature_v2.rs

@@ -173,17 +173,10 @@ fn check_ty<const N: usize>(
                 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.)

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) => {