Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ impl From<FeatureFlag> for AptosFeatureFlag {
},
FeatureFlag::EnableEnumTypes => AptosFeatureFlag::ENABLE_ENUM_TYPES,
FeatureFlag::EnableResourceAccessControl => {
AptosFeatureFlag::ENABLE_RESOURCE_ACCESS_CONTROL
AptosFeatureFlag::_DEPRECATED_ENABLE_RESOURCE_ACCESS_CONTROL
},
FeatureFlag::RejectUnstableBytecodeForScript => {
AptosFeatureFlag::_REJECT_UNSTABLE_BYTECODE_FOR_SCRIPT
Expand Down Expand Up @@ -554,7 +554,7 @@ impl From<AptosFeatureFlag> for FeatureFlag {
FeatureFlag::UseCompatibilityCheckerV2
},
AptosFeatureFlag::ENABLE_ENUM_TYPES => FeatureFlag::EnableEnumTypes,
AptosFeatureFlag::ENABLE_RESOURCE_ACCESS_CONTROL => {
AptosFeatureFlag::_DEPRECATED_ENABLE_RESOURCE_ACCESS_CONTROL => {
FeatureFlag::EnableResourceAccessControl
},
AptosFeatureFlag::_REJECT_UNSTABLE_BYTECODE_FOR_SCRIPT => {
Expand Down
1 change: 1 addition & 0 deletions aptos-move/aptos-vm-environment/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ rust-version = { workspace = true }
aptos-framework = { workspace = true }
aptos-gas-algebra = { workspace = true }
aptos-gas-schedule = { workspace = true }
aptos-logger = { workspace = true }
aptos-move-stdlib = { workspace = true }
aptos-native-interface = { workspace = true }
aptos-table-natives = { workspace = true }
Expand Down
13 changes: 10 additions & 3 deletions aptos-move/aptos-vm-environment/src/prod_configs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use aptos_gas_schedule::{
gas_feature_versions::{RELEASE_V1_15, RELEASE_V1_30, RELEASE_V1_34, RELEASE_V1_38},
AptosGasParameters,
};
use aptos_logger::warn;
use aptos_types::{
on_chain_config::{
randomness_api_v0_config::{AllowCustomMaxGasFlag, RequiredGasDeposit},
Expand Down Expand Up @@ -122,8 +123,14 @@ pub fn aptos_prod_verifier_config(gas_feature_version: u64, features: &Features)
features.is_enabled(FeatureFlag::SIGNATURE_CHECKER_V2_SCRIPT_FIX);
let sig_checker_v2_fix_function_signatures = gas_feature_version >= RELEASE_V1_34;
let enable_enum_types = features.is_enabled(FeatureFlag::ENABLE_ENUM_TYPES);
let enable_resource_access_control =
features.is_enabled(FeatureFlag::ENABLE_RESOURCE_ACCESS_CONTROL);
// Resource access control has been removed from the VM, so the on-chain flag no longer has
// any effect: access specifiers are rejected at verification and never enforced at runtime.
if features.is_enabled(FeatureFlag::_DEPRECATED_ENABLE_RESOURCE_ACCESS_CONTROL) {
warn!(
"On-chain feature ENABLE_RESOURCE_ACCESS_CONTROL is enabled but has been \
removed; ignoring it."
);
}
let enable_function_values = features.is_enabled(FeatureFlag::ENABLE_FUNCTION_VALUES);
// Note: we reuse the `enable_function_values` flag to set various stricter limits on types.

Expand Down Expand Up @@ -153,7 +160,7 @@ pub fn aptos_prod_verifier_config(gas_feature_version: u64, features: &Features)
sig_checker_v2_fix_script_ty_param_count,
sig_checker_v2_fix_function_signatures,
enable_enum_types,
enable_resource_access_control,
_enable_resource_access_control: false,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just remove this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment of the structure says

/// Configuration for the bytecode verifier.
///
/// Always add new fields to the end, as we rely on the hash or serialized bytes of config to
/// detect if it has changed (e.g., new feature flag was enabled). Also, do not delete existing
/// fields, or change the type of existing field.
#[derive(Debug, Clone, Eq, PartialEq, Serialize)]
pub struct VerifierConfig {

Though it seems to me it's removable.

enable_function_values,
max_function_return_values: if enable_function_values {
Some(128)
Expand Down
32 changes: 0 additions & 32 deletions aptos-move/e2e-move-tests/src/tests/move_feature_gating.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,38 +44,6 @@ fn enum_types(enabled: Vec<FeatureFlag>, disabled: Vec<FeatureFlag>) {
}
}

#[rstest(enabled, disabled,
case(vec![], vec![FeatureFlag::ENABLE_RESOURCE_ACCESS_CONTROL]),
case(vec![FeatureFlag::ENABLE_RESOURCE_ACCESS_CONTROL], vec![]),
)]
fn resource_access_control(enabled: Vec<FeatureFlag>, disabled: Vec<FeatureFlag>) {
let positive_test = !enabled.is_empty();
let mut h = MoveHarness::new_with_features(enabled, disabled);
let acc = h.new_account_at(AccountAddress::from_hex_literal("0x815").unwrap());

let mut builder = PackageBuilder::new("Package");
let source = r#"
module 0x815::m {
struct R has key, copy {}
fun read(a: address): R reads R {
*borrow_global<R>(a)
}
}
"#;
builder.add_source("m.move", source);
let path = builder.write_to_temp().unwrap();
let result = h.publish_package_with_options(
&acc,
path.path(),
BuildOptions::move_2().set_latest_language(),
);
if positive_test {
assert_success!(result);
} else {
assert_vm_status!(result, StatusCode::FEATURE_NOT_ENABLED);
}
}

#[test]
fn function_values() {
let sources = &[
Expand Down
3 changes: 0 additions & 3 deletions third_party/move/move-binary-format/src/proptest_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ pub struct CompiledModuleStrategyGen {
parameters_count: SizeRange,
return_count: SizeRange,
func_type_params: SizeRange,
access_specifiers_count: SizeRange,
acquires_count: SizeRange,
random_sigs_count: SizeRange,
tokens_per_random_sig_count: SizeRange,
Expand All @@ -106,7 +105,6 @@ impl CompiledModuleStrategyGen {
parameters_count: (0..4).into(),
return_count: (0..3).into(),
func_type_params: (0..3).into(),
access_specifiers_count: (0..8).into(),
acquires_count: (0..2).into(),
random_sigs_count: (0..5).into(),
tokens_per_random_sig_count: (0..5).into(),
Expand Down Expand Up @@ -185,7 +183,6 @@ impl CompiledModuleStrategyGen {
self.parameters_count.clone(),
self.return_count.clone(),
self.func_type_params.clone(),
self.access_specifiers_count.clone(),
),
1..=self.size,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use crate::{
file_format::{
AccessSpecifier, Bytecode, CodeOffset, CodeUnit, ConstantPoolIndex, FieldHandle,
Bytecode, CodeOffset, CodeUnit, ConstantPoolIndex, FieldHandle,
FieldHandleIndex, FieldInstantiation, FieldInstantiationIndex, FunctionDefinition,
FunctionHandle, FunctionHandleIndex, FunctionInstantiation, FunctionInstantiationIndex,
IdentifierIndex, LocalIndex, ModuleHandleIndex, Signature, SignatureIndex, SignatureToken,
Expand All @@ -24,7 +24,6 @@ use crate::{
use move_core_types::{ability::AbilitySet, int256::U256};
use proptest::{
collection::{vec, SizeRange},
option::of,
prelude::*,
sample::{select, Index as PropIndex},
};
Expand Down Expand Up @@ -168,15 +167,13 @@ pub struct FunctionHandleGen {
parameters: SignatureGen,
return_: SignatureGen,
type_parameters: Vec<AbilitySetGen>,
access_specifiers: Option<Vec<AccessSpecifier>>,
}

impl FunctionHandleGen {
pub fn strategy(
param_count: impl Into<SizeRange>,
return_count: impl Into<SizeRange>,
type_parameter_count: impl Into<SizeRange>,
access_specifiers_count: impl Into<SizeRange>,
) -> impl Strategy<Value = Self> {
let return_count = return_count.into();
let param_count = param_count.into();
Expand All @@ -186,18 +183,14 @@ impl FunctionHandleGen {
SignatureGen::strategy(param_count),
SignatureGen::strategy(return_count),
vec(AbilitySetGen::strategy(), type_parameter_count),
of(vec(any::<AccessSpecifier>(), access_specifiers_count)),
)
.prop_map(
|(module, name, parameters, return_, type_parameters, access_specifiers)| Self {
module,
name,
parameters,
return_,
type_parameters,
access_specifiers,
},
)
.prop_map(|(module, name, parameters, return_, type_parameters)| Self {
module,
name,
parameters,
return_,
type_parameters,
})
}

pub fn materialize(self, state: &mut FnHandleMaterializeState) -> Option<FunctionHandle> {
Expand Down Expand Up @@ -227,7 +220,8 @@ impl FunctionHandleGen {
parameters: params_idx,
return_: return_idx,
type_parameters,
access_specifiers: self.access_specifiers,
// Resource access control has been removed; the verifier rejects any specifiers.
access_specifiers: None,
attributes: vec![],
})
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright (c) The Move Contributors
// SPDX-License-Identifier: Apache-2.0

//! Resource access control has been removed, so access specifiers are rejected regardless of
//! configuration. Only hand-crafted bytecode can still carry them; no compiler emits them.

use move_binary_format::{
file_format::{
basic_test_module, empty_script, AccessKind, AccessSpecifier, AddressIdentifierIndex,
AddressSpecifier, ResourceSpecifier, TableIndex,
},
CompiledModule,
};
use move_bytecode_verifier::VerifierConfig;
use move_core_types::{account_address::AccountAddress, vm_status::StatusCode};

/// An access specifier reading any resource declared at the address added to `addresses`.
fn reads_any_at_new_address(addresses: &mut Vec<AccountAddress>) -> AccessSpecifier {
let addr = AddressIdentifierIndex::new(addresses.len() as TableIndex);
addresses.push(AccountAddress::ONE);
AccessSpecifier {
kind: AccessKind::Reads,
negated: false,
resource: ResourceSpecifier::DeclaredAtAddress(addr),
address: AddressSpecifier::Any,
}
}

fn module_with_access_specifiers() -> CompiledModule {
let mut m = basic_test_module();
let specifier = reads_any_at_new_address(&mut m.address_identifiers);
m.function_handles[0].access_specifiers = Some(vec![specifier]);
m
}

#[test]
fn module_access_specifiers_are_rejected() {
let m = module_with_access_specifiers();
let err = move_bytecode_verifier::verify_module_with_config(&VerifierConfig::production(), &m)
.unwrap_err();
assert_eq!(err.major_status(), StatusCode::FEATURE_NOT_ENABLED);
}

#[test]
fn script_access_specifiers_are_rejected() {
let mut s = empty_script();
let specifier = reads_any_at_new_address(&mut s.address_identifiers);
s.access_specifiers = Some(vec![specifier]);
let err = move_bytecode_verifier::verify_script_with_config(&VerifierConfig::production(), &s)
.unwrap_err();
assert_eq!(err.major_status(), StatusCode::FEATURE_NOT_ENABLED);
}

/// The same module without access specifiers must verify, so that the rejections above are
/// attributable to the specifiers rather than to an unrelated defect in the test fixtures.
#[test]
fn module_without_access_specifiers_is_accepted() {
let mut m = module_with_access_specifiers();
m.function_handles[0].access_specifiers = None;
assert!(
move_bytecode_verifier::verify_module_with_config(&VerifierConfig::production(), &m)
.is_ok()
);
}

#[test]
fn script_without_access_specifiers_is_accepted() {
let s = empty_script();
assert!(s.access_specifiers.is_none());
assert!(
move_bytecode_verifier::verify_script_with_config(&VerifierConfig::production(), &s)
.is_ok()
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Copyright (c) The Move Contributors
// SPDX-License-Identifier: Apache-2.0

pub mod access_specifier_tests;
pub mod binary_samples;
pub mod bounds_tests;
pub mod catch_unwind;
Expand Down
28 changes: 12 additions & 16 deletions third_party/move/move-bytecode-verifier/src/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ impl<'a> FeatureVerifier<'a> {
};
verifier.verify_signatures()?;
verifier.verify_function_handles()?;
if !config.enable_resource_access_control && script.access_specifiers.is_some() {
if script.access_specifiers.is_some() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove script.access_specifiers?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the file format already enabled on-chain, so we'd better keep it but ignore the RAC.

return Err(PartialVMError::new(StatusCode::FEATURE_NOT_ENABLED)
.with_message("resource access control feature not enabled".to_string()));
.with_message("resource access control is not supported".to_string()));
}
verifier.verify_code(&script.code.code, None)
}
Expand Down Expand Up @@ -106,20 +106,16 @@ impl<'a> FeatureVerifier<'a> {
}

fn verify_function_handles(&self) -> PartialVMResult<()> {
if !self.config.enable_resource_access_control || !self.config.enable_function_values {
for (idx, function_handle) in self.code.function_handles().iter().enumerate() {
if !self.config.enable_resource_access_control
&& function_handle.access_specifiers.is_some()
{
return Err(PartialVMError::new(StatusCode::FEATURE_NOT_ENABLED)
.at_index(IndexKind::FunctionHandle, idx as u16)
.with_message("resource access control feature not enabled".to_string()));
}
if !self.config.enable_function_values && !function_handle.attributes.is_empty() {
return Err(PartialVMError::new(StatusCode::FEATURE_NOT_ENABLED)
.at_index(IndexKind::FunctionDefinition, idx as u16)
.with_message("function value feature not enabled".to_string()));
}
for (idx, function_handle) in self.code.function_handles().iter().enumerate() {
if function_handle.access_specifiers.is_some() {
return Err(PartialVMError::new(StatusCode::FEATURE_NOT_ENABLED)
.at_index(IndexKind::FunctionHandle, idx as u16)
.with_message("resource access control is not supported".to_string()));
}
if !self.config.enable_function_values && !function_handle.attributes.is_empty() {
return Err(PartialVMError::new(StatusCode::FEATURE_NOT_ENABLED)
.at_index(IndexKind::FunctionDefinition, idx as u16)
.with_message("function value feature not enabled".to_string()));
}
}
Ok(())
Expand Down
9 changes: 6 additions & 3 deletions third_party/move/move-bytecode-verifier/src/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ pub struct VerifierConfig {
pub _use_signature_checker_v2: bool,
pub sig_checker_v2_fix_script_ty_param_count: bool,
pub enable_enum_types: bool,
pub enable_resource_access_control: bool,
// Deprecated: resource access control has been removed. Access specifiers are always
// rejected, regardless of this field, which is kept only to preserve the serialized
// layout of this config.
pub _enable_resource_access_control: bool,
pub enable_function_values: bool,
/// Maximum number of function return values.
pub max_function_return_values: Option<usize>,
Expand Down Expand Up @@ -263,7 +266,7 @@ impl Default for VerifierConfig {
sig_checker_v2_fix_function_signatures: true,

enable_enum_types: true,
enable_resource_access_control: true,
_enable_resource_access_control: false,
enable_function_values: true,

max_function_return_values: None,
Expand Down Expand Up @@ -312,7 +315,7 @@ impl VerifierConfig {
sig_checker_v2_fix_function_signatures: true,

enable_enum_types: true,
enable_resource_access_control: true,
_enable_resource_access_control: false,
enable_function_values: true,

max_function_return_values: Some(128),
Expand Down
Loading