Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Enforce read-only nature of an action with read-only attribute #1068

Merged
merged 37 commits into from
May 24, 2021

Conversation

bogniq
Copy link
Collaborator

@bogniq bogniq commented Mar 16, 2021

Change Description

This PR adds checking of eosio:read_only attribute for actions. Currently, it only supports checking for direct calls and some ways of indirect calls of specific write host functions.

If compiler option --warn-action-read-only is used, even if some read-only action calls write host functions, the contract compilation still continues but will output warning messages. Otherwise, the compilation will stop and output error messages.

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

tools/include/eosio/gen.hpp Outdated Show resolved Hide resolved
@bogniq bogniq requested a review from larryk85 April 13, 2021 18:09
@bogniq bogniq requested a review from jgiszczak April 27, 2021 03:51
@bogniq bogniq marked this pull request as ready for review May 10, 2021 14:57
.gitmodules Outdated
@@ -9,6 +9,7 @@
[submodule "eosio_llvm"]
path = eosio_llvm
url = https://github.com/eosio/llvm
branch = qy-read-only-action
Copy link
Contributor

Choose a reason for hiding this comment

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

This branch should be the 'eosio' branch of llvm. So, please make a PR to LLVM to the branch 'eosio' and then update this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

EOSIO/llvm#29 was just created, which updates clang to the head of its 'eosio' branch (with read-only attribute added). The branch setting here will be removed after the LLVM PR is merged.


virtual bool VisitFunctionDecl(FunctionDecl* func_decl) {
SourceManager &sm = get_rewriter().getSourceMgr();
if (sm.isInSystemHeader(func_decl->getLocation()) || sm.isInExternCSystemHeader(func_decl->getLocation())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fine short circuit. But, we do have a few places in our C++ and C headers where we have some extern C <writable_host_functions> to supply the underlying bases for libc. This will more than likely miss those calls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This system header checking is deleted in the new commit:

virtual bool VisitFunctionDecl(FunctionDecl* func_decl) {
if (func_calls.count(func_decl) == 0 && is_write_host_func(func_decl)) {


inline bool is_write_host_func( const std::string& t ) {
static const std::set<std::string> write_host_funcs =
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend only using the C name for looking for these host functions.
I.e. instead of eosio::internal_use_do_not_use::set_resource_limits, just look for the stripped name of set_resource_limits and if it is a extern-C function or a regular C function.

The reason for this, is that as you can see, you have to duplicate some of these, also the C++ namespaced names are purely decorative and just supplying context for the smart contract developer. You should still only have one set_resource_limits per smart contract.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. (Method is_eosio_wasm_import_write_func is also removed, because when using C name to find write host functions, the updated is_write_host_func method can also handle the "eosio_wasm_import" functions. Please let me know if I missed anything. Thanks!)

inline bool is_write_host_func( const clang::FunctionDecl *func_decl ) {
static const std::set<std::string> write_host_funcs =
{
"set_resource_limits",
"set_wasm_parameters_packed",
"set_resource_limit",
"set_proposed_producers",
"set_proposed_producers_ex",
"set_blockchain_parameters_packed",
"set_parameters_packed",
"set_kv_parameters_packed",
"set_privileged",
"db_store_i64",
"db_update_i64",
"db_remove_i64",
"db_idx64_store",
"db_idx64_update",
"db_idx64_remove",
"db_idx128_store",
"db_idx128_update",
"db_idx128_remove",
"db_idx256_store",
"db_idx256_update",
"db_idx256_remove",
"db_idx_double_store",
"db_idx_double_update",
"db_idx_double_remove",
"db_idx_long_double_store",
"db_idx_long_double_update",
"db_idx_long_double_remove",
"kv_erase",
"kv_set",
"send_deferred",
"send_inline",
"send_context_free_inline"
};
return write_host_funcs.count(func_decl->getNameInfo().getAsString()) >= 1;
}

@bogniq bogniq requested a review from larryk85 May 21, 2021 20:28
@bogniq bogniq merged commit bef15f5 into develop May 24, 2021
@bogniq bogniq deleted the qy-read-only-action branch July 2, 2021 17:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants