Limit the maximum identifier length (#10678)

aptos-labs · Oct 26, 2023 · c5268ae · c5268ae
1 parent b16b153
commit c5268ae
Show file tree

Hide file tree

Showing 20 changed files with 245 additions and 108 deletions.
diff --git a/aptos-move/aptos-release-builder/src/components/feature_flags.rs b/aptos-move/aptos-release-builder/src/components/feature_flags.rs
@@ -83,6 +83,7 @@ pub enum FeatureFlag {
     SignatureCheckerV2ScriptFix,
     SaferResourceGroups,
     SaferMetadata,
+    LimitMaxIdentifierLength,
 }
 
 fn generate_features_blob(writer: &CodeWriter, data: &[u64]) {
@@ -216,6 +217,7 @@ impl From<FeatureFlag> for AptosFeatureFlag {
             },
             FeatureFlag::SaferResourceGroups => AptosFeatureFlag::SAFER_RESOURCE_GROUPS,
             FeatureFlag::SaferMetadata => AptosFeatureFlag::SAFER_METADATA,
+            FeatureFlag::LimitMaxIdentifierLength => AptosFeatureFlag::LIMIT_MAX_IDENTIFIER_LENGTH,
         }
     }
 }
@@ -272,6 +274,7 @@ impl From<AptosFeatureFlag> for FeatureFlag {
             },
             AptosFeatureFlag::SAFER_RESOURCE_GROUPS => FeatureFlag::SaferResourceGroups,
             AptosFeatureFlag::SAFER_METADATA => FeatureFlag::SaferMetadata,
+            AptosFeatureFlag::LIMIT_MAX_IDENTIFIER_LENGTH => FeatureFlag::LimitMaxIdentifierLength,
         }
     }
 }

diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs
@@ -53,7 +53,9 @@ use fail::fail_point;
 use move_binary_format::{
     access::ModuleAccess,
     compatibility::Compatibility,
+    deserializer::DeserializerConfig,
     errors::{verification_error, Location, PartialVMError, VMError, VMResult},
+    file_format_common::{IDENTIFIER_SIZE_MAX, LEGACY_IDENTIFIER_SIZE_MAX},
     CompiledModule, IndexKind,
 };
 use move_core_types::{
@@ -443,7 +445,7 @@ impl AptosVM {
                     // Gerardo: consolidate the extended validation to verifier.
                     verifier::event_validation::verify_no_event_emission_in_script(
                         script.code(),
-                        session.get_vm_config().max_binary_format_version,
+                        &session.get_vm_config().deserializer_config,
                     )?;
 
                     let args =
@@ -763,7 +765,10 @@ impl AptosVM {
         module_bundle: &ModuleBundle,
     ) -> VMResult<()> {
         for module_blob in module_bundle.iter() {
-            match CompiledModule::deserialize(module_blob.code()) {
+            match CompiledModule::deserialize_with_config(
+                module_blob.code(),
+                &session.get_vm_config().deserializer_config,
+            ) {
                 Ok(module) => {
                     // verify the module doesn't exist
                     if session.load_module(&module.self_id()).is_ok() {
@@ -837,9 +842,19 @@ impl AptosVM {
         } else {
             5
         };
+        let max_identifier_size = if self
+            .0
+            .get_features()
+            .is_enabled(FeatureFlag::LIMIT_MAX_IDENTIFIER_LENGTH)
+        {
+            IDENTIFIER_SIZE_MAX
+        } else {
+            LEGACY_IDENTIFIER_SIZE_MAX
+        };
+        let config = DeserializerConfig::new(max_version, max_identifier_size);
         let mut result = vec![];
         for module_blob in modules.iter() {
-            match CompiledModule::deserialize_with_max_version(module_blob.code(), max_version) {
+            match CompiledModule::deserialize_with_config(module_blob.code(), &config) {
                 Ok(module) => {
                     result.push(module);
                 },

diff --git a/aptos-move/aptos-vm/src/data_cache.rs b/aptos-move/aptos-vm/src/data_cache.rs
@@ -5,7 +5,10 @@
 
 use crate::{
     aptos_vm_impl::gas_config,
-    move_vm_ext::{get_max_binary_format_version, AptosMoveResolver, StateValueMetadataResolver},
+    move_vm_ext::{
+        get_max_binary_format_version, get_max_identifier_size, AptosMoveResolver,
+        StateValueMetadataResolver,
+    },
 };
 #[allow(unused_imports)]
 use anyhow::Error;
@@ -25,7 +28,7 @@ use aptos_types::{
         state_value::{StateValue, StateValueMetadata},
     },
 };
-use move_binary_format::{errors::*, CompiledModule};
+use move_binary_format::{deserializer::DeserializerConfig, errors::*, CompiledModule};
 use move_core_types::{
     account_address::AccountAddress,
     language_storage::{ModuleId, StructTag},
@@ -51,7 +54,7 @@ pub(crate) fn get_resource_group_from_metadata(
 pub struct StorageAdapter<'a, S> {
     state_store: &'a S,
     accurate_byte_count: bool,
-    max_binary_format_version: u32,
+    deserializer_config: DeserializerConfig,
     resource_group_cache:
         RefCell<BTreeMap<AccountAddress, BTreeMap<StructTag, BTreeMap<StructTag, Vec<u8>>>>>,
 }
@@ -62,16 +65,22 @@ impl<'a, S> StorageAdapter<'a, S> {
         gas_feature_version: u64,
         features: &Features,
     ) -> Self {
+        let max_binary_format_version =
+            get_max_binary_format_version(features, gas_feature_version);
+        let max_identifier_size = get_max_identifier_size(features);
+
         let mut s = Self {
             state_store,
             accurate_byte_count: false,
-            max_binary_format_version: 0,
+            deserializer_config: DeserializerConfig::new(
+                max_binary_format_version,
+                max_identifier_size,
+            ),
             resource_group_cache: RefCell::new(BTreeMap::new()),
         };
         if gas_feature_version >= 9 {
             s.accurate_byte_count = true;
         }
-        s.max_binary_format_version = get_max_binary_format_version(features, gas_feature_version);
         s
     }
 }
@@ -81,15 +90,18 @@ impl<'a, S: StateView> StorageAdapter<'a, S> {
         let mut s = Self {
             state_store,
             accurate_byte_count: false,
-            max_binary_format_version: 0,
+            deserializer_config: DeserializerConfig::new(0, 0),
             resource_group_cache: RefCell::new(BTreeMap::new()),
         };
         let (_, gas_feature_version) = gas_config(&s);
         let features = Features::fetch_config(&s).unwrap_or_default();
         if gas_feature_version >= 9 {
             s.accurate_byte_count = true;
         }
-        s.max_binary_format_version = get_max_binary_format_version(&features, gas_feature_version);
+        s.deserializer_config = DeserializerConfig::new(
+            get_max_binary_format_version(&features, gas_feature_version),
+            get_max_identifier_size(&features),
+        );
         s
     }
 
@@ -189,13 +201,12 @@ impl<'a, S: StateView> ModuleResolver for StorageAdapter<'a, S> {
             Ok(Some(bytes)) => bytes,
             _ => return vec![],
         };
-        let module = match CompiledModule::deserialize_with_max_version(
-            &module_bytes,
-            self.max_binary_format_version,
-        ) {
-            Ok(module) => module,
-            _ => return vec![],
-        };
+        let module =
+            match CompiledModule::deserialize_with_config(&module_bytes, &self.deserializer_config)
+            {
+                Ok(module) => module,
+                _ => return vec![],
+            };
         module.metadata
     }
 

diff --git a/aptos-move/aptos-vm/src/move_vm_ext/mod.rs b/aptos-move/aptos-vm/src/move_vm_ext/mod.rs
@@ -12,5 +12,5 @@ pub use crate::move_vm_ext::{
     resolver::{AptosMoveResolver, MoveResolverExt, StateValueMetadataResolver},
     respawned_session::RespawnedSession,
     session::{SessionExt, SessionId},
-    vm::{get_max_binary_format_version, verifier_config, MoveVmExt},
+    vm::{get_max_binary_format_version, get_max_identifier_size, verifier_config, MoveVmExt},
 };
diff --git a/aptos-move/aptos-vm/src/move_vm_ext/vm.rs b/aptos-move/aptos-vm/src/move_vm_ext/vm.rs
@@ -18,7 +18,11 @@ use aptos_gas_schedule::{MiscGasParameters, NativeGasParameters};
 use aptos_native_interface::SafeNativeBuilder;
 use aptos_table_natives::NativeTableContext;
 use aptos_types::on_chain_config::{FeatureFlag, Features, TimedFeatureFlag, TimedFeatures};
-use move_binary_format::errors::VMResult;
+use move_binary_format::{
+    deserializer::DeserializerConfig,
+    errors::VMResult,
+    file_format_common::{IDENTIFIER_SIZE_MAX, LEGACY_IDENTIFIER_SIZE_MAX},
+};
 use move_bytecode_verifier::VerifierConfig;
 use move_vm_runtime::{
     config::VMConfig, move_vm::MoveVM, native_extensions::NativeContextExtensions,
@@ -39,6 +43,14 @@ pub fn get_max_binary_format_version(features: &Features, gas_feature_version: u
     }
 }
 
+pub fn get_max_identifier_size(features: &Features) -> u64 {
+    if features.is_enabled(FeatureFlag::LIMIT_MAX_IDENTIFIER_LENGTH) {
+        IDENTIFIER_SIZE_MAX
+    } else {
+        LEGACY_IDENTIFIER_SIZE_MAX
+    }
+}
+
 impl MoveVmExt {
     fn new_impl<F>(
         native_gas_params: NativeGasParameters,
@@ -58,6 +70,8 @@ impl MoveVmExt {
         let max_binary_format_version =
             get_max_binary_format_version(&features, gas_feature_version);
 
+        let max_identifier_size = get_max_identifier_size(&features);
+
         let enable_invariant_violation_check_in_swap_loc =
             !timed_features.is_enabled(TimedFeatureFlag::DisableInvariantViolationCheckInSwapLoc);
         let type_size_limit = true;
@@ -89,7 +103,10 @@ impl MoveVmExt {
         Ok(Self {
             inner: MoveVM::new_with_config(aptos_natives_with_builder(&mut builder), VMConfig {
                 verifier: verifier_config,
-                max_binary_format_version,
+                deserializer_config: DeserializerConfig::new(
+                    max_binary_format_version,
+                    max_identifier_size,
+                ),
                 paranoid_type_checks: crate::AptosVM::get_paranoid_checks(),
                 enable_invariant_violation_check_in_swap_loc,
                 type_size_limit,

diff --git a/aptos-move/aptos-vm/src/verifier/event_validation.rs b/aptos-move/aptos-vm/src/verifier/event_validation.rs
@@ -5,6 +5,7 @@ use crate::move_vm_ext::SessionExt;
 use aptos_framework::RuntimeModuleMetadataV1;
 use move_binary_format::{
     access::{ModuleAccess, ScriptAccess},
+    deserializer::DeserializerConfig,
     errors::{Location, PartialVMError, VMError, VMResult},
     file_format::{
         Bytecode, CompiledScript,
@@ -121,8 +122,11 @@ pub(crate) fn extract_event_metadata_from_module(
     module_id: &ModuleId,
 ) -> VMResult<HashSet<String>> {
     let metadata = session.load_module(module_id).map(|module| {
-        CompiledModule::deserialize(&module)
-            .map(|module| aptos_framework::get_metadata_from_compiled_module(&module))
+        CompiledModule::deserialize_with_config(
+            &module,
+            &session.get_vm_config().deserializer_config,
+        )
+        .map(|module| aptos_framework::get_metadata_from_compiled_module(&module))
     });
 
     if let Ok(Ok(Some(metadata))) = metadata {
@@ -149,12 +153,9 @@ pub(crate) fn extract_event_metadata(
 
 pub(crate) fn verify_no_event_emission_in_script(
     script_code: &[u8],
-    max_binary_format_version: u32,
+    config: &DeserializerConfig,
 ) -> VMResult<()> {
-    let script = match CompiledScript::deserialize_with_max_version(
-        script_code,
-        max_binary_format_version,
-    ) {
+    let script = match CompiledScript::deserialize_with_config(script_code, config) {
         Ok(script) => script,
         Err(err) => {
             let msg = format!("[VM] deserializer for script returned error: {:?}", err);

diff --git a/aptos-move/aptos-vm/src/verifier/resource_groups.rs b/aptos-move/aptos-vm/src/verifier/resource_groups.rs
@@ -151,9 +151,12 @@ pub(crate) fn extract_resource_group_metadata_from_module(
     BTreeMap<String, StructTag>,
     BTreeSet<String>,
 )> {
-    let module = session
-        .load_module(module_id)
-        .map(|module| CompiledModule::deserialize(&module));
+    let module = session.load_module(module_id).map(|module| {
+        CompiledModule::deserialize_with_config(
+            &module,
+            &session.get_vm_config().deserializer_config,
+        )
+    });
     let (metadata, module) = if let Ok(Ok(module)) = module {
         (
             aptos_framework::get_metadata_from_compiled_module(&module),

diff --git a/aptos-move/e2e-move-tests/src/tests/mod.rs b/aptos-move/e2e-move-tests/src/tests/mod.rs
@@ -40,7 +40,6 @@ mod storage_refund;
 mod string_args;
 mod token_event_store;
 mod token_objects;
-mod too_large;
 mod transaction_fee;
 mod type_too_large;
 mod vector_numeric_address;

diff --git a/aptos-move/e2e-move-tests/src/tests/too_large.rs b/aptos-move/e2e-move-tests/src/tests/too_large.rs
diff --git a/aptos-move/framework/move-stdlib/doc/features.md b/aptos-move/framework/move-stdlib/doc/features.md
@@ -296,6 +296,17 @@ Lifetime: transient
 
 
 
+<a name="0x1_features_LIMIT_MAX_IDENTIFIER_LENGTH"></a>
+
+Whether the fix to reduce the maximum identifer length is enabled.
+Lifetime: transient
+
+
+<pre><code><b>const</b> <a href="features.md#0x1_features_LIMIT_MAX_IDENTIFIER_LENGTH">LIMIT_MAX_IDENTIFIER_LENGTH</a>: u64 = 33;
+</code></pre>
+
+
+
 <a name="0x1_features_MODULE_EVENT"></a>
 
 Whether emit function in <code>event.<b>move</b></code> are enabled for module events.

diff --git a/aptos-move/framework/move-stdlib/sources/configs/features.move b/aptos-move/framework/move-stdlib/sources/configs/features.move
@@ -251,6 +251,10 @@ module std::features {
         is_enabled(AGGREGATOR_SNAPSHOTS)
     }
 
+    /// Whether the fix to reduce the maximum identifer length is enabled.
+    /// Lifetime: transient
+    const LIMIT_MAX_IDENTIFIER_LENGTH: u64 = 33;
+
     // ============================================================================================
     // Feature Flag Implementation