From 0dc25667af46b1e6fca08efb952eae635bb7bd58 Mon Sep 17 00:00:00 2001 From: Eduardo Bart Date: Fri, 20 Sep 2024 18:04:54 -0300 Subject: [PATCH] refactor: let Lua manage allocated references of JSON functions --- src/Makefile | 8 +-- src/clua-cartesi.cpp | 8 ++- src/clua-jsonrpc-machine.cpp | 31 ++++++----- src/clua-machine-util.cpp | 100 ++++++++++++++++++----------------- src/clua-machine-util.h | 38 +++++++++++-- src/clua-machine.cpp | 3 ++ 6 files changed, 118 insertions(+), 70 deletions(-) diff --git a/src/Makefile b/src/Makefile index 8b3e7029d..611086ae1 100644 --- a/src/Makefile +++ b/src/Makefile @@ -188,9 +188,11 @@ else ifeq ($(release),yes) OPTFLAGS+=-O2 DEFS+=-DNDEBUG # disable all asserts else ifeq ($(debug),yes) -OPTFLAGS+=-Og -g +OPTFLAGS+=-Og -g -fno-omit-frame-pointer +OPTFLAGS+=-D_FORTIFY_SOURCE=3 -D_GLIBCXX_ASSERTIONS -fstack-clash-protection -fstack-protector-strong else ifeq ($(sanitize),yes) -OPTFLAGS+=-O1 -g +OPTFLAGS+=-O1 -g -fno-omit-frame-pointer +OPTFLAGS+=-D_FORTIFY_SOURCE=3 -D_GLIBCXX_ASSERTIONS -fstack-clash-protection -fstack-protector-strong endif # Git commit hash (for releases) @@ -222,7 +224,7 @@ endif ifeq ($(sanitize),yes) # Enable address and undefined sanitizers -UBFLAGS+=-fsanitize=address,undefined -fno-sanitize-recover=all -fno-omit-frame-pointer +UBFLAGS+=-fsanitize=address,undefined -fno-sanitize-recover=all LIBASAN_SO=$(shell realpath `$(CC) -print-file-name=libasan.so`) LIBSTDCPP_SO=$(shell realpath `$(CC) -print-file-name=libstdc++.so`) LD_PRELOAD="$(LIBASAN_SO) $(LIBSTDCPP_SO)" diff --git a/src/clua-cartesi.cpp b/src/clua-cartesi.cpp index 07ee73bac..1caee1c72 100644 --- a/src/clua-cartesi.cpp +++ b/src/clua-cartesi.cpp @@ -87,8 +87,10 @@ static int cartesi_mod_keccak(lua_State *L) { static int cartesi_mod_tobase64(lua_State *L) try { size_t size = 0; const char *data = luaL_checklstring(L, 1, &size); - std::string value = cartesi::encode_base64(std::string_view(data, size)); + std::string &value = + *cartesi::clua_push_new_managed_toclose_ptr(L, cartesi::encode_base64(std::string_view(data, size))); lua_pushlstring(L, value.data(), value.size()); + value.clear(); return 1; } catch (std::exception &e) { luaL_error(L, "%s", e.what()); @@ -98,8 +100,10 @@ static int cartesi_mod_tobase64(lua_State *L) try { static int cartesi_mod_frombase64(lua_State *L) try { size_t size = 0; const char *data = luaL_checklstring(L, 1, &size); - std::string value = cartesi::decode_base64(std::string_view(data, size)); + std::string &value = + *cartesi::clua_push_new_managed_toclose_ptr(L, cartesi::decode_base64(std::string_view(data, size))); lua_pushlstring(L, value.data(), value.size()); + value.clear(); return 1; } catch (std::exception &e) { luaL_error(L, "%s", e.what()); diff --git a/src/clua-jsonrpc-machine.cpp b/src/clua-jsonrpc-machine.cpp index 540541f21..4254cbc34 100644 --- a/src/clua-jsonrpc-machine.cpp +++ b/src/clua-jsonrpc-machine.cpp @@ -31,13 +31,14 @@ void clua_delete(cm_jsonrpc_mgr *ptr) { /// \brief This is the machine.get_default_machine_config() /// static method implementation. static int jsonrpc_machine_class_get_default_config(lua_State *L) { - auto &managed_jsonrpc_mgr = - clua_check>(L, lua_upvalueindex(1), lua_upvalueindex(2)); + const int stubidx = lua_upvalueindex(1); + const int ctxidx = lua_upvalueindex(2); + auto &managed_jsonrpc_mgr = clua_check>(L, stubidx, ctxidx); const char *config = nullptr; if (cm_jsonrpc_get_default_config(managed_jsonrpc_mgr.get(), &config) != 0) { return luaL_error(L, "%s", cm_get_last_error_message()); } - clua_push_json_table(L, config); + clua_push_json_table(L, config, ctxidx); return 1; } @@ -61,7 +62,7 @@ static int jsonrpc_machine_class_verify_step_uarch(lua_State *L) { const int ctxidx = lua_upvalueindex(2); lua_settop(L, 5); auto &managed_jsonrpc_mgr = clua_check>(L, stubidx, ctxidx); - const char *access_log = clua_check_json_string(L, 2); + const char *access_log = clua_check_json_string(L, 2, -1, ctxidx); if (!lua_isnil(L, 1) || !lua_isnil(L, 3)) { cm_hash root_hash{}; clua_check_cm_hash(L, 1, &root_hash); @@ -87,7 +88,7 @@ static int jsonrpc_machine_class_verify_reset_uarch(lua_State *L) { const int ctxidx = lua_upvalueindex(2); lua_settop(L, 5); auto &managed_jsonrpc_mgr = clua_check>(L, stubidx, ctxidx); - const char *access_log = clua_check_json_string(L, 2); + const char *access_log = clua_check_json_string(L, 2, -1, ctxidx); if (!lua_isnil(L, 1) || !lua_isnil(L, 3)) { cm_hash root_hash{}; clua_check_cm_hash(L, 1, &root_hash); @@ -117,7 +118,7 @@ static int jsonrpc_machine_class_verify_send_cmio_response(lua_State *L) { size_t length{0}; // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) const auto *data = reinterpret_cast(luaL_checklstring(L, 2, &length)); - const char *access_log = clua_check_json_string(L, 4); + const char *access_log = clua_check_json_string(L, 4, -1, ctxidx); if (!lua_isnil(L, 3) || !lua_isnil(L, 5)) { cm_hash root_hash{}; clua_check_cm_hash(L, 3, &root_hash); @@ -157,16 +158,17 @@ static int jsonrpc_machine_tostring(lua_State *L) { /// \brief This is the cartesi.machine() constructor implementation. /// \param L Lua state. static int jsonrpc_machine_ctor(lua_State *L) { - lua_settop(L, 3); + const int stubidx = lua_upvalueindex(1); const int ctxidx = lua_upvalueindex(2); - auto &managed_jsonrpc_mgr = clua_check>(L, lua_upvalueindex(1), ctxidx); + lua_settop(L, 3); + auto &managed_jsonrpc_mgr = clua_check>(L, stubidx, ctxidx); auto &managed_machine = clua_push_to(L, clua_managed_cm_ptr(nullptr), ctxidx); const char *runtime_config = nullptr; if (!lua_isnil(L, 3)) { - runtime_config = clua_check_json_string(L, 3); + runtime_config = clua_check_json_string(L, 3, -1, ctxidx); } if (lua_type(L, 2) == LUA_TTABLE) { - const char *config = clua_check_json_string(L, 2); + const char *config = clua_check_json_string(L, 2, -1, ctxidx); if (cm_jsonrpc_create_machine(managed_jsonrpc_mgr.get(), config, runtime_config, &managed_machine.get()) != 0) { return luaL_error(L, "%s", cm_get_last_error_message()); } @@ -200,13 +202,14 @@ static int jsonrpc_server_class_get_machine(lua_State *L) { /// \brief This is the machine.get_version() static method implementation. static int jsonrpc_server_class_get_version(lua_State *L) { - auto &managed_jsonrpc_mgr = - clua_check>(L, lua_upvalueindex(1), lua_upvalueindex(2)); + const int stubidx = lua_upvalueindex(1); + const int ctxidx = lua_upvalueindex(2); + auto &managed_jsonrpc_mgr = clua_check>(L, stubidx, ctxidx); const char *version = nullptr; if (cm_jsonrpc_get_version(managed_jsonrpc_mgr.get(), &version) != 0) { return luaL_error(L, "%s", cm_get_last_error_message()); } - clua_push_json_table(L, version); + clua_push_json_table(L, version, ctxidx); return 1; } @@ -295,6 +298,8 @@ static const auto mod = cartesi::clua_make_luaL_Reg_array({ int clua_jsonrpc_machine_init(lua_State *L, int ctxidx) { clua_createnewtype>(L, ctxidx); clua_createnewtype>(L, ctxidx); + clua_createnewtype>(L, ctxidx); + clua_createnewtype>(L, ctxidx); clua_createnewtype>(L, ctxidx); return 1; } diff --git a/src/clua-machine-util.cpp b/src/clua-machine-util.cpp index b255f555d..d423c6cb4 100644 --- a/src/clua-machine-util.cpp +++ b/src/clua-machine-util.cpp @@ -26,24 +26,31 @@ namespace cartesi { -/// \brief Deleter for C string template <> void clua_delete(char *ptr) { // NOLINT(readability-non-const-parameter) delete[] ptr; } -/// \brief Deleter for C data buffer template <> void clua_delete(unsigned char *ptr) { // NOLINT(readability-non-const-parameter) delete[] ptr; } -/// \brief Deleter for C machine template <> -void clua_delete(cm_machine *ptr) { +void clua_delete(cm_machine *ptr) { cm_delete(ptr); } +template <> +void clua_delete(std::string *ptr) { + delete ptr; +} + +template <> +void clua_delete(nlohmann::json *ptr) { + delete ptr; +} + CM_CSR clua_check_cm_proc_csr(lua_State *L, int idx) try { /// \brief Mapping between CSR names and C API constants const static std::unordered_map g_cm_proc_csr_name = { @@ -256,12 +263,9 @@ static int64_t clua_get_array_table_len(lua_State *L, int tabidx) { return len; } -static nlohmann::json clua_check_json_value(lua_State *L, int idx, bool base64encode) { - // ??(edubart): This function has the assumption that we will be always able to - // traverse and read a Lua table without triggering lua_error. - // A lua_error should never happen here in practice, - // therefore we assume this JSON object will always be destructed leaving no leaks. - nlohmann::json j; +static nlohmann::json &clua_push_json_value_ref(lua_State *L, int ctxidx, int idx, bool base64encode = false) { + nlohmann::json &j = *clua_push_new_managed_toclose_ptr(L, nlohmann::json(), ctxidx); + idx -= idx < 0 ? 1 : 0; // adjust offset after pushing j reference switch (lua_type(L, idx)) { case LUA_TTABLE: { const int64_t len = clua_get_array_table_len(L, idx); @@ -269,8 +273,8 @@ static nlohmann::json clua_check_json_value(lua_State *L, int idx, bool base64en j = nlohmann::json::array(); for (int64_t i = 1; i <= len; ++i) { lua_geti(L, idx, i); - j.push_back(clua_check_json_value(L, -1, base64encode)); - lua_pop(L, 1); // pop value + j.push_back(clua_push_json_value_ref(L, ctxidx, -1, base64encode)); + lua_pop(L, 2); // pop value, child j reference } } else { // object j = nlohmann::json::object(); @@ -278,15 +282,16 @@ static nlohmann::json clua_check_json_value(lua_State *L, int idx, bool base64en lua_pushnil(L); // push key while (lua_next(L, -2)) { // update key, push value if (!lua_isstring(L, -2)) { - lua_pop(L, 3); // pop table, key, value - throw std::runtime_error{"table maps cannot contain non string keys"}; + luaL_error(L, "table maps cannot contain keys of type %s", lua_typename(L, lua_type(L, -2))); } - const std::string_view key = lua_tostring(L, -2); + size_t len = 0; + const char *ptr = lua_tolstring(L, -2, &len); + const std::string_view key(ptr, len); const bool base64encode_child = base64encode || key == "read_hash" || key == "read" || key == "sibling_hashes" || key == "written_hash" || key == "written" || key == "target_hash" || key == "root_hash"; - j[key] = clua_check_json_value(L, -1, base64encode_child); - lua_pop(L, 1); // pop value + j[key] = clua_push_json_value_ref(L, ctxidx, -1, base64encode_child); + lua_pop(L, 2); // pop value, child j reference } lua_pop(L, 1); // pop table } @@ -295,7 +300,7 @@ static nlohmann::json clua_check_json_value(lua_State *L, int idx, bool base64en case LUA_TNUMBER: { if (lua_isinteger(L, idx)) { j = lua_tointeger(L, idx); - } else { + } else { // floating point j = lua_tonumber(L, idx); } break; @@ -305,7 +310,7 @@ static nlohmann::json clua_check_json_value(lua_State *L, int idx, bool base64en const char *ptr = lua_tolstring(L, idx, &len); const std::string_view data(ptr, len); if (base64encode) { - j = cartesi::encode_base64(data); + j = encode_base64(data); } else { j = data; } @@ -318,37 +323,34 @@ static nlohmann::json clua_check_json_value(lua_State *L, int idx, bool base64en j = nullptr; break; default: - throw std::runtime_error{ - std::string("lua value of type ") + lua_typename(L, lua_type(L, idx)) + "cannot be serialized to JSON", - }; + luaL_error(L, "lua value of type %s cannot be serialized to JSON", lua_typename(L, lua_type(L, idx))); + break; } return j; } -const char *clua_check_json_string(lua_State *L, int idx, int indent) { - if (lua_istable(L, idx)) { - try { - // Use static thread local to avoid destruction leaks in case of a lua_error - static THREAD_LOCAL std::string s; - s = cartesi::clua_check_json_value(L, idx, false).dump(indent); - lua_pushlstring(L, s.data(), s.size()); - s.clear(); - lua_replace(L, idx); - } catch (std::exception &e) { - luaL_error(L, "failed to parse JSON from a table: %s", e.what()); - return nullptr; - } +const char *clua_check_json_string(lua_State *L, int idx, int indent, int ctxidx) { + assert(idx > 0); + try { + const nlohmann::json &j = clua_push_json_value_ref(L, ctxidx, idx); + std::string &s = *clua_push_new_managed_toclose_ptr(L, j.dump(indent), ctxidx); + lua_pushlstring(L, s.data(), s.size()); + lua_replace(L, idx); // replace the Lua value with its JSON string representation + lua_pop(L, 2); // pop s, j references + return luaL_checkstring(L, idx); // return the string + } catch (std::exception &e) { + luaL_error(L, "failed to parse JSON from a table: %s", e.what()); + return nullptr; } - return luaL_checkstring(L, idx); } -static void clua_push_json_value(lua_State *L, const nlohmann::json &j, bool base64decode) { +static void clua_push_json_value(lua_State *L, int ctxidx, const nlohmann::json &j, bool base64decode = false) { switch (j.type()) { case nlohmann::json::value_t::array: { lua_createtable(L, static_cast(j.size()), 0); int64_t i = 1; for (auto it = j.begin(); it != j.end(); ++it, ++i) { - clua_push_json_value(L, *it, base64decode); + clua_push_json_value(L, ctxidx, *it, base64decode); lua_rawseti(L, -2, i); } break; @@ -360,7 +362,7 @@ static void clua_push_json_value(lua_State *L, const nlohmann::json &j, bool bas const bool base64decode_child = base64decode || key == "read_hash" || key == "read" || key == "sibling_hashes" || key == "written_hash" || key == "written" || key == "target_hash" || key == "root_hash"; - clua_push_json_value(L, el.value(), base64decode_child); + clua_push_json_value(L, ctxidx, el.value(), base64decode_child); lua_setfield(L, -2, key.c_str()); } break; @@ -368,11 +370,11 @@ static void clua_push_json_value(lua_State *L, const nlohmann::json &j, bool bas case nlohmann::json::value_t::string: { const std::string_view &data = j.template get(); if (base64decode) { - // Use static thread local to avoid destruction leaks in case of a luaL_error - static THREAD_LOCAL std::string binary_data; - binary_data = cartesi::decode_base64(data); + lua_pushnil(L); // reserve a slot in the stack (needed because of lua_toclose semantics) + std::string &binary_data = *clua_push_new_managed_toclose_ptr(L, decode_base64(data), ctxidx); lua_pushlstring(L, binary_data.data(), binary_data.length()); - binary_data.clear(); + lua_replace(L, -3); // move into the placeholder slot + lua_pop(L, 1); // pop binary_data reference } else { lua_pushlstring(L, data.data(), data.length()); } @@ -399,13 +401,13 @@ static void clua_push_json_value(lua_State *L, const nlohmann::json &j, bool bas } } -void clua_push_json_table(lua_State *L, const char *s) { +void clua_push_json_table(lua_State *L, const char *s, int ctxidx) { try { - // Use static thread local to avoid destruction leaks in case of a lua_error - static THREAD_LOCAL nlohmann::json j; - j = nlohmann::json::parse(s); - clua_push_json_value(L, j, false); - j.clear(); + lua_pushnil(L); // reserve a slot in the stack (needed because of lua_toclose semantics) + const nlohmann::json &j = *clua_push_new_managed_toclose_ptr(L, nlohmann::json::parse(s), ctxidx); + clua_push_json_value(L, ctxidx, j); + lua_replace(L, -3); // move into the placeholder slot + lua_pop(L, 1); // pop j reference } catch (std::exception &e) { luaL_error(L, "failed to parse JSON from a string: %s", e.what()); } diff --git a/src/clua-machine-util.h b/src/clua-machine-util.h index 5abf54ca3..1ca0ac5e2 100644 --- a/src/clua-machine-util.h +++ b/src/clua-machine-util.h @@ -24,6 +24,7 @@ extern "C" { #include } +#include "clua.h" #include "json-util.h" #include "machine-c-api.h" @@ -44,10 +45,18 @@ void clua_delete(char *ptr); template <> void clua_delete(unsigned char *ptr); -/// \brief Deleter for C api machine +/// \brief Deleter for machine template <> void clua_delete(cm_machine *ptr); +/// \brief Deleter for string +template <> +void clua_delete(std::string *ptr); + +/// \brief Deleter for JSON +template <> +void clua_delete(nlohmann::json *ptr); + // clua_managed_cm_ptr is a smart pointer, // however we don't use all its functionally, therefore we exclude it from code coverage. // LCOV_EXCL_START @@ -107,6 +116,27 @@ class clua_managed_cm_ptr final { }; // LCOV_EXCL_STOP +/// \brief Allocates a new type, pushes its reference into the Lua stack and returns its pointer. +/// \param L Lua state +/// \param value Initial value +/// \param ctxidx Index (or pseudo-index) of clua context +/// \returns The value pointer, valid until its reference is removed from the Lua stack. +/// \details The value is marked to-be-closed when popped from the Lua stack. +/// This follow lua_toclose semantics (check Lua 5.4 manual), +/// therefore the stack index can only be removed via lua_pop (e.g. don't use lua_remove). +template +T *clua_push_new_managed_toclose_ptr(lua_State *L, T &&value, int ctxidx = lua_upvalueindex(1)) { + auto &managed_value = clua_push_to(L, clua_managed_cm_ptr(new T(std::forward(value))), ctxidx); + // ??(edubart): Unfortunately Lua 5.4.4 (default on Ubuntu 22.04) has a bug that causes a crash + // when using lua_settop with lua_toclose, it was fixed only in Lua 5.4.5 in + // https://github.com/lua/lua/commit/196bb94d66e727e0aec053a0276c3ad701500762 . + // Without lua_toclose call, reference will be only collected by the GC (non deterministic). +#if LUA_VERSION_RELEASE_NUM > 50404 + lua_toclose(L, -1); +#endif + return managed_value.get(); +} + /// \brief Returns a CSR selector from Lua /// \param L Lua state /// \param idx Index in stack @@ -128,15 +158,17 @@ void clua_check_cm_hash(lua_State *L, int idx, cm_hash *c_hash); /// \param L Lua state /// \param tabidx Lua table stack index which will be converted to a Lua string. /// \param indent JSON indentation when converting it to a string. +/// \param ctxidx Index (or pseudo-index) of clua context /// \returns It traverses the Lua value while converting to a JSON object. /// \details In case the Lua valua is already a string, it just returns it. -const char *clua_check_json_string(lua_State *L, int idx, int indent = -1); +const char *clua_check_json_string(lua_State *L, int idx, int indent = -1, int ctxidx = lua_upvalueindex(1)); /// \brief Parses a JSON from a string and pushes it as a Lua table. /// \param L Lua state /// \param s JSON string. +/// \param ctxidx Index (or pseudo-index) of clua context /// \returns It traverses the JSON object while converting to a Lua object. -void clua_push_json_table(lua_State *L, const char *s); +void clua_push_json_table(lua_State *L, const char *s, int ctxidx = lua_upvalueindex(1)); } // namespace cartesi diff --git a/src/clua-machine.cpp b/src/clua-machine.cpp index 713e590fe..a37fb4a51 100644 --- a/src/clua-machine.cpp +++ b/src/clua-machine.cpp @@ -17,6 +17,7 @@ #include "clua-htif.h" #include "clua-machine-util.h" #include "clua.h" +#include "json-util.h" #include "machine-c-api.h" #include "riscv-constants.h" @@ -143,6 +144,8 @@ struct machine_class {}; int clua_machine_init(lua_State *L, int ctxidx) { clua_createnewtype>(L, ctxidx); clua_createnewtype>(L, ctxidx); + clua_createnewtype>(L, ctxidx); + clua_createnewtype>(L, ctxidx); if (!clua_typeexists(L, ctxidx)) { clua_createtype(L, "cartesi machine class", ctxidx); clua_setmethods(L, machine_class_index.data(), 0, ctxidx);