SC: Sync call entry point validation for `eosvm`, `eosvm-jit`, and `eosvmoc` #1455

linh2931 · 2025-04-28T14:12:47Z

This PR implements a new method to validate sync call entry point. It is applicable to eosvm, eosvm-jit, and eosvmoc. When an account code is deployed, check whether sync_call entry point exists. If it does, its signature is checked. A flag sync_call_supported is added to code_object. Before a sync call is executed, sync_call_supported is used to determine whether the call be proceeded.

sync_call_supported is not written into snapshots. It is populated from the code when reading from snapshots.

Resolves #1219 and #1392

…ation

linh2931 · 2025-04-28T15:00:11Z

libraries/chain/eosio_contract.cpp

   int64_t code_size = (int64_t)act.code.size();

   if( code_size > 0 ) {
     code_hash = fc::sha256::hash( act.code.data(), (uint32_t)act.code.size() );
-     wasm_interface::validate(context.control, act.code);
+     wasm_interface::validate(context.control, act.code, sync_call_supported);


I was debating whether I should do the sync_call check here inside validate(), or call is_sync_call_supported() separately so not changing validate()'s signature. I opted to do it in validate() to save reconstruction of vm::backend from the code and an extra call to is_sync_call_supported().

Returning a struct of bools (only sync_call_supported for now) would be better I think.

Initially I thought about returning a bool, but that does not sound right as the method name is validate. A struct of bools is a better choice. Thanks.

libraries/chain/controller.cpp

spoonincode · 2025-04-28T15:39:18Z

libraries/chain/include/eosio/chain/code_object.hpp

+      uint8_t      vm_type = 0;
+      uint8_t      vm_version = 0;
+   };
+


I'm a little confused on this new snapshot_code_object_v1 because it's the same as the code_object previously was serialized. Also, when we're converting from different chainbase<->snapshot types usually (always?) we use snapshot_row_traits which I don't see anywhere in here.

I'm suspicious we don't need any of these snapshot changes and can instead add a code_object::reflector_init() which just populates the new sync_call_supported field. (this would mean that we'd not include sync_call_supported as part of the fc_reflect which is a little weird but not unheard of)

I'm not 100% confident on the above but I'd suggest trying it

The new snapshot_code_object_v1 is for snapshot versions prior to sync call protocol feature. We have similar structs for block_state and chain_config.

Will try using code_object::reflector_init(). Thanks.

ah okay yeah I saw maximum_version = 8; but I forgot we were on 9 on this branch

Thanks!

code_object::reflector_init() works. We don't need to place sync_call_supported into snapshots.

spoonincode

Looks really good

spoonincode · 2025-04-29T01:23:45Z

libraries/chain/eosio_contract.cpp

@@ -174,6 +175,7 @@ void apply_eosio_setcode(apply_context& context) {
      if( new_code_entry ) {
         db.modify(*new_code_entry, [&](code_object& o) {
            ++o.code_ref_count;
+            o.sync_call_supported = sync_call_supported;


This line is unnecessary.

spoonincode · 2025-04-29T01:42:39Z

libraries/chain/wasm_interface.cpp

      const auto& pso = control.db().get<protocol_state_object>();

      if (control.is_builtin_activated(builtin_protocol_feature_t::configurable_wasm_limits)) {
         const auto& gpo = control.get_global_properties();
-         webassembly::eos_vm_runtime::validate( control, code, gpo.wasm_configuration, pso.whitelisted_intrinsics );
+         webassembly::eos_vm_runtime::validate( control, code, gpo.wasm_configuration, pso.whitelisted_intrinsics, sync_call_supported );


I know it is academic for Vaulta, but in the case where CONFIGURABLE_WASM_LIMITS2 has not be activated we fall down below and don't populate sync_call_supported. We really should populate it both places.

I thought about it. Because sync_call depends on all previous protocol features, CONFIGURABLE_WASM_LIMITS2 should have been activated. That's why I did not add to the other validate(). But will add for completeness and for not missing some corner cases.

linh2931 · 2025-04-29T12:20:21Z

Looks really good

Thanks for all your suggestions

…t been activated

libraries/chain/code_object.cpp

libraries/chain/webassembly/runtimes/eos-vm.cpp

heifner · 2025-04-29T12:40:34Z

libraries/chain/host_context.cpp

@@ -101,6 +101,11 @@ int64_t host_context::execute_sync_call(name call_receiver, uint64_t flags, std:
            return handle_call_failure();
         }

+         const code_object* const codeobject = db.find<code_object, by_code_hash>(boost::make_tuple(receiver_account->code_hash, receiver_account->vm_type, receiver_account->vm_version));


Since you already look it up here. Seems like it would be cleaner to pass code_object into execute. Then code_hash, vm_type, and vm_version do not have to be passed in.

apply_context uses the same execute() and it does not dip code_object. Even if we pass in code_object here, we still need to split it and pass in code_hash, vm_type and vm_version before calling wasm_interface_private::execute() to reuse the same code.

I think apply_context might as well look it up and pass it in. In either case it is needed inside wasm_interface_private::execute().

It only looks code_object when the code is not cached. We will need two execute()s with different signatures to avoid unnecessary lookup for apply_context. Can we do this refactor in a separate PR?

Good point about code_object not always needing to be looked up. Maybe we should cache the sync_call_supported in wasm_cache_entry and code_descriptor. That would mean it would have to validated in execute().

We already have call_offset for oc we can check. Seems a bit fragile to rely on the JIT check, so probably should check it. So add it to the wasm_cache_entry?

We already have call_offset for oc we can check

Not really though, since it isn't checking for signature now.

In current VM JIT implementation, it will crash if sync_call() is not exported. In OC, we have an assert if code.call_offset is 0.

I plan to do a separate PR for the sync call case to avoid second look up of code_object in wasm_interface_private::get_or_build_instantiated_module().

#1461

Will maintain the current simplicity

heifner · 2025-04-29T12:43:52Z

libraries/chain/eosio_contract.cpp

   int64_t code_size = (int64_t)act.code.size();

   if( code_size > 0 ) {
     code_hash = fc::sha256::hash( act.code.data(), (uint32_t)act.code.size() );
-     wasm_interface::validate(context.control, act.code);
+     wasm_interface::validate(context.control, act.code, sync_call_supported);


Returning a struct of bools (only sync_call_supported for now) would be better I think.

…ted by reference

…ntry point function

spoonincode · 2025-05-01T15:48:10Z

libraries/chain/webassembly/runtimes/eos-vm.cpp

+   wasm_code_ptr code_ptr((uint8_t*)code_bytes, code_size);
+   bool supported = false;
+   try {
+      eos_vm_null_backend_t<setcode_options> bkend(code_ptr, code_size, nullptr);


afaict this is good now, but it's something to be mindful of in the future -- if we ever relax WASM validation via some protocol feature in the future, this could cause problems.

What problems might be? Do you mean previously non-supported becomes supported?

When activating CONFIGURABLE_WASM_LIMITS2 validation becomes more strict on setcode to prevent some violations of wasm spec. (contracts that violate these rules would still work after activation but could not be setcode again)

So creating a backend with pre-CONFIGURABLE_WASM_LIMITS2 rules is okay since CONFIGURABLE_WASM_LIMITS2 is more strict.

However let's say we made a protocol feature that relaxed validation for the "read only host functions" we've considered; let's just call that READONLY_HF. This would be a problem because here we'd not allow that relaxation since we're still validating against pre-READONLY_HF (and pre-CONFIGURABLE_WASM_LIMITS2) rules. We'd need access to what protocol features are enabled to know what kind of backend to create, and we don't have access to that information readily in the reflector_init()

Thanks for the detailed explanation. If that happens, we will have to save sync_call_supported in the snapshot, so that the caller of is_sync_call_supported() can pass in controller to have access to protocol feature set. I will add a comment.

…idation

…ion ie relaxed

linh2931 added 3 commits April 28, 2025 09:55

Add sync_call_supported to code_object and use it for sync call valid…

cee8fe4

…ation

Update snapshot handling to read snapshot_code_object_v1

84d7222

Re-enable sync call entry point validation tests

b8e74eb

linh2931 requested review from spoonincode and heifner April 28, 2025 14:13

linh2931 added 2 commits April 28, 2025 10:25

Update snapshot compatible version test files

321481f

Update reference snapshot file for verify_block_compatibitity test

91aad6f

linh2931 commented Apr 28, 2025

View reviewed changes

libraries/chain/controller.cpp Outdated Show resolved Hide resolved

spoonincode reviewed Apr 28, 2025

View reviewed changes

linh2931 added 2 commits April 28, 2025 15:35

No need to place sync_call_supported in snapshots

0d047f1

Revert snapshot test data files

bd95d68

spoonincode reviewed Apr 29, 2025

View reviewed changes

linh2931 added 2 commits April 29, 2025 08:23

Remove an unnessary update of sync_call_supported

fa7a3c6

Populate sync_call_supported even if CONFIGURABLE_WASM_LIMITS2 has no…

13e5f5e

…t been activated

heifner suggested changes Apr 29, 2025

View reviewed changes

linh2931 added 2 commits April 29, 2025 09:54

remove unnecessary static_assert for FC support of reflector_init

fc69fd0

Make validate() return a struct of bool instead pass sync_call_suppor…

e63966f

…ted by reference

linh2931 mentioned this pull request Apr 29, 2025

SC: Avoid the second look up of code_object in wasm_interface_private::get_or_build_instantiated_module #1461

Open

Add a test for validating the number of parameters in the sync_call e…

14cb1e3

…ntry point function

spoonincode approved these changes May 1, 2025

View reviewed changes

linh2931 and others added 2 commits May 1, 2025 21:14

Merge branch 'revert_entry_point_validation' into new_entry_point_val…

80aaab9

…idation

Add a comment about future change of creating backend if WASM validat…

96076ac

…ion ie relaxed

Base automatically changed from revert_entry_point_validation to sync_call May 7, 2025 22:02

Merge branch 'sync_call' into new_entry_point_validation

729bb53

heifner approved these changes May 8, 2025

View reviewed changes

linh2931 merged commit 2e4f2c2 into sync_call May 8, 2025
28 checks passed

linh2931 deleted the new_entry_point_validation branch May 8, 2025 13:00

linh2931 restored the new_entry_point_validation branch July 5, 2025 20:05

SC: Sync call entry point validation for eosvm, eosvm-jit, and eosvmoc #1455

SC: Sync call entry point validation for eosvm, eosvm-jit, and eosvmoc #1455

Uh oh!

Conversation

linh2931 commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spoonincode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linh2931 commented Apr 29, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SC: Sync call entry point validation for `eosvm`, `eosvm-jit`, and `eosvmoc` #1455

SC: Sync call entry point validation for `eosvm`, `eosvm-jit`, and `eosvmoc` #1455

linh2931 commented Apr 28, 2025 •

edited

Loading