[vm] introduce separation between CompiledModule and CompiledModuleMut

Similar to the previous diff, this introduces a separation between mutable and immutable variants of `CompiledModule`. The only bits of code that are affected are at the boundaries: the compiler and code to introduce invalid mutations.

This also found a number of places that did bounds checks twice. I've fixed all those places up (in fact I was forced to by the type system).

language/bytecode_verifier/invalid_mutations/src/bounds.rs

@@ -10,9 +10,9 @@ use std::collections::BTreeMap;
 use vm::{
     errors::{VMStaticViolation, VerificationError},
     file_format::{
-        AddressPoolIndex, CompiledModule, FieldDefinitionIndex, FunctionHandleIndex,
-        FunctionSignatureIndex, LocalsSignatureIndex, ModuleHandleIndex, StringPoolIndex,
-        StructHandleIndex, TableIndex, TypeSignatureIndex,
+        AddressPoolIndex, CompiledModule, CompiledModuleMut, FieldDefinitionIndex,
+        FunctionHandleIndex, FunctionSignatureIndex, LocalsSignatureIndex, ModuleHandleIndex,
+        StringPoolIndex, StructHandleIndex, TableIndex, TypeSignatureIndex,
     },
     internals::ModuleIndex,
     views::{ModuleView, SignatureTokenView},
@@ -162,8 +162,8 @@ impl AsRef<PropIndex> for OutOfBoundsMutation {
     }
 }
 
-pub struct ApplyOutOfBoundsContext<'a> {
-    module: &'a mut CompiledModule,
+pub struct ApplyOutOfBoundsContext {
+    module: CompiledModuleMut,
     // This is an Option because it gets moved out in apply before apply_one is called. Rust
     // doesn't let you call another con-consuming method after a partial move out.
     mutations: Option<Vec<OutOfBoundsMutation>>,
@@ -174,22 +174,22 @@ pub struct ApplyOutOfBoundsContext<'a> {
     locals_sig_structs: Vec<(LocalsSignatureIndex, usize)>,
 }
 
-impl<'a> ApplyOutOfBoundsContext<'a> {
-    pub fn new(module: &'a mut CompiledModule, mutations: Vec<OutOfBoundsMutation>) -> Self {
-        let type_sig_structs: Vec<_> = Self::type_sig_structs(module).collect();
-        let function_sig_structs: Vec<_> = Self::function_sig_structs(module).collect();
-        let locals_sig_structs: Vec<_> = Self::locals_sig_structs(module).collect();
+impl ApplyOutOfBoundsContext {
+    pub fn new(module: CompiledModule, mutations: Vec<OutOfBoundsMutation>) -> Self {
+        let type_sig_structs: Vec<_> = Self::type_sig_structs(&module).collect();
+        let function_sig_structs: Vec<_> = Self::function_sig_structs(&module).collect();
+        let locals_sig_structs: Vec<_> = Self::locals_sig_structs(&module).collect();
 
         Self {
-            module,
+            module: module.into_inner(),
             mutations: Some(mutations),
             type_sig_structs,
             function_sig_structs,
             locals_sig_structs,
         }
     }
 
-    pub fn apply(mut self) -> Vec<VerificationError> {
+    pub fn apply(mut self) -> (CompiledModuleMut, Vec<VerificationError>) {
         // This is a map from (source kind, dest kind) to the actual mutations -- this is done to
         // figure out how many mutations to do for a particular pair, which is required for
         // pick_slice_idxs below.
@@ -212,7 +212,7 @@ impl<'a> ApplyOutOfBoundsContext<'a> {
             // to get the lifetimes right :)
             results.extend(self.apply_one(src_kind, dst_kind, mutations));
         }
-        results
+        (self.module, results)
     }
 
     fn apply_one(

language/bytecode_verifier/invalid_mutations/src/bounds/code_unit.rs

@@ -7,7 +7,7 @@ use std::collections::BTreeMap;
 use vm::{
     errors::{VMStaticViolation, VerificationError},
     file_format::{
-        AddressPoolIndex, ByteArrayPoolIndex, Bytecode, CodeOffset, CompiledModule,
+        AddressPoolIndex, ByteArrayPoolIndex, Bytecode, CodeOffset, CompiledModuleMut,
         FieldDefinitionIndex, FunctionHandleIndex, LocalIndex, StringPoolIndex,
         StructDefinitionIndex, TableIndex,
     },
@@ -43,7 +43,7 @@ impl AsRef<PropIndex> for CodeUnitBoundsMutation {
 }
 
 pub struct ApplyCodeUnitBoundsContext<'a> {
-    module: &'a mut CompiledModule,
+    module: &'a mut CompiledModuleMut,
     // This is so apply_one can be called after mutations has been iterated on.
     mutations: Option<Vec<CodeUnitBoundsMutation>>,
 }
@@ -82,7 +82,7 @@ macro_rules! locals_bytecode {
 }
 
 impl<'a> ApplyCodeUnitBoundsContext<'a> {
-    pub fn new(module: &'a mut CompiledModule, mutations: Vec<CodeUnitBoundsMutation>) -> Self {
+    pub fn new(module: &'a mut CompiledModuleMut, mutations: Vec<CodeUnitBoundsMutation>) -> Self {
         Self {
             module,
             mutations: Some(mutations),

language/bytecode_verifier/invalid_mutations/src/signature.rs

@@ -9,7 +9,7 @@ use proptest_helpers::{pick_slice_idxs, RepeatVec};
 use std::collections::BTreeMap;
 use vm::{
     errors::{VMStaticViolation, VerificationError},
-    file_format::{CompiledModule, SignatureToken},
+    file_format::{CompiledModuleMut, SignatureToken},
     internals::ModuleIndex,
     IndexKind, SignatureTokenKind,
 };
@@ -38,12 +38,12 @@ impl AsRef<PropIndex> for DoubleRefMutation {
 
 /// Context for applying a list of `DoubleRefMutation` instances.
 pub struct ApplySignatureDoubleRefContext<'a> {
-    module: &'a mut CompiledModule,
+    module: &'a mut CompiledModuleMut,
     mutations: Vec<DoubleRefMutation>,
 }
 
 impl<'a> ApplySignatureDoubleRefContext<'a> {
-    pub fn new(module: &'a mut CompiledModule, mutations: Vec<DoubleRefMutation>) -> Self {
+    pub fn new(module: &'a mut CompiledModuleMut, mutations: Vec<DoubleRefMutation>) -> Self {
         Self { module, mutations }
     }
 
@@ -134,12 +134,12 @@ impl AsRef<PropIndex> for FieldRefMutation {
 
 /// Context for applying a list of `FieldRefMutation` instances.
 pub struct ApplySignatureFieldRefContext<'a> {
-    module: &'a mut CompiledModule,
+    module: &'a mut CompiledModuleMut,
     mutations: Vec<FieldRefMutation>,
 }
 
 impl<'a> ApplySignatureFieldRefContext<'a> {
-    pub fn new(module: &'a mut CompiledModule, mutations: Vec<FieldRefMutation>) -> Self {
+    pub fn new(module: &'a mut CompiledModuleMut, mutations: Vec<FieldRefMutation>) -> Self {
         Self { module, mutations }
     }
 

language/bytecode_verifier/src/check_duplication.rs

@@ -14,7 +14,7 @@ use vm::{
     errors::{VMStaticViolation, VerificationError},
     file_format::{
         CompiledModule, FieldDefinitionIndex, FunctionHandleIndex, ModuleHandleIndex,
-        StructHandleIndex,
+        StructHandleIndex, TableIndex,
     },
     IndexKind,
 };
@@ -138,9 +138,14 @@ impl<'a> DuplicationChecker<'a> {
                 break;
             }
             let next_start_field_index = start_field_index + struct_def.field_count as usize;
-            if !(start_field_index..next_start_field_index)
-                .all(|i| struct_def.struct_handle == self.module.field_defs[i].struct_)
-            {
+            let all_fields_match = (start_field_index..next_start_field_index).all(|i| {
+                struct_def.struct_handle
+                    == self
+                        .module
+                        .field_def_at(FieldDefinitionIndex::new(i as TableIndex))
+                        .struct_
+            });
+            if !all_fields_match {
                 idx_opt = Some(idx);
                 break;
             }
@@ -152,7 +157,7 @@ impl<'a> DuplicationChecker<'a> {
                 idx,
                 err: VMStaticViolation::InconsistentFields,
             });
-        } else if start_field_index != self.module.field_defs.len() {
+        } else if start_field_index != self.module.field_defs().len() {
             errors.push(VerificationError {
                 kind: IndexKind::FieldDefinition,
                 idx: start_field_index,
@@ -188,7 +193,7 @@ impl<'a> DuplicationChecker<'a> {
         // implemented.
         let implemented_struct_handles: HashSet<StructHandleIndex> =
             self.module.struct_defs().map(|x| x.struct_handle).collect();
-        if let Some(idx) = (0..self.module.struct_handles.len()).position(|x| {
+        if let Some(idx) = (0..self.module.struct_handles().len()).position(|x| {
             let y = StructHandleIndex::new(x as u16);
             self.module.struct_handle_at(y).module
                 == ModuleHandleIndex::new(CompiledModule::IMPLEMENTED_MODULE_INDEX)
@@ -204,7 +209,7 @@ impl<'a> DuplicationChecker<'a> {
         // implemented.
         let implemented_function_handles: HashSet<FunctionHandleIndex> =
             self.module.function_defs().map(|x| x.function).collect();
-        if let Some(idx) = (0..self.module.function_handles.len()).position(|x| {
+        if let Some(idx) = (0..self.module.function_handles().len()).position(|x| {
             let y = FunctionHandleIndex::new(x as u16);
             self.module.function_handle_at(y).module
                 == ModuleHandleIndex::new(CompiledModule::IMPLEMENTED_MODULE_INDEX)

language/bytecode_verifier/src/struct_defs.rs

@@ -61,7 +61,7 @@ impl<'a> StructDefGraphBuilder<'a> {
         let mut handle_to_def = BTreeMap::new();
         // the mapping from struct definitions to struct handles is already checked to be 1-1 by
         // DuplicationChecker
-        for (idx, struct_def) in module.struct_defs.iter().enumerate() {
+        for (idx, struct_def) in module.struct_defs().enumerate() {
             let sh_idx = struct_def.struct_handle;
             handle_to_def.insert(sh_idx, StructDefinitionIndex::new(idx as TableIndex));
         }
@@ -75,7 +75,7 @@ impl<'a> StructDefGraphBuilder<'a> {
     pub fn build(self) -> Graph<StructDefinitionIndex, (), Directed, u32> {
         let mut graph = Graph::new();
 
-        let struct_def_count = self.module.struct_defs.len();
+        let struct_def_count = self.module.struct_defs().len();
 
         let nodes: Vec<_> = (0..struct_def_count)
             .map(|idx| graph.add_node(StructDefinitionIndex::new(idx as TableIndex)))

language/bytecode_verifier/src/verifier.rs

@@ -11,23 +11,21 @@ use std::collections::BTreeMap;
 use types::language_storage::CodeKey;
 use vm::{
     access::{BaseAccess, ScriptAccess},
-    checks::BoundsChecker,
     errors::{VMStaticViolation, VerificationError},
     file_format::{CompiledModule, CompiledScript},
     resolver::Resolver,
     views::{ModuleView, ViewInternals},
     IndexKind,
 };
 
-/// Verification of a module is performed through a sequnence of checks.
-/// There is a partial order on the checs. For example, bounds checking must precede all other
-/// checks and duplication check must precede the structural recursion check. In general, later
-/// checks are more expensive.
+/// Verification of a module is performed through a sequence of checks.
+///
+/// There is a partial order on the checks. For example, the duplication check must precede the
+/// structural recursion check. In general, later checks are more expensive.
 pub fn verify_module(module: CompiledModule) -> (CompiledModule, Vec<VerificationError>) {
-    let mut errors = BoundsChecker::new(&module).verify();
-    if errors.is_empty() {
-        errors.append(&mut DuplicationChecker::new(&module).verify());
-    }
+    // All CompiledModule instances are statically guaranteed to be bounds checked, so there's no
+    // need for more checking.
+    let mut errors = DuplicationChecker::new(&module).verify();
     if errors.is_empty() {
         errors.append(&mut SignatureChecker::new(&module).verify());
         errors.append(&mut ResourceTransitiveChecker::new(&module).verify());

language/bytecode_verifier/tests/bounds_tests.rs

@@ -10,15 +10,15 @@ use types::{account_address::AccountAddress, byte_array::ByteArray};
 use vm::{
     checks::BoundsChecker,
     errors::{VMStaticViolation, VerificationError},
+    file_format::{CompiledModule, CompiledModuleMut},
     proptest_types::CompiledModuleStrategyGen,
-    CompiledModule, IndexKind,
+    IndexKind,
 };
 
 proptest! {
     #[test]
-    fn valid_bounds(module in CompiledModule::valid_strategy(20)) {
-        let bounds_checker = BoundsChecker::new(&module);
-        prop_assert_eq!(bounds_checker.verify(), vec![]);
+    fn valid_bounds(_module in CompiledModule::valid_strategy(20)) {
+        // valid_strategy will panic if there are any bounds check issues.
     }
 }
 
@@ -30,9 +30,8 @@ proptest! {
 fn valid_bounds_no_members() {
     let mut gen = CompiledModuleStrategyGen::new(20);
     gen.member_count(0);
-    proptest!(|(module in gen.generate())| {
-        let bounds_checker = BoundsChecker::new(&module);
-        prop_assert_eq!(bounds_checker.verify(), vec![])
+    proptest!(|(_module in gen.generate())| {
+        // gen.generate() will panic if there are any bounds check issues.
     });
 }
 
@@ -42,9 +41,8 @@ proptest! {
         module in CompiledModule::valid_strategy(20),
         oob_mutations in vec(OutOfBoundsMutation::strategy(), 0..40),
     ) {
-        let mut module = module;
-        let mut expected_violations = {
-            let oob_context = ApplyOutOfBoundsContext::new(&mut module, oob_mutations);
+        let (module, mut expected_violations) = {
+            let oob_context = ApplyOutOfBoundsContext::new(module, oob_mutations);
             oob_context.apply()
         };
         expected_violations.sort();
@@ -60,7 +58,7 @@ proptest! {
         module in CompiledModule::valid_strategy(20),
         mutations in vec(CodeUnitBoundsMutation::strategy(), 0..40),
     ) {
-        let mut module = module;
+        let mut module = module.into_inner();
         let mut expected_violations = {
             let context = ApplyCodeUnitBoundsContext::new(&mut module, mutations);
             context.apply()
@@ -81,7 +79,7 @@ proptest! {
     ) {
         // If there are no module handles, the only other things that can be stored are intrinsic
         // data.
-        let mut module = CompiledModule::default();
+        let mut module = CompiledModuleMut::default();
         module.string_pool = string_pool;
         module.address_pool = address_pool;
         module.byte_array_pool = byte_array_pool;
@@ -108,8 +106,7 @@ proptest! {
 
     /// Make sure that garbage inputs don't crash the bounds checker.
     #[test]
-    fn garbage_inputs(module in any_with::<CompiledModule>(16)) {
-        let bounds_checker = BoundsChecker::new(&module);
-        bounds_checker.verify();
+    fn garbage_inputs(module in any_with::<CompiledModuleMut>(16)) {
+        let _ = module.freeze();
     }
 }

language/bytecode_verifier/tests/duplication_tests.rs

@@ -3,12 +3,11 @@
 
 use bytecode_verifier::DuplicationChecker;
 use proptest::prelude::*;
-use vm::{checks::BoundsChecker, file_format::CompiledModule};
+use vm::file_format::CompiledModule;
 
 proptest! {
     #[test]
     fn valid_duplication(module in CompiledModule::valid_strategy(20)) {
-        prop_assert!(BoundsChecker::new(&module).verify().is_empty());
         let duplication_checker = DuplicationChecker::new(&module);
         prop_assert!(!duplication_checker.verify().is_empty());
     }

language/bytecode_verifier/tests/resources_tests.rs

@@ -3,12 +3,11 @@
 
 use bytecode_verifier::ResourceTransitiveChecker;
 use proptest::prelude::*;
-use vm::{checks::BoundsChecker, file_format::CompiledModule};
+use vm::file_format::CompiledModule;
 
 proptest! {
     #[test]
     fn valid_resource_transitivity(module in CompiledModule::valid_strategy(20)) {
-        prop_assert!(BoundsChecker::new(&module).verify().is_empty());
         let resource_checker = ResourceTransitiveChecker::new(&module);
         prop_assert!(resource_checker.verify().is_empty());
     }

language/bytecode_verifier/tests/signature_tests.rs

@@ -7,12 +7,11 @@ use invalid_mutations::signature::{
     FieldRefMutation,
 };
 use proptest::{collection::vec, prelude::*};
-use vm::{checks::BoundsChecker, errors::VMStaticViolation, file_format::CompiledModule};
+use vm::{errors::VMStaticViolation, file_format::CompiledModule};
 
 proptest! {
     #[test]
     fn valid_signatures(module in CompiledModule::valid_strategy(20)) {
-        prop_assert!(BoundsChecker::new(&module).verify().is_empty());
         let signature_checker = SignatureChecker::new(&module);
         prop_assert_eq!(signature_checker.verify(), vec![]);
     }
@@ -22,14 +21,14 @@ proptest! {
         module in CompiledModule::valid_strategy(20),
         mutations in vec(DoubleRefMutation::strategy(), 0..40),
     ) {
-        let mut module = module;
+        let mut module = module.into_inner();
         let mut expected_violations = {
             let context = ApplySignatureDoubleRefContext::new(&mut module, mutations);
             context.apply()
         };
         expected_violations.sort();
+        let module = module.freeze().expect("should satisfy bounds checker");
 
-        prop_assert!(BoundsChecker::new(&module).verify().is_empty());
         let signature_checker = SignatureChecker::new(&module);
 
         let actual_violations = signature_checker.verify();
@@ -52,14 +51,14 @@ proptest! {
         module in CompiledModule::valid_strategy(20),
         mutations in vec(FieldRefMutation::strategy(), 0..40),
     ) {
-        let mut module = module;
+        let mut module = module.into_inner();
         let mut expected_violations = {
             let context = ApplySignatureFieldRefContext::new(&mut module, mutations);
             context.apply()
         };
         expected_violations.sort();
+        let module = module.freeze().expect("should satisfy bounds checker");
 
-        prop_assert!(BoundsChecker::new(&module).verify().is_empty());
         let signature_checker = SignatureChecker::new(&module);
 
         let mut actual_violations = signature_checker.verify();

language/bytecode_verifier/tests/struct_defs_tests.rs

@@ -3,12 +3,11 @@
 
 use bytecode_verifier::RecursiveStructDefChecker;
 use proptest::prelude::*;
-use vm::{checks::BoundsChecker, file_format::CompiledModule};
+use vm::file_format::CompiledModule;
 
 proptest! {
     #[test]
     fn valid_recursive_struct_defs(module in CompiledModule::valid_strategy(20)) {
-        prop_assert!(BoundsChecker::new(&module).verify().is_empty());
         let recursive_checker = RecursiveStructDefChecker::new(&module);
         prop_assert!(recursive_checker.verify().is_empty());
     }