Skip to content

Commit

Permalink
[diem-vm] vm was ignoring upgrade compatibility requests. (#6)
Browse files Browse the repository at this point in the history
  • Loading branch information
@0o-de-lally
0o-de-lally committed Mar 1, 2024
1 parent e18ba55 commit 7b843bc
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 13 deletions.
27 changes: 18 additions & 9 deletions diem-move/diem-vm/src/diem_vm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -956,7 +956,7 @@ impl DiemVM {
bundle,
expected_modules,
allowed_deps,
check_compat: _,
check_compat,
}) = session.extract_publish_request()
{
// TODO: unfortunately we need to deserialize the entire bundle here to handle
Expand All @@ -977,21 +977,30 @@ impl DiemVM {
}
}

//////// 0L ////////
// vendor code is not actually checking the upgrade preferences
// sometimes 0x1 needs to make an arbitrary code upgrade.
let compat_cfg = if check_compat {
Compatibility::new(
true,
true,
!self
.0
.get_features()
.is_enabled(FeatureFlag::TREAT_FRIEND_AS_PRIVATE),
)
} else {
Compatibility::no_check()
};
//////// end 0L ////////
// Publish the bundle and execute initializers
// publish_module_bundle doesn't actually load the published module into
// the loader cache. It only puts the module data in the data cache.
return_on_failure!(session.publish_module_bundle_with_compat_config(
bundle.into_inner(),
destination,
gas_meter,
Compatibility::new(
true,
true,
!self
.0
.get_features()
.is_enabled(FeatureFlag::TREAT_FRIEND_AS_PRIVATE),
),
compat_cfg, //////// 0L ////////
));

self.execute_module_initialization(
Expand Down
29 changes: 25 additions & 4 deletions third_party/move/move-binary-format/src/compatibility.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,11 @@ impl Compatibility {
let mut struct_layout = true;
let mut friend_linking = true;

let mut err_message = "compatibility: \n".to_owned();

// module's name and address are unchanged
if old_module.address != new_module.address || old_module.name != new_module.name {
err_message.push_str(&format!("\n- module name mismatch: {:?}", new_module.name));
struct_and_pub_function_linking = false;
}

Expand All @@ -87,7 +90,10 @@ impl Compatibility {
None => {
// Struct not present in new . Existing modules that depend on this struct will fail to link with the new version of the module.
// Also, struct layout cannot be guaranteed transitively, because after
// removing the struct, it could be re-added later with a different layout.
// removing the struct, it could be re-added later with a
// different layout.
err_message.push_str(&format!("\n- struct is no longer present: {:?}", &name));

struct_and_pub_function_linking = false;
struct_layout = false;
break;
Expand All @@ -100,6 +106,7 @@ impl Compatibility {
&new_struct.type_parameters,
)
{
err_message.push_str(&format!("\n- struct abilities or types incompatible: {:?}", &old_struct));
struct_and_pub_function_linking = false;
}
if new_struct.fields != old_struct.fields {
Expand All @@ -109,6 +116,9 @@ impl Compatibility {
// choose that changing the name (but not position or type) of a field is
// compatible. The VM does not care about the name of a field
// (it's purely informational), but clients presumably do.

err_message.push_str(&format!("\n- new struct fields mismatch: {:?}", &new_struct.fields));

struct_layout = false
}
}
Expand All @@ -132,8 +142,12 @@ impl Compatibility {
Some(new_func) => new_func,
None => {
if matches!(old_func.visibility, Visibility::Friend) {
err_message.push_str(&format!("\n- missing friend function: {:?}", &name));

friend_linking = false;
} else {
err_message.push_str(&format!("\n- missing exposed function: {:?}", &name));

struct_and_pub_function_linking = false;
}
continue;
Expand Down Expand Up @@ -171,8 +185,10 @@ impl Compatibility {
)
{
if matches!(old_func.visibility, Visibility::Friend) {
err_message.push_str(&format!("\n- function visibility: {:?}", &name));
friend_linking = false;
} else {
err_message.push_str(&format!("\n- no exposed function: {:?}", &name));
struct_and_pub_function_linking = false;
}
}
Expand All @@ -186,23 +202,28 @@ impl Compatibility {
let old_friend_module_ids: BTreeSet<_> = old_module.friends.iter().cloned().collect();
let new_friend_module_ids: BTreeSet<_> = new_module.friends.iter().cloned().collect();
if !old_friend_module_ids.is_subset(&new_friend_module_ids) {
err_message.push_str(&format!("\n- missing friend linking: {:?}", &old_module.friends.iter()));

friend_linking = false;
}

if self.check_struct_and_pub_function_linking && !struct_and_pub_function_linking {
dbg!(format!("module error: {:#?}, check_struct_and_pub_function_linking {:#?}", &old_module.name, &err_message));
return Err(PartialVMError::new(
StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE,
));
).with_message(err_message));
}
if self.check_struct_layout && !struct_layout {
dbg!(format!("module error: {:#?}, check_struct_layout {:#?}",&old_module.name, &err_message));
return Err(PartialVMError::new(
StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE,
));
).with_message(err_message));
}
if self.check_friend_linking && !friend_linking {
dbg!(format!("module error: {:#?}, check_friend_linking {:#?}", &old_module, &err_message));
return Err(PartialVMError::new(
StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE,
));
).with_message(err_message));
}

Ok(())
Expand Down

0 comments on commit 7b843bc

Please sign in to comment.