SC: Validate sync call entry point #1266

linh2931 · 2025-03-21T19:20:11Z

Implement this feature earlier to help prevent errors in WASM test contracts.

Before a contract is inserted into wasm_instantiation_cache, check if sync_call entry point is present in the contract; if present, validate the signature. Set receiver_supports_sync_call flag of the cache entry accordingly.
When a sync call is made, check receiver_supports_sync_call of the receiver. If false, return -1 and let the caller decide what to do about the failure.
Tests are added for those validation.

Resolves #1219

…ture is activated, enforce no_op_if_receiver_not_support_sync_call flag

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

…ode complex, hard to follow, and not needed for actions. Now check if sync_call entry point directly in do_sync_call which is simple and natural

libraries/chain/sync_call_context.cpp

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

…EOS_ASSERT error text

spoonincode · 2025-03-24T16:04:29Z

libraries/chain/sync_call_context.cpp

+}
+
+bool sync_call_context::no_op_if_receiver_not_support_sync_call()const {
+   return flags & 0b10;  // second bit from LSB. bit index 1


Is there some pattern we can use to get rid of the naked bit constants here and

spring/libraries/chain/apply_context.cpp

Line 262 in cdb1beb

EOS_ASSERT((flags & ~0b11) == 0, sync_call_validate_exception, // all but last 2 bits are 0s

Simplest being something like

enum sync_call_flags { read_only = 1<<0; noop.. = 1<<1; last = noop..; }

then flags are valid if flags <= last.

Lots of other potential approaches too.

Thanks!

287e1c5

If we are getting rid of the naked bit constants, I would recommend you do it like this:

spring/libraries/chain/include/eosio/chain/account_object.hpp

Lines 67 to 72 in e186a90

bool is_privileged()const { return has_field( flags, flags_fields::privileged ); }

void set_privileged( bool privileged ) {

flags = set_field( flags, flags_fields::privileged, privileged );

}

};

Either way is fine with me. @spoonincode Do you have any preferences?

No preference from me; using has_field() seems, if I'm understanding right, to offer some minor additional paranoia that is good.

My main disappointment is that there is no automatic way to get the last item, thus requiring us to manually specify it. My hunch is it's possible to construct something fancy with constexpr to resolve that but not something I'm advocating we do now.

I will change to use has_field() so it is more consistent with existing code style.

Looked at this further. We need to verify bits other than the first bit can have a value. The has_field does not provide a way to do that. Looks like the existing implementation is a simple one.

spoonincode · 2025-03-24T16:50:50Z

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

+            }
+         }
+         // If the contract contains sync_call entry point, its signature had already been
+         // validated when the contract was deployed.


I don't think this comment is entirely correct -- its signature will be validated provided that the contract was deployed after the activation of SYNC_CALL. If someone deploys a contract that exports sync_call with an unexpected signature prior to activation of SYNC_CALL we will have never checked its signature ahead of time.

In this case here it seems okayish (not UB or such) because EOS VM does validation on each call() that the parameters and parameter count match up with the export. But it does mean if code is deployed with bad signature prior to SYNC_CALL and the call is made with no_op_if_receiver_not_support_sync_call you'll not get a noop but rather some other eosvm assert. In practice we would never expect this situation, but it is quirky behavior enshrined in the protocol we would need to remember and replicate in OC too. I feel like we should tighten this up so the rule is simpler to describe.

i.e. the rule should be something like "when a sync call is made to a contract that does not export an expected sync_call, it aborts with sync_call_not_supported_by_receiver_exception unless the sync call flag no_op_if_receiver_not_support_sync_call is set which makes it a noop"

I thought last time this topic was discussed we were going to cache away a bool somewhere that indicates if sync_call is properly exported. Ideally we do that in a common location so we are not repeating this check in each wasm runtime. Unfortunately I'm not sure exactly where that should go -- I initially thought wasm_cache_entry was the right place but seems not since OC can skip it.

Personally I don't think we should bother adding additional checks to setcode because even if we add a check there we still have have to be prepared for a mismatched sync_call anyways -- the check doesn't really help simplify anything in nodeos.¹ Not sure how strongly I feel about it in this case.

Footnotes

There is an argument that the check in setcode makes it more upfront to a user they got something wrong. There is also an argument the other way that by making the validation more strict we potentially prevent valid code that is deployed today from being redeployed -- though of course that's very academic for this scenario. ↩

I like your simplified rule -- not checking the signature.

But we will miss one feature "someone wants a no-op if the signature is bad". But I think it is very rare and we don't need to support it, at least for now.

"expected sync_call" was meant to imply proper signature.

So the rule should be something like

When a sync call is made to a contract that does not export sync_call with a proper signature, it aborts with sync_call_not_supported_by_receiver_exception unless the sync call flag no_op_if_receiver_not_support_sync_call is set which makes it a noop

The code does not follow this simple rule right now. The behavior is more complex -- behavior is dependent on when the code was deployed.

Per team discussion, if the receiver does not have valid entry point, return -1 and let the caller decide what to do about the failure.

4aead0a

…contract deployment, cache a flag indicating whethere a contract is a valid entry point, at execution check the flag and return -1 if the receiver does not have the valid sync call entry point

spoonincode · 2025-04-01T19:32:48Z

libraries/chain/apply_context.cpp

-   EOS_ASSERT((flags & 0xFFFFFFFFFFFFFFFE) == 0, sync_call_validate_exception,
-              "sync call's flags ${f} can only set bit 0", ("f", flags));
+   EOS_ASSERT(flags <= static_cast<uint64_t>(sync_call_flags::last), sync_call_validate_exception,  // all but last `sync_call_flags::last` bits must be 0s
+              "only ${last} least significant bits of sync call's flags (${flags}) can be set", ("last", static_cast<uint64_t>(sync_call_flags::last))("flags", flags));


Using sync_call_flags::last here doesn't seem right. If last is 1<<3 then this message will say "only 8 least significant bits of sync call's flags (${flags}) can be set". That value should be 4 instead of 8. I think you want the std::bit_width(sync_call_flags::last)

Thanks for noticing this!

64eb928

I think the check flags <= static_cast<uint64_t>(sync_call_flags::last) is wrong. For example if two bits out of two were set, flags would be 3 and static_cast<uint64_t>(sync_call_flags::last) would be 2.

Thanks. I renamed last and added comments to prevent future confusions. Also added flags validation tests.

spoonincode · 2025-04-01T19:44:27Z

libraries/chain/include/eosio/chain/webassembly/interface.hpp

          *
         */
-         uint32_t call(name receiver, uint64_t flags, span<const char> data);
+         int32_t call(name receiver, uint64_t flags, span<const char> data);


I know this is academic but it's the cost of CONFIGURABLE_WASM_LIMITS2's infinite knobs: int32_t isn't enough for greater than 2GB unless the contract treats this as unsigned. Unless there is a compelling reason to keep it 32-bit I'd suggest moving to int64_t so that the semantics are clearer for these academic edge cases.

The value returned by call() (the number of sync call return value) is used as the size component for constructing memory argument in get_call_return_value(span<char> memory). In WASM, the memory span is represented as (param i32 i32). Is it safe to cast down from i64 to i32 when the user uses it?

Another point: the return value size is limited by max_sync_call_data_size, which is likely less than int32_t size.

@arhag Any comment about this?

I don't really have a position on this one.

To prevent any potential edge cases, I changed the return type to int64_t.

Thanks.

f6605d3

spoonincode · 2025-04-01T19:44:51Z

libraries/chain/include/eosio/chain/webassembly/runtime_interface.hpp

   public:
      virtual std::unique_ptr<wasm_instantiated_module_interface> instantiate_module(const char* code_bytes, size_t code_size,
-                                                                                     const digest_type& code_hash, const uint8_t& vm_type, const uint8_t& vm_version) = 0;
+                                                                                     const digest_type& code_hash, const uint8_t& vm_type, const uint8_t& vm_version, bool& sync_call_supporte) = 0;


…d comments about how to add new flags, add tests for flags validation

spoonincode

We are going to need to do some non trivial refactoring when wiring up OC but we can keep this as is for now

…w return type i64_t for call() host function

linh2931 · 2025-04-02T20:40:41Z

We are going to need to do some non trivial refactoring when wiring up OC but we can keep this as is for now

I copied the link to this comment to the OC issue #1043

Validate sync call entry point if present and sync call protocal fear…

17abffc

…ture is activated, enforce no_op_if_receiver_not_support_sync_call flag

linh2931 requested review from heifner and spoonincode March 21, 2025 19:20

linh2931 commented Mar 22, 2025

View reviewed changes

libraries/chain/webassembly/runtimes/eos-vm.cpp Show resolved Hide resolved

linh2931 added 2 commits March 22, 2025 18:58

Do not cache sync_call_supported in the wasm code cache which makes c…

e94850e

…ode complex, hard to follow, and not needed for actions. Now check if sync_call entry point directly in do_sync_call which is simple and natural

Restore original get_instantiated_module signature

ebce5e2

heifner suggested changes Mar 24, 2025

View reviewed changes

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

libraries/chain/webassembly/runtimes/eos-vm.cpp Outdated Show resolved Hide resolved

libraries/chain/webassembly/runtimes/eos-vm.cpp Outdated Show resolved Hide resolved

linh2931 added 4 commits March 24, 2025 09:48

Remove std::move the data span

bbe2ffa

Make the sync call entry point wrong function siganture clear in the …

d84de41

…EOS_ASSERT error text

Remove debug log for no sync call entry point but the op-op flag is set

d76df03

Update bad_entry_point_test for new exception text

cdb1beb

heifner approved these changes Mar 24, 2025

View reviewed changes

spoonincode reviewed Mar 24, 2025

View reviewed changes

linh2931 added 2 commits March 24, 2025 14:35

Use enum for sync_call_flags

287e1c5

Remove use of no_op_if_receiver_not_support_sync_call

cbfed70

Base automatically changed from params_return_value to sync_call March 31, 2025 21:55

Major rework of entry point validation: do not validate sync_call at …

4aead0a

…contract deployment, cache a flag indicating whethere a contract is a valid entry point, at execution check the flag and return -1 if the receiver does not have the valid sync call entry point

spoonincode reviewed Apr 1, 2025

View reviewed changes

linh2931 changed the title ~~SC: Validate sync call entry point if it it present and sync call protocol feature is activated; enforce no_op_if_receiver_not_support_sync_call flag~~ SC: Validate sync call entry point Apr 1, 2025

linh2931 and others added 6 commits April 1, 2025 16:03

Skip eos_vm_oc for now

95ddc14

Use std::bit_width to get correct number of valid flags bits

64eb928

Correct a typo in parameter name

268deaa

Rename sync_call_flags::last to sync_call_flags::all_allowed_bits, ad…

a7945b5

…d comments about how to add new flags, add tests for flags validation

Merge branch 'sync_call' into validate_entry_point

7e1cf84

Change call host function return type from int32_t to int64_t

f6605d3

spoonincode approved these changes Apr 2, 2025

View reviewed changes

Update protocol_feature_tests/sync_call_activation_test to use the ne…

21a8b90

…w return type i64_t for call() host function

linh2931 mentioned this pull request Apr 2, 2025

Make OC allow for nested Executor execution #1043

Closed

linh2931 merged commit 3f14d16 into sync_call Apr 2, 2025
36 checks passed

linh2931 deleted the validate_entry_point branch April 2, 2025 23:11

	bool is_privileged()const { return has_field( flags, flags_fields::privileged ); }

	void set_privileged( bool privileged ) {
	flags = set_field( flags, flags_fields::privileged, privileged );
	}
	};

SC: Validate sync call entry point #1266

SC: Validate sync call entry point #1266

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

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!

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

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

Footnotes

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!

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

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!

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

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!

linh2931 commented Apr 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

linh2931 commented Mar 21, 2025 •

edited

Loading

linh2931 Mar 24, 2025 •

edited

Loading

greg7mdp Apr 1, 2025 •

edited

Loading

linh2931 Apr 2, 2025 •

edited

Loading