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
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
a46cde2
add testing smart contract for read-only query
bogniq Feb 26, 2021
2839f32
add FunctionDecl visitor
bogniq Mar 9, 2021
0c305e5
Add read only query test contract as a part of read only query TDD.
softprofe Mar 10, 2021
51233fd
Add host functions test actions. not finished, continue next week
softprofe Mar 13, 2021
2abede0
Add test action for some host functions
softprofe Mar 16, 2021
f3c7af6
add implementation for read-only checking
bogniq Mar 16, 2021
0d0d176
add checking for extern c function
bogniq Mar 19, 2021
70b08b6
fix if condition for process_function
bogniq Mar 22, 2021
b231e24
add option -warn-action-read-only
bogniq Mar 24, 2021
439bb39
Add test case for inline host functions.
softprofe Mar 25, 2021
52cebae
Add alias test for host functions
softprofe Mar 25, 2021
3502972
add support for CDT_WARN/ERROR in codegen
bogniq Mar 29, 2021
087fa9b
Add two more chained indirect call test actions
softprofe Mar 30, 2021
602b057
set git branch for submodule eosio_llvm
bogniq Mar 30, 2021
c99f002
add more checkings; clean up code
bogniq Mar 31, 2021
486c04e
update commit hash for submodule eosio_llvm
bogniq Apr 1, 2021
529e50b
Merge branch 'develop' into qy-read-only-action
bogniq Apr 1, 2021
7544797
Move test contract files so as to easily used by team.
softprofe Apr 1, 2021
17c82b0
Merge remote-tracking branch 'origin/keke_epe748' into qy-read-only-a…
bogniq Apr 2, 2021
9314f35
add toolchain tests
bogniq Apr 2, 2021
2818077
Change flag to read_only for get() action
softprofe Apr 2, 2021
e96c14e
Merge remote-tracking branch 'origin/keke_epe748' into qy-read-only-a…
bogniq Apr 2, 2021
4ff7792
add regex support for toolchain tester; fix string compare
bogniq Apr 6, 2021
ec018c2
Using get_self instead of "eosio"_n so as to decouple tables with "eo…
softprofe Apr 7, 2021
3c77cf7
Merge remote-tracking branch 'origin/keke_epe748' into qy-read-only-a…
bogniq Apr 7, 2021
5b9b142
Merge branch 'develop' into qy-read-only-action
bogniq Apr 12, 2021
3bcf551
check local function pointers
bogniq Apr 19, 2021
61a24c0
fix read-only action calling issue
bogniq Apr 19, 2021
d8a16d3
Merge branch 'develop' into qy-read-only-action
bogniq Apr 19, 2021
17a0a29
check global function pointer for write function
bogniq Apr 22, 2021
e4beb61
check class data members for write funciton
bogniq Apr 26, 2021
add76f9
fix function pointer reassignment
bogniq Apr 26, 2021
f5cef84
code clean-up for different statement types
bogniq Apr 29, 2021
0edea87
add support for multiple declarations; clean up code
bogniq May 10, 2021
69f6916
Merge branch 'develop' into qy-read-only-action
bogniq May 18, 2021
ef00c9d
remove system header checking; use C name to find write host function
bogniq May 21, 2021
bf9c37a
change llvm branch to eosio
bogniq May 24, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -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.

[submodule "libraries/native/softfloat"]
path = libraries/native/softfloat
url = https://github.com/EOSIO/berkeley-softfloat-3.git
2 changes: 1 addition & 1 deletion eosio_llvm
Submodule eosio_llvm updated 2 files
+1 −0 .gitmodules
+1 −1 tools/clang
48 changes: 48 additions & 0 deletions tests/unit/test_contracts/read_only_tests.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#include <eosio/eosio.hpp>
#include <eosio/privileged.hpp>

using namespace eosio;
extern "C" __attribute__((weak)) __attribute__((eosio_wasm_import)) void set_resource_limit(int64_t, int64_t, int64_t);

class [[eosio::contract]] read_only_tests : public contract {
public:

[[eosio::action, eosio::read_only]]
void test1( name user, int64_t limit ) {
print( "Hello, ", user);
set_resource_limit(user.value, 0, 0);
}

[[eosio::action]]
void test2( name user, int64_t limit ) {
internal_use_do_not_use::set_resource_limits(user.value, 0, 0, 0);
}

[[eosio::action, eosio::read_only]]
void test3( name user, int64_t limit ) {
internal_use_do_not_use::set_resource_limits(user.value, 0, 0, 0);
}

[[eosio::action, eosio::read_only]]
void test4( name user, int64_t limit ) {
foo1(user);
}

[[eosio::action, eosio::read_only]]
void test5( name user, int64_t limit ) {
foo3(user);
}

void foo1(name user) {
foo(user);
}

void foo(name user) {
internal_use_do_not_use::set_resource_limits(user.value, 0, 0, 0);
}

void foo3(name user) {
set_resource_limit(user.value, 0, 0);
}

};
7 changes: 4 additions & 3 deletions tools/cc/eosio-cpp.cpp.in
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ using namespace eosio::cdt;

void handle_empty_abigen(const std::string& contract_name, bool has_o_opt, bool has_contract_opt);

void generate(const std::vector<std::string>& base_options, std::string input, std::string contract_name, const std::vector<std::string>& resource_paths, const std::pair<int, int>& abi_version, bool abigen, bool suppress_ricardian_warning, bool has_o_opt, bool has_contract_opt) {
void generate(const std::vector<std::string>& base_options, std::string input, std::string contract_name, const std::vector<std::string>& resource_paths, const std::pair<int, int>& abi_version, bool abigen, bool suppress_ricardian_warning, bool has_o_opt, bool has_contract_opt, bool warn_action_read_only) {
std::vector<std::string> options;
options.push_back("eosio-cpp");
options.push_back(input); // don't remove oddity of CommonOptionsParser?
Expand All @@ -67,13 +67,14 @@ void generate(const std::vector<std::string>& base_options, std::string input, s
abigen::get().set_abi_version(std::get<0>(abi_version), std::get<1>(abi_version));
abigen::get().set_suppress_ricardian_warning(suppress_ricardian_warning);
codegen::get().set_contract_name(contract_name);
codegen::get().set_warn_action_read_only(warn_action_read_only);

int tool_run = -1;
tool_run = ctool.run(newFrontendActionFactory<eosio_abigen_frontend_action>().get());
if (tool_run != 0) {
throw std::runtime_error("abigen error");
}

if (!abigen::get().is_empty()) {
std::string abi_s;
abigen::get().to_json().dump(abi_s);
Expand Down Expand Up @@ -136,7 +137,7 @@ int main(int argc, const char **argv) {
tool_opts.erase(std::remove_if(tool_opts.begin(), tool_opts.end(),
[&](const auto& opt){ return non_tool_opts.count(opt); }),
tool_opts.end());
generate(tool_opts, input, opts.abigen_contract, opts.abigen_resources, opts.abi_version, opts.abigen, opts.suppress_ricardian_warning, opts.has_o_opt, opts.has_contract_opt);
generate(tool_opts, input, opts.abigen_contract, opts.abigen_resources, opts.abi_version, opts.abigen, opts.suppress_ricardian_warning, opts.has_o_opt, opts.has_contract_opt, opts.warn_action_read_only);

auto src = SmallString<64>(input);
llvm::sys::path::remove_filename(src);
Expand Down
19 changes: 16 additions & 3 deletions tools/include/compiler_options.hpp.in
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,10 @@ static cl::opt<std::string> contract_name(
"contract",
cl::desc("Contract name"),
cl::cat(EosioCompilerToolCategory));

static cl::opt<bool> warn_action_read_only_opt(
"warn-action-read-only",
cl::desc("Issue a warning if a read-only action uses a write API and continue compilation"),
cl::cat(EosioCompilerToolCategory));
/// end c/c++ options

/// begin c++ options
Expand Down Expand Up @@ -387,6 +390,7 @@ struct Options {
std::pair<int, int> abi_version;
bool has_o_opt;
bool has_contract_opt;
bool warn_action_read_only;
};

static void GetCompDefaults(std::vector<std::string>& copts) {
Expand Down Expand Up @@ -548,6 +552,7 @@ static Options CreateOptions(bool add_defaults=true) {
std::string abigen_contract;
bool has_o_opt;
bool has_contract_opt;
bool warn_action_read_only;

#ifdef ONLY_LD
bool abigen = false;
Expand Down Expand Up @@ -905,6 +910,14 @@ static Options CreateOptions(bool add_defaults=true) {
ldopts.emplace_back("-fnative");
if (fuse_main_opt)
ldopts.emplace_back("-fuse-main");

/* TODO */
if(warn_action_read_only_opt) {
warn_action_read_only = true;
} else {
warn_action_read_only = false;
}

#endif

/* TODO add some way of defaulting these to the current highest version */
Expand All @@ -918,8 +931,8 @@ static Options CreateOptions(bool add_defaults=true) {
}

#ifndef ONLY_LD
return {output_fn, inputs, link, abigen, no_missing_ricardian_clause_opt, pp_only, pp_dir, abigen_output, abigen_contract, copts, ldopts, agopts, agresources, debug, fnative_opt, {abi_version_major, abi_version_minor}, has_o_opt, has_contract_opt};
return {output_fn, inputs, link, abigen, no_missing_ricardian_clause_opt, pp_only, pp_dir, abigen_output, abigen_contract, copts, ldopts, agopts, agresources, debug, fnative_opt, {abi_version_major, abi_version_minor}, has_o_opt, has_contract_opt, warn_action_read_only};
#else
return {output_fn, {}, link, abigen, no_missing_ricardian_clause_opt, pp_only, pp_dir, abigen_output, abigen_contract, copts, ldopts, agopts, agresources, debug, fnative_opt, {abi_version_major, abi_version_minor}, has_o_opt, has_contract_opt};
return {output_fn, {}, link, abigen, no_missing_ricardian_clause_opt, pp_only, pp_dir, abigen_output, abigen_contract, copts, ldopts, agopts, agresources, debug, fnative_opt, {abi_version_major, abi_version_minor}, has_o_opt, has_contract_opt, warn_action_read_only};
#endif
}
72 changes: 70 additions & 2 deletions tools/include/eosio/codegen.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ namespace eosio { namespace cdt {
llvm::ArrayRef<std::string> sources;
size_t source_index = 0;
std::map<std::string, std::string> tmp_files;
bool warn_action_read_only;

using generation_utils::generation_utils;

Expand All @@ -76,6 +77,10 @@ namespace eosio { namespace cdt {
void set_abi(std::string s) {
abi = s;
}

void set_warn_action_read_only(bool w) {
warn_action_read_only = w;
}
};

class eosio_codegen_visitor : public RecursiveASTVisitor<eosio_codegen_visitor>, public generation_utils {
Expand All @@ -88,14 +93,19 @@ namespace eosio { namespace cdt {
bool apply_was_found = false;

public:
using call_map_t = std::map<FunctionDecl*, std::vector<CallExpr*>>;

std::vector<CXXMethodDecl*> action_decls;
std::vector<CXXMethodDecl*> notify_decls;
std::set<CXXMethodDecl*> read_only_actions;
call_map_t func_calls;

explicit eosio_codegen_visitor(CompilerInstance *CI)
: generation_utils(), ci(CI) {
cg.ast_context = &(CI->getASTContext());
cg.codegen_ci = CI;
rewriter.setSourceMgr(CI->getASTContext().getSourceManager(), CI->getASTContext().getLangOpts());
get_error_emitter().set_compiler_instance(CI);
}

void set_main_fid(FileID fid) {
Expand Down Expand Up @@ -234,9 +244,12 @@ namespace eosio { namespace cdt {
create_action_dispatch(decl);
}
cg.actions.insert(full_action_name); // insert the method action, so we don't create the dispatcher twice

if (decl->isEosioReadOnly()) {
read_only_actions.insert(decl);
}
}
else if (decl->isEosioNotify()) {

name = generation_utils::get_notify_pair(decl);
auto first = name.substr(0, name.find("::"));
if (first != "*")
Expand Down Expand Up @@ -265,6 +278,48 @@ namespace eosio { namespace cdt {
return true;
}

void process_function(FunctionDecl *func_decl) {
if (func_decl->isThisDeclarationADefinition() && func_decl->hasBody()) {
Stmt *stmts = func_decl->getBody();
for (auto it = stmts->child_begin(); it != stmts->child_end(); it++) {
if (Stmt *s = *it) {
if (ExprWithCleanups *ec = dyn_cast<ExprWithCleanups>(s)) {
s = ec->getSubExpr();
while (ImplicitCastExpr *ice = dyn_cast<ImplicitCastExpr>(s))
s = ice->getSubExpr();
}

if (CallExpr *call = dyn_cast<CallExpr>(s)) {
if (FunctionDecl *fd = call->getDirectCallee()) {
if (func_calls.count(fd) == 0) {
process_function(fd);
}
if (!func_calls[fd].empty()) {
func_calls[func_decl].push_back(call);
break;
}
}
}
}
}
}
}

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)) {

return true;
}

std::string fn = func_decl->getQualifiedNameAsString();
if (func_calls.count(func_decl) == 0 && (is_write_host_func(fn) || is_eosio_wasm_import_write_func(func_decl))) {
func_calls[func_decl] = {(CallExpr*)func_decl};
} else {
process_function(func_decl);
}
return true;
}

virtual bool VisitDecl(clang::Decl* decl) {
if (auto* fd = dyn_cast<clang::FunctionDecl>(decl)) {
if (fd->getNameInfo().getAsString() == "apply")
Expand All @@ -274,7 +329,7 @@ namespace eosio { namespace cdt {
}
};

class eosio_codegen_consumer : public ASTConsumer {
class eosio_codegen_consumer : public ASTConsumer, public generation_utils {
private:
eosio_codegen_visitor *visitor;
std::string main_file;
Expand All @@ -295,6 +350,19 @@ namespace eosio { namespace cdt {
visitor->set_main_fid(fid);
visitor->set_main_name(main_fe->getName());
visitor->TraverseDecl(Context.getTranslationUnitDecl());

for (auto const& ra : visitor->read_only_actions) {
auto it = visitor->func_calls.find(ra);
if (it != visitor->func_calls.end()) {
std::string msg = "read-only action cannot call write host function";
if (cg.warn_action_read_only) {
CDT_WARN("codegen_warning", ra->getLocation(), msg);
} else {
CDT_ERROR("codegen_error", ra->getLocation(), msg);
}
}
}

for (auto ad : visitor->action_decls)
visitor->create_action_dispatch(ad);

Expand Down
Loading