SC: Enforce host function preconditions for sync calls #1296
Conversation
| * @throws missing_auth_exception If no sufficient permission was found | ||
| */ | ||
| virtual void require_authorization(const account_name& account) = 0; | ||
| virtual void require_authorization(const account_name& account) { EOS_ASSERT(false, unaccessible_api, "default implemention should never be used"); } // This function should never be called in sync calls due to require_auth host wrapper preconditions. This EOS_ASSERT double makes sure that. |
There was a problem hiding this comment.
Since it's impossible to get here would assert() be a more appropriate choice for these?
There was a problem hiding this comment.
Yes, I was debating over assert or EOS_ASSERT. Now I think about it more, assert is more appropriate as it is a coding error to get to this point. Will change.
| virtual int get_action( uint32_t type, uint32_t index, char* buffer, size_t buffer_size )const = 0; | ||
| virtual int get_context_free_data( uint32_t index, char* buffer, size_t buffer_size )const = 0; | ||
| virtual int get_action( uint32_t type, uint32_t index, char* buffer, size_t buffer_size ) const { EOS_ASSERT(false, unaccessible_api, "default implemention should never be used"); } | ||
| virtual int get_context_free_data( uint32_t index, char* buffer, size_t buffer_size )const { EOS_ASSERT(false, unaccessible_api, "default implemention should never be used"); } |
There was a problem hiding this comment.
well, I guess for these couldn't just do an assert(), but would also need a __builtin_unreachable() to silence the warning of not having a return value
| std::string _pending_console_output; | ||
|
|
||
| const bool _is_action = true; // used to avoid using dynamic_cast to identify type of host_context | ||
| const bool _is_sync_call = false; // used to avoid using dynamic_cast to identify type of host_context |
There was a problem hiding this comment.
An alternative to these would be for is_action() and is_sync_call() to be virtual and return false on host_context, and then apply_context and sync_call_context can override the appropriate one to return true. I'm indifferent on which approach to take, just mentioning it.
There was a problem hiding this comment.
I like your idea. It removes 2 member fields and makes the constructor a little simpler.
…f functions which should have never be called
…r returning a 0-length value
SC: Tests for sync calls without parameters, not returning a value, or returning a 0-length value
SC: privilege support
The following host functions are not allowed to be called from sync calls:
require_auth(),require_auth2(),has_auth(),require_recipient(),read_action_data(),action_data_size(),set_action_return_value(),get_context_free_data(),send_inline(),send_context_free_inline(),send_deferred(),cancel_deferred()The following host functions are not allowed to be called from actions:
get_call_data(),set_call_return_value()This PR enforces those preconditions via host function definition wrappers, which is cleaner than the checks are scattered in the function implementation. Thanks @spoonincode for the suggestion!
For each of the host functions, a test is added to verify the precondition is checked.
Resolves #1218