[move-vm] Fixes to enum type implementation (#14657)

aptos-move/framework/src/module_metadata.rs

@@ -624,11 +624,23 @@ fn check_module_complexity(module: &CompiledModule) -> Result<(), MetaDataValida
         check_ident_complexity(module, &mut meter, handle.name)?;
     }
     for def in module.struct_defs() {
-        if let StructFieldInformation::Declared(fields) = &def.field_information {
-            for field in fields {
-                check_ident_complexity(module, &mut meter, field.name)?;
-                check_sigtok_complexity(module, &mut meter, &field.signature.0)?
-            }
+        match &def.field_information {
+            StructFieldInformation::Native => {},
+            StructFieldInformation::Declared(fields) => {
+                for field in fields {
+                    check_ident_complexity(module, &mut meter, field.name)?;
+                    check_sigtok_complexity(module, &mut meter, &field.signature.0)?
+                }
+            },
+            StructFieldInformation::DeclaredVariants(variants) => {
+                for variant in variants {
+                    check_ident_complexity(module, &mut meter, variant.name)?;
+                    for field in &variant.fields {
+                        check_ident_complexity(module, &mut meter, field.name)?;
+                        check_sigtok_complexity(module, &mut meter, &field.signature.0)?
+                    }
+                }
+            },
         }
     }
     for def in module.function_defs() {

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

@@ -385,8 +385,11 @@ impl<'a> BoundsChecker<'a> {
                 }
             },
             StructFieldInformation::DeclaredVariants(variants) => {
-                for field in variants.iter().flat_map(|v| v.fields.iter()) {
-                    self.check_field_def(type_param_count, field)?;
+                for variant in variants {
+                    check_bounds_impl(self.view.identifiers(), variant.name)?;
+                    for field in &variant.fields {
+                        self.check_field_def(type_param_count, field)?;
+                    }
                 }
                 if variants.is_empty() {
                     // Empty variants are not allowed

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

@@ -244,6 +244,7 @@ impl<'a> BinaryComplexityMeter<'a> {
                 },
                 StructFieldInformation::DeclaredVariants(variants) => {
                     for variant in variants {
+                        self.meter_identifier(variant.name)?;
                         for field in &variant.fields {
                             self.charge(field.signature.0.num_nodes() as u64)?;
                         }

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

@@ -230,15 +230,22 @@ impl StructDefinitionGen {
                     for (i, fd) in fields.into_iter().enumerate() {
                         variant_fields[i % self.variants.len()].push(fd)
                     }
+                    let mut seen_names = BTreeSet::new();
                     StructFieldInformation::DeclaredVariants(
                         variant_fields
                             .into_iter()
                             .zip(self.variants.iter())
-                            .map(|(fields, name)| VariantDefinition {
-                                name: IdentifierIndex(
-                                    name.index(state.identifiers_len) as TableIndex
-                                ),
-                                fields,
+                            .filter_map(|(fields, name)| {
+                                let variant_name = name.index(state.identifiers_len) as TableIndex;
+                                // avoid duplicates
+                                if seen_names.insert(variant_name) {
+                                    Some(VariantDefinition {
+                                        name: IdentifierIndex(variant_name),
+                                        fields,
+                                    })
+                                } else {
+                                    None
+                                }
                             })
                             .collect(),
                     )

third_party/move/move-bytecode-verifier/bytecode-verifier-tests/src/unit_tests/mod.rs

@@ -22,4 +22,5 @@ pub mod negative_stack_size_tests;
 pub mod reference_safety_tests;
 pub mod signature_tests;
 pub mod struct_defs_tests;
+pub mod variant_name_test;
 pub mod vec_pack_tests;

third_party/move/move-bytecode-verifier/bytecode-verifier-tests/src/unit_tests/variant_name_test.rs

@@ -0,0 +1,81 @@
+// Copyright (c) The Move Contributors
+// SPDX-License-Identifier: Apache-2.0
+
+use move_binary_format::{
+    file_format::{
+        AbilitySet, AddressIdentifierIndex, FieldDefinition, IdentifierIndex, ModuleHandle,
+        ModuleHandleIndex, Signature, SignatureToken, StructDefinition, StructFieldInformation,
+        StructHandle, StructHandleIndex, StructTypeParameter, TypeSignature, VariantDefinition,
+    },
+    file_format_common::VERSION_7,
+    CompiledModule,
+};
+use move_bytecode_verifier::{
+    verifier::verify_module_with_config_for_test_with_version, VerifierConfig,
+};
+use move_core_types::{identifier::Identifier, vm_status::StatusCode};
+
+/// Tests whether the name of a variant is in bounds. (That is, the IdentifierIndex
+/// is in bounds of the identifier table.)
+#[test]
+fn test_variant_name() {
+    // This is a POC produced during auditing
+    let ty = SignatureToken::Bool;
+
+    let cm = CompiledModule {
+        version: 7,
+        self_module_handle_idx: ModuleHandleIndex(0),
+        module_handles: vec![ModuleHandle {
+            address: AddressIdentifierIndex(0),
+            name: IdentifierIndex(0),
+        }],
+        struct_handles: vec![StructHandle {
+            module: ModuleHandleIndex(0),
+            name: IdentifierIndex(0),
+            abilities: AbilitySet::ALL,
+            type_parameters: vec![StructTypeParameter {
+                constraints: AbilitySet::EMPTY,
+                is_phantom: true,
+            }],
+        }],
+        function_handles: vec![],
+        field_handles: vec![],
+        friend_decls: vec![],
+        struct_def_instantiations: vec![],
+        function_instantiations: vec![],
+        field_instantiations: vec![],
+        signatures: vec![Signature(vec![]), Signature(vec![ty])],
+        identifiers: vec![Identifier::new("M").unwrap()],
+        address_identifiers: vec![],
+        constant_pool: vec![],
+        metadata: vec![],
+        struct_defs: vec![StructDefinition {
+            struct_handle: StructHandleIndex(0),
+            field_information: StructFieldInformation::DeclaredVariants(vec![VariantDefinition {
+                fields: vec![FieldDefinition {
+                    name: IdentifierIndex(0),
+                    signature: TypeSignature(SignatureToken::Bool),
+                }],
+                // <---- out of bound
+                name: IdentifierIndex(1),
+            }]),
+        }],
+        function_defs: vec![],
+        struct_variant_handles: vec![],
+        struct_variant_instantiations: vec![],
+        variant_field_handles: vec![],
+        variant_field_instantiations: vec![],
+    };
+
+    let result = verify_module_with_config_for_test_with_version(
+        "test_variant_name",
+        &VerifierConfig::production(),
+        &cm,
+        Some(VERSION_7),
+    );
+
+    assert_eq!(
+        result.unwrap_err().major_status(),
+        StatusCode::INDEX_OUT_OF_BOUNDS,
+    );
+}

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

@@ -15,7 +15,7 @@ use move_binary_format::{
     file_format::{
         CompiledModule, CompiledScript, Constant, FieldDefinition, FunctionHandle,
         FunctionHandleIndex, FunctionInstantiation, ModuleHandle, Signature,
-        StructFieldInformation, StructHandle, StructHandleIndex, TableIndex,
+        StructFieldInformation, StructHandle, StructHandleIndex, TableIndex, VariantDefinition,
     },
     IndexKind,
 };
@@ -52,6 +52,10 @@ impl<'a> DuplicationChecker<'a> {
         let checker = Self { module };
         checker.check_field_handles()?;
         checker.check_field_instantiations()?;
+        checker.check_variant_field_handles()?;
+        checker.check_variant_field_instantiations()?;
+        checker.check_struct_variant_handles()?;
+        checker.check_struct_variant_instantiations()?;
         checker.check_function_definitions()?;
         checker.check_struct_definitions()?;
         checker.check_struct_instantiations()
@@ -201,6 +205,50 @@ impl<'a> DuplicationChecker<'a> {
         Ok(())
     }
 
+    fn check_variant_field_handles(&self) -> PartialVMResult<()> {
+        match Self::first_duplicate_element(self.module.variant_field_handles()) {
+            Some(idx) => Err(verification_error(
+                StatusCode::DUPLICATE_ELEMENT,
+                IndexKind::VariantFieldHandle,
+                idx,
+            )),
+            None => Ok(()),
+        }
+    }
+
+    fn check_variant_field_instantiations(&self) -> PartialVMResult<()> {
+        match Self::first_duplicate_element(self.module.variant_field_instantiations()) {
+            Some(idx) => Err(verification_error(
+                StatusCode::DUPLICATE_ELEMENT,
+                IndexKind::VariantFieldInstantiation,
+                idx,
+            )),
+            None => Ok(()),
+        }
+    }
+
+    fn check_struct_variant_handles(&self) -> PartialVMResult<()> {
+        match Self::first_duplicate_element(self.module.struct_variant_handles()) {
+            Some(idx) => Err(verification_error(
+                StatusCode::DUPLICATE_ELEMENT,
+                IndexKind::StructVariantHandle,
+                idx,
+            )),
+            None => Ok(()),
+        }
+    }
+
+    fn check_struct_variant_instantiations(&self) -> PartialVMResult<()> {
+        match Self::first_duplicate_element(self.module.struct_variant_instantiations()) {
+            Some(idx) => Err(verification_error(
+                StatusCode::DUPLICATE_ELEMENT,
+                IndexKind::StructVariantInstantiation,
+                idx,
+            )),
+            None => Ok(()),
+        }
+    }
+
     fn check_struct_definitions(&self) -> PartialVMResult<()> {
         // StructDefinition - contained StructHandle defines uniqueness
         if let Some(idx) =
@@ -212,7 +260,7 @@ impl<'a> DuplicationChecker<'a> {
                 idx,
             ));
         }
-        // Field names in structs must be unique
+        // Field names in variants and structs must be unique
         for (struct_idx, struct_def) in self.module.struct_defs().iter().enumerate() {
             match &struct_def.field_information {
                 StructFieldInformation::Native => continue,
@@ -227,6 +275,7 @@ impl<'a> DuplicationChecker<'a> {
                     Self::check_duplicate_fields(fields.iter())?
                 },
                 StructFieldInformation::DeclaredVariants(variants) => {
+                    Self::check_duplicate_variants(variants.iter())?;
                     for variant in variants {
                         Self::check_duplicate_fields(variant.fields.iter())?
                     }
@@ -278,6 +327,20 @@ impl<'a> DuplicationChecker<'a> {
         }
     }
 
+    fn check_duplicate_variants<'l>(
+        variants: impl Iterator<Item = &'l VariantDefinition>,
+    ) -> PartialVMResult<()> {
+        if let Some(idx) = Self::first_duplicate_element(variants.map(|x| x.name)) {
+            Err(verification_error(
+                StatusCode::DUPLICATE_ELEMENT,
+                IndexKind::VariantDefinition,
+                idx,
+            ))
+        } else {
+            Ok(())
+        }
+    }
+
     fn check_function_definitions(&self) -> PartialVMResult<()> {
         // FunctionDefinition - contained FunctionHandle defines uniqueness
         if let Some(idx) =

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

@@ -97,10 +97,20 @@ impl<'a> LimitsVerifier<'a> {
         }
         if let Some(sdefs) = self.resolver.struct_defs() {
             for sdef in sdefs {
-                if let StructFieldInformation::Declared(fdefs) = &sdef.field_information {
-                    for fdef in fdefs {
-                        self.verify_type_node(config, &fdef.signature.0)?
-                    }
+                match &sdef.field_information {
+                    StructFieldInformation::Native => {},
+                    StructFieldInformation::Declared(fdefs) => {
+                        for fdef in fdefs {
+                            self.verify_type_node(config, &fdef.signature.0)?
+                        }
+                    },
+                    StructFieldInformation::DeclaredVariants(variants) => {
+                        for variant in variants {
+                            for fdef in &variant.fields {
+                                self.verify_type_node(config, &fdef.signature.0)?
+                            }
+                        }
+                    },
                 }
             }
         }

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

@@ -1151,14 +1151,28 @@ fn max_num_of_ty_params_or_args(resolver: BinaryIndexedView) -> usize {
 
     if let Some(struct_defs) = resolver.struct_defs() {
         for struct_def in struct_defs {
-            if let StructFieldInformation::Declared(fields) = &struct_def.field_information {
-                for field in fields {
-                    for ty in field.signature.0.preorder_traversal() {
-                        if let SignatureToken::TypeParameter(ty_param_idx) = ty {
-                            n = n.max(*ty_param_idx as usize + 1)
+            match &struct_def.field_information {
+                StructFieldInformation::Native => {},
+                StructFieldInformation::Declared(fields) => {
+                    for field in fields {
+                        for ty in field.signature.0.preorder_traversal() {
+                            if let SignatureToken::TypeParameter(ty_param_idx) = ty {
+                                n = n.max(*ty_param_idx as usize + 1)
+                            }
                         }
                     }
-                }
+                },
+                StructFieldInformation::DeclaredVariants(variants) => {
+                    for variant in variants {
+                        for field in &variant.fields {
+                            for ty in field.signature.0.preorder_traversal() {
+                                if let SignatureToken::TypeParameter(ty_param_idx) = ty {
+                                    n = n.max(*ty_param_idx as usize + 1)
+                                }
+                            }
+                        }
+                    }
+                },
             }
         }
     }

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

@@ -63,10 +63,21 @@ pub fn verify_module_with_config_for_test(
     name: &str,
     config: &VerifierConfig,
     module: &CompiledModule,
+) -> VMResult<()> {
+    verify_module_with_config_for_test_with_version(name, config, module, None)
+}
+
+pub fn verify_module_with_config_for_test_with_version(
+    name: &str,
+    config: &VerifierConfig,
+    module: &CompiledModule,
+    bytecode_version: Option<u32>,
 ) -> VMResult<()> {
     const MAX_MODULE_SIZE: usize = 65355;
     let mut bytes = vec![];
-    module.serialize(&mut bytes).unwrap();
+    module
+        .serialize_for_version(bytecode_version, &mut bytes)
+        .unwrap();
     let now = Instant::now();
     let result = verify_module_with_config(config, module);
     eprintln!(