Address review comments: - use struct for SignatureToken::Function - …

…simplify AcuiresVerifier::ld_function_acquire - comments cleanup, especially file_format semantics for new bytecodes

api/types/src/bytecode.rs

@@ -105,7 +105,7 @@ pub trait Bytecode {
                 mutable: true,
                 to: Box::new(self.new_move_type(t.borrow())),
             },
-            SignatureToken::Function(..) => {
+            SignatureToken::Function { .. } => {
                 // TODO(LAMBDA)
                 unimplemented!("signature token function to API MoveType")
             },

aptos-move/script-composer/src/decompiler.rs

@@ -208,7 +208,7 @@ impl LocalState {
             SignatureToken::Vector(s) => {
                 TypeTag::Vector(Box::new(Self::type_tag_from_sig_token(script, s)?))
             },
-            SignatureToken::Function(..) => {
+            SignatureToken::Function { .. } => {
                 // TODO(LAMBDA)
                 bail!("function types not yet implemented for script composer")
             },

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

@@ -343,7 +343,7 @@ impl<'a> BinaryIndexedView<'a> {
             Vector(ty) => AbilitySet::polymorphic_abilities(AbilitySet::VECTOR, vec![false], vec![
                 self.abilities(ty, constraints)?,
             ]),
-            Function(_, _, abilities) => Ok(*abilities),
+            Function { abilities, .. } => Ok(*abilities),
             Struct(idx) => {
                 let sh = self.struct_handle_at(*idx);
                 Ok(sh.abilities)

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

@@ -235,11 +235,15 @@ impl CompiledScriptBuilder {
                 MutableReference(Box::new(self.import_signature_token(module, ty)?))
             },
             Vector(ty) => Vector(Box::new(self.import_signature_token(module, ty)?)),
-            Function(args, result, abilities) => Function(
-                import_vec(self, args)?,
-                import_vec(self, result)?,
-                *abilities,
-            ),
+            Function {
+                args,
+                results,
+                abilities,
+            } => Function {
+                args: import_vec(self, args)?,
+                results: import_vec(self, results)?,
+                abilities: *abilities,
+            },
             Struct(idx) => Struct(self.import_struct(module, *idx)?),
             StructInstantiation(idx, inst_tys) => StructInstantiation(
                 self.import_struct(module, *idx)?,

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

@@ -685,8 +685,20 @@ impl<'a> BoundsChecker<'a> {
 
         for ty in ty.preorder_traversal() {
             match ty {
-                Bool | U8 | U16 | U32 | U64 | U128 | U256 | Address | Signer | TypeParameter(_)
-                | Reference(_) | MutableReference(_) | Vector(_) | Function(..) => (),
+                Bool
+                | U8
+                | U16
+                | U32
+                | U64
+                | U128
+                | U256
+                | Address
+                | Signer
+                | TypeParameter(_)
+                | Reference(_)
+                | MutableReference(_)
+                | Vector(_)
+                | Function { .. } => (),
                 Struct(idx) => {
                     check_bounds_impl(self.view.struct_handles(), *idx)?;
                     if let Some(sh) = self.view.struct_handles().get(idx.into_index()) {

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

@@ -67,8 +67,20 @@ impl<'a> BinaryComplexityMeter<'a> {
                     cost = cost.saturating_add(struct_name.len() as u64 * COST_PER_IDENT_BYTE);
                     cost = cost.saturating_add(moduel_name.len() as u64 * COST_PER_IDENT_BYTE);
                 },
-                U8 | U16 | U32 | U64 | U128 | U256 | Signer | Address | Bool | Vector(_)
-                | Function(..) | TypeParameter(_) | Reference(_) | MutableReference(_) => (),
+                U8
+                | U16
+                | U32
+                | U64
+                | U128
+                | U256
+                | Signer
+                | Address
+                | Bool
+                | Vector(_)
+                | Function { .. }
+                | TypeParameter(_)
+                | Reference(_)
+                | MutableReference(_) => (),
             }
         }
 

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

@@ -17,7 +17,7 @@ fn sig_to_ty(sig: &SignatureToken) -> Option<MoveTypeLayout> {
         SignatureToken::U128 => Some(MoveTypeLayout::U128),
         SignatureToken::U256 => Some(MoveTypeLayout::U256),
         SignatureToken::Vector(v) => Some(MoveTypeLayout::Vector(Box::new(sig_to_ty(v.as_ref())?))),
-        SignatureToken::Function(..) => {
+        SignatureToken::Function { .. } => {
             // TODO(LAMBDA): do we need representation in MoveTypeLayout?
             None
         },

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

@@ -1262,7 +1262,11 @@ pub enum SignatureToken {
     /// Vector
     Vector(Box<SignatureToken>),
     /// Function, with n argument types and m result types, and an associated ability set.
-    Function(Vec<SignatureToken>, Vec<SignatureToken>, AbilitySet),
+    Function {
+        args: Vec<SignatureToken>,
+        results: Vec<SignatureToken>,
+        abilities: AbilitySet,
+    },
     /// User defined type
     Struct(StructHandleIndex),
     StructInstantiation(StructHandleIndex, Vec<SignatureToken>),
@@ -1305,9 +1309,9 @@ impl<'a> Iterator for SignatureTokenPreorderTraversalIter<'a> {
                         self.stack.extend(inner_toks.iter().rev())
                     },
 
-                    Function(args, result, _) => {
+                    Function { args, results, .. } => {
                         self.stack.extend(args.iter().rev());
-                        self.stack.extend(result.iter().rev());
+                        self.stack.extend(results.iter().rev());
                     },
 
                     Signer | Bool | Address | U8 | U16 | U32 | U64 | U128 | U256 | Struct(_)
@@ -1343,11 +1347,11 @@ impl<'a> Iterator for SignatureTokenPreorderTraversalIterWithDepth<'a> {
                         .stack
                         .extend(inner_toks.iter().map(|tok| (tok, depth + 1)).rev()),
 
-                    Function(args, result, _) => {
+                    Function { args, results, .. } => {
                         self.stack
                             .extend(args.iter().map(|tok| (tok, depth + 1)).rev());
                         self.stack
-                            .extend(result.iter().map(|tok| (tok, depth + 1)).rev());
+                            .extend(results.iter().map(|tok| (tok, depth + 1)).rev());
                     },
 
                     Signer | Bool | Address | U8 | U16 | U32 | U64 | U128 | U256 | Struct(_)
@@ -1410,8 +1414,12 @@ impl std::fmt::Debug for SignatureToken {
             SignatureToken::Address => write!(f, "Address"),
             SignatureToken::Signer => write!(f, "Signer"),
             SignatureToken::Vector(boxed) => write!(f, "Vector({:?})", boxed),
-            SignatureToken::Function(args, result, abilities) => {
-                write!(f, "Function({:?}, {:?}, {})", args, result, abilities)
+            SignatureToken::Function {
+                args,
+                results,
+                abilities,
+            } => {
+                write!(f, "Function({:?}, {:?}, {})", args, results, abilities)
             },
             SignatureToken::Reference(boxed) => write!(f, "Reference({:?})", boxed),
             SignatureToken::Struct(idx) => write!(f, "Struct({:?})", idx),
@@ -1425,7 +1433,7 @@ impl std::fmt::Debug for SignatureToken {
 }
 
 impl SignatureToken {
-    // Returns `true` if the `SignatureToken` is an integer type.
+    /// Returns `true` if the `SignatureToken` is an integer type.
     pub fn is_integer(&self) -> bool {
         use SignatureToken::*;
         match self {
@@ -1434,7 +1442,7 @@ impl SignatureToken {
             | Address
             | Signer
             | Vector(_)
-            | Function(..)
+            | Function { .. }
             | Struct(_)
             | StructInstantiation(_, _)
             | Reference(_)
@@ -1473,7 +1481,7 @@ impl SignatureToken {
             Bool | U8 | U16 | U32 | U64 | U128 | U256 | Address => true,
             Vector(inner) => inner.is_valid_for_constant(),
             Signer
-            | Function(..)
+            | Function { .. }
             | Struct(_)
             | StructInstantiation(_, _)
             | Reference(_)
@@ -1486,7 +1494,7 @@ impl SignatureToken {
     pub fn is_function(&self) -> bool {
         use SignatureToken::*;
 
-        matches!(self, Function(..))
+        matches!(self, Function { .. })
     }
 
     /// Set the index to this one. Useful for random testing.
@@ -1537,8 +1545,14 @@ impl SignatureToken {
             Address => Address,
             Signer => Signer,
             Vector(ty) => Vector(Box::new(ty.instantiate(subst_mapping))),
-            Function(args, result, abilities) => {
-                Function(inst_vec(args), inst_vec(result), *abilities)
+            Function {
+                args,
+                results,
+                abilities,
+            } => Function {
+                args: inst_vec(args),
+                results: inst_vec(results),
+                abilities: *abilities,
             },
             Struct(idx) => Struct(*idx),
             StructInstantiation(idx, struct_type_args) => {
@@ -1834,6 +1848,11 @@ pub enum Bytecode {
         // Here `func` is loaded from the file format, containing information like the
         // the function signature, the locals, and the body.
 
+        // TODO(LAMBDA) - try to loosen this restriction after MVP if we can
+        // prove safety using access specifiers or other mechanisms.
+        if conservative_invocation_mode then
+            if stack already contains a call to the same module as func, then abort
+
         ty_args = if func.is_generic then func.ty_args else []
 
         n = func.num_params
@@ -3047,8 +3066,13 @@ pub enum Bytecode {
         Return values are pushed onto the stack from the first to the last and
         available to the caller after returning from the callee.
 
-        During execution of the function conservative_invocation_mode is enabled;
-        afterwards, it is restored to its original state.
+        For MVP, we dynamically disallow all module reentrancy starting with
+        any dynamic call.  This means that normal calls need a dynamic stack check
+        if there is a dynamic call frame on the stack.  We denote this here
+        using a "conservative_invocation_mode" state variable.
+
+        During execution of the dynamic function conservative_invocation_mode
+        is enabled; afterwards, it is restored to its original state.
     "#]
     #[semantics = r#"
         stack >> function_handle
@@ -3058,6 +3082,10 @@ pub enum Bytecode {
         n = func.num_params
         ty_args = if func.is_generic then func.ty_args else []
 
+        // TODO(LAMBDA) - try to loosen this restriction after MVP if we can
+        // prove safety using access specifiers or other mechanisms.
+        if stack already contains a call to the same module as func, then abort
+
         n = func.num_params
         stack >> arg_{n-1}
         ..

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

@@ -193,7 +193,7 @@ impl Type {
             Reference(t) => Type::Reference(Box::new(Type::new(m, t))),
             MutableReference(t) => Type::MutableReference(Box::new(Type::new(m, t))),
 
-            Function(..) => panic!("normalized representation does not support function types"),
+            Function { .. } => panic!("normalized representation does not support function types"),
         }
     }
 

third_party/move/move-binary-format/src/proptest_types/functions.rs

@@ -1061,8 +1061,18 @@ impl BytecodeGen {
     fn check_signature_token(token: &SignatureToken) -> bool {
         use SignatureToken::*;
         match token {
-            U8 | U16 | U32 | U64 | U128 | U256 | Bool | Address | Signer | Struct(_)
-            | Function(..) | TypeParameter(_) => true,
+            U8
+            | U16
+            | U32
+            | U64
+            | U128
+            | U256
+            | Bool
+            | Address
+            | Signer
+            | Struct(_)
+            | Function { .. }
+            | TypeParameter(_) => true,
             Vector(element_token) => BytecodeGen::check_signature_token(element_token),
             StructInstantiation(_, type_arguments) => type_arguments
                 .iter()

third_party/move/move-binary-format/src/proptest_types/types.rs

@@ -71,7 +71,7 @@ impl StDefnMaterializeState {
                 let inner = self.potential_abilities(ty);
                 inner.intersect(AbilitySet::VECTOR)
             },
-            Function(_, _, a) => *a,
+            Function { abilities, .. } => *abilities,
             Struct(idx) => {
                 let sh = &self.struct_handles[idx.0 as usize];
                 sh.abilities

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

@@ -800,7 +800,7 @@ fn serialize_signature_token_single_node_impl(
             binary.push(SerializedType::TYPE_PARAMETER as u8)?;
             serialize_type_parameter_index(binary, *idx)?;
         },
-        SignatureToken::Function(..) => {
+        SignatureToken::Function { .. } => {
             unimplemented!("serialization of function types")
         },
     }

third_party/move/move-bytecode-verifier/invalid-mutations/src/bounds.rs

@@ -358,7 +358,17 @@ fn struct_handle(token: &SignatureToken) -> Option<StructHandleIndex> {
         Struct(sh_idx) => Some(*sh_idx),
         StructInstantiation(sh_idx, _) => Some(*sh_idx),
         Reference(token) | MutableReference(token) => struct_handle(token),
-        Bool | U8 | U16 | U32 | U64 | U128 | U256 | Address | Signer | Vector(_)
-        | TypeParameter(_) | Function(..) => None,
+        Bool
+        | U8
+        | U16
+        | U32
+        | U64
+        | U128
+        | U256
+        | Address
+        | Signer
+        | Vector(_)
+        | TypeParameter(_)
+        | Function { .. } => None,
     }
 }

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

@@ -212,23 +212,20 @@ impl<'a> AcquiresVerifier<'a> {
 
     fn ld_function_acquire(
         &mut self,
-        fh_idx: FunctionHandleIndex,
-        offset: CodeOffset,
+        _fh_idx: FunctionHandleIndex,
+        _offset: CodeOffset,
     ) -> PartialVMResult<()> {
-        // Currenty we are disallowing acquires for any function value which
-        // is created, so InvokeFunction does nothing with acquires.
+        // Note that function values are dynamically disallowed to be called from the module hosting
+        // their implementation function, and to be called while their host module is on the stack,
+        // so we don't need to do anything about acquires at this point.
         // TODO(LAMBDA) In the future this may change.
-        let function_handle = self.module.function_handle_at(fh_idx);
-        let function_acquired_resources = self.function_acquired_resources(function_handle, fh_idx);
-        if !function_acquired_resources.is_empty() {
-            return Err(self.error(StatusCode::LD_FUNCTION_NONEMPTY_ACQUIRES, offset));
-        }
         Ok(())
     }
 
     fn invoke_acquire(&mut self, _offset: CodeOffset) -> PartialVMResult<()> {
-        // Currenty we are disallowing acquires for any function value which
-        // is created, so InvokeFunction does nothing with acquires.
+        // Note that function values are dynamically disallowed to be called from their
+        // hosting module, or if that module appears on the stack at all.  This means
+        // that a dynamic call cannot affect local resources in the caller.
         // TODO(LAMBDA) In the future this may change.
         Ok(())
     }

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

@@ -456,13 +456,22 @@ fn compare_types(
             compare_types(context, ty1, ty2, def_module)
         },
         (
-            SignatureToken::Function(args1, result1, ab1),
-            SignatureToken::Function(args2, result2, ab2),
+            SignatureToken::Function {
+                args: args1,
+                results: result1,
+                abilities: abilities1,
+            },
+            SignatureToken::Function {
+                args: args2,
+                results: result2,
+                abilities: abilities2,
+            },
         ) => {
             compare_cross_module_signatures(context, args1, args2, def_module)?;
             compare_cross_module_signatures(context, result1, result2, def_module)?;
-            if ab1 != ab2 {
-                Err(PartialVMError::new(StatusCode::TYPE_MISMATCH))
+            if abilities1 != abilities2 {
+                Err(PartialVMError::new(StatusCode::TYPE_MISMATCH)
+                    .with_message("Function type ability constraints mismatch".to_string()))
             } else {
                 Ok(())
             }
@@ -495,7 +504,7 @@ fn compare_types(
         | (SignatureToken::Address, _)
         | (SignatureToken::Signer, _)
         | (SignatureToken::Vector(_), _)
-        | (SignatureToken::Function(..), _)
+        | (SignatureToken::Function { .. }, _)
         | (SignatureToken::Struct(_), _)
         | (SignatureToken::StructInstantiation(_, _), _)
         | (SignatureToken::Reference(_), _)

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

@@ -148,11 +148,11 @@ impl<'a> InstantiationLoopChecker<'a> {
                     type_params.insert(*idx);
                 },
                 Vector(ty) => rec(type_params, ty),
-                Function(args, result, _) => {
+                Function { args, results, .. } => {
                     for ty in args {
                         rec(type_params, ty);
                     }
-                    for ty in result {
+                    for ty in results {
                         rec(type_params, ty);
                     }
                 },