From: Yuval Lifshitz Date: Sun, 14 May 2023 16:06:51 +0000 (+0000) Subject: rgw/lua: use an iterator to avoid infinite loops in case of multimap X-Git-Tag: v19.0.0~909^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=c9661d58f1f9a1dcbbbec56998251afb1fb2363a;p=ceph.git rgw/lua: use an iterator to avoid infinite loops in case of multimap also: * unification of the iteration function for different map types * it gived better performance that iteration via lookups * iterator invalidations is handled when deleting an entry during iterations Fixes: https://tracker.ceph.com/issues/59738 Signed-off-by: Yuval Lifshitz --- diff --git a/src/rgw/rgw_lua_background.h b/src/rgw/rgw_lua_background.h index e1271bceb9b..bc967a7bde5 100644 --- a/src/rgw/rgw_lua_background.h +++ b/src/rgw/rgw_lua_background.h @@ -16,23 +16,6 @@ constexpr const int INIT_EXECUTE_INTERVAL = 5; using BackgroundMapValue = std::variant; using BackgroundMap = std::unordered_map; -inline void pushvalue(lua_State* L, const std::string& value) { - pushstring(L, value); -} - -inline void pushvalue(lua_State* L, long long value) { - lua_pushinteger(L, value); -} - -inline void pushvalue(lua_State* L, double value) { - lua_pushnumber(L, value); -} - -inline void pushvalue(lua_State* L, bool value) { - lua_pushboolean(L, value); -} - - struct RGWTable : EmptyMetaTable { static const char* INCREMENT; @@ -102,7 +85,11 @@ struct RGWTable : EmptyMetaTable { switch (value_type) { case LUA_TNIL: - map->erase(std::string(index)); + // erase the element. since in lua: "t[index] = nil" is removing the entry at "t[index]" + if (const auto it = map->find(index); it != map->end()) { + // index was found + update_erased_iterator(L, it, map->erase(it)); + } return NO_RETURNVAL; case LUA_TBOOLEAN: value = static_cast(lua_toboolean(L, 3)); @@ -143,45 +130,18 @@ struct RGWTable : EmptyMetaTable { static int PairsClosure(lua_State* L) { auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); - ceph_assert(map); lua_pushlightuserdata(L, map); - lua_pushcclosure(L, stateless_iter, ONE_UPVAL); // push the stateless iterator function - lua_pushnil(L); // indicate this is the first call - // return stateless_iter, nil - - return TWO_RETURNVALS; - } - - static int stateless_iter(lua_State* L) { - // based on: http://lua-users.org/wiki/GeneralizedPairsAndIpairs - auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); - typename BackgroundMap::const_iterator next_it; - if (lua_isnil(L, -1)) { - next_it = map->begin(); - } else { - const char* index = luaL_checkstring(L, 2); - const auto it = map->find(std::string(index)); - ceph_assert(it != map->end()); - next_it = std::next(it); - } - - if (next_it == map->end()) { - // index of the last element was provided - lua_pushnil(L); - lua_pushnil(L); - // return nil, nil - } else { - pushstring(L, next_it->first); - std::visit([L](auto&& value) { pushvalue(L, value); }, next_it->second); - // return key, value - } + lua_pushcclosure(L, next, ONE_UPVAL); // push the stateless iterator function + lua_pushnil(L); // indicate this is the first call + // return next(), nil return TWO_RETURNVALS; } }; class Background : public RGWRealmReloader::Pauser { - +public: + static const BackgroundMapValue empty_table_value; private: BackgroundMap rgw_map; bool stopped = false; @@ -197,7 +157,6 @@ private: std::mutex cond_mutex; std::mutex pause_mutex; std::condition_variable cond; - static const BackgroundMapValue empty_table_value; void run(); diff --git a/src/rgw/rgw_lua_data_filter.cc b/src/rgw/rgw_lua_data_filter.cc index 9ebaf345388..08ceebb9e66 100644 --- a/src/rgw/rgw_lua_data_filter.cc +++ b/src/rgw/rgw_lua_data_filter.cc @@ -37,7 +37,7 @@ struct BufferlistMetaTable : public EmptyMetaTable { } static int PairsClosure(lua_State* L) { - auto bl = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(1))); + auto bl = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); ceph_assert(bl); lua_pushlightuserdata(L, bl); lua_pushcclosure(L, stateless_iter, ONE_UPVAL); // push the stateless iterator function diff --git a/src/rgw/rgw_lua_request.cc b/src/rgw/rgw_lua_request.cc index 551f5fd7220..7dcd71ed550 100644 --- a/src/rgw/rgw_lua_request.cc +++ b/src/rgw/rgw_lua_request.cc @@ -405,8 +405,10 @@ struct GrantsMetaTable : public EmptyMetaTable { static std::string TableName() {return "Grants";} static std::string Name() {return TableName() + "Meta";} + using Type = ACLGrantMap; + static int IndexClosure(lua_State* L) { - const auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); + const auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); const char* index = luaL_checkstring(L, 2); @@ -420,58 +422,18 @@ struct GrantsMetaTable : public EmptyMetaTable { } static int PairsClosure(lua_State* L) { - auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); + auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); ceph_assert(map); lua_pushlightuserdata(L, map); - lua_pushcclosure(L, stateless_iter, ONE_UPVAL); // push the stateless iterator function - lua_pushnil(L); // indicate this is the first call - // return stateless_iter, nil + lua_pushcclosure(L, next, ONE_UPVAL); // push the "next()" function + lua_pushnil(L); // indicate this is the first call + // return next, nil return TWO_RETURNVALS; } - static int stateless_iter(lua_State* L) { - // based on: http://lua-users.org/wiki/GeneralizedPairsAndIpairs - auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); - ACLGrantMap::iterator next_it; - if (lua_isnil(L, -1)) { - next_it = map->begin(); - } else { - const char* index = luaL_checkstring(L, 2); - const auto it = map->find(std::string(index)); - ceph_assert(it != map->end()); - next_it = std::next(it); - } - - if (next_it == map->end()) { - // index of the last element was provided - lua_pushnil(L); - lua_pushnil(L); - return TWO_RETURNVALS; - // return nil, nil - } - - while (next_it->first.empty()) { - // this is a multimap and the next element does not have a unique key - ++next_it; - if (next_it == map->end()) { - // index of the last element was provided - lua_pushnil(L); - lua_pushnil(L); - return TWO_RETURNVALS; - // return nil, nil - } - } - - pushstring(L, next_it->first); - create_metatable(L, false, &(next_it->second)); - // return key, value - - return TWO_RETURNVALS; - } - static int LenClosure(lua_State* L) { - const auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); + const auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); lua_pushinteger(L, map->size()); diff --git a/src/rgw/rgw_lua_utils.cc b/src/rgw/rgw_lua_utils.cc index accb3b3945d..3cfbf3aea20 100644 --- a/src/rgw/rgw_lua_utils.cc +++ b/src/rgw/rgw_lua_utils.cc @@ -36,7 +36,8 @@ void stack_dump(lua_State* L) { std::cout << std::endl << " ---------------- Stack Dump ----------------" << std::endl; std::cout << "Stack Size: " << top << std::endl; for (int i = 1, j = -top; i <= top; i++, j++) { - std::cout << "[" << i << "," << j << "]: " << luaL_tolstring(L, i, NULL) << std::endl; + std::cout << "[" << i << "," << j << "][" << lua_typename(L, lua_type(L, i)) << "]: " + << luaL_tolstring(L, i, NULL) << std::endl; lua_pop(L, 1); } std::cout << "--------------- Stack Dump Finished ---------------" << std::endl; diff --git a/src/rgw/rgw_lua_utils.h b/src/rgw/rgw_lua_utils.h index ba703f1c57d..4906fff236a 100644 --- a/src/rgw/rgw_lua_utils.h +++ b/src/rgw/rgw_lua_utils.h @@ -1,5 +1,7 @@ #pragma once +#include +#include #include #include #include @@ -11,6 +13,13 @@ #include "include/common_fwd.h" #include "rgw_perf_counters.h" +// a helper type traits structs for detecting std::variant +template +struct is_variant : std::false_type {}; +template +struct is_variant> : + std::true_type {}; + namespace rgw::lua { // push ceph time in string format: "%Y-%m-%d %H:%M:%S" @@ -24,12 +33,28 @@ void pushtime(lua_State* L, const CephTime& tp) lua_pushstring(L, buff); } -static inline void pushstring(lua_State* L, std::string_view str) +inline void pushstring(lua_State* L, std::string_view str) { lua_pushlstring(L, str.data(), str.size()); } -static inline void unsetglobal(lua_State* L, const char* name) +inline void pushvalue(lua_State* L, const std::string& value) { + pushstring(L, value); +} + +inline void pushvalue(lua_State* L, long long value) { + lua_pushinteger(L, value); +} + +inline void pushvalue(lua_State* L, double value) { + lua_pushnumber(L, value); +} + +inline void pushvalue(lua_State* L, bool value) { + lua_pushboolean(L, value); +} + +inline void unsetglobal(lua_State* L, const char* name) { lua_pushnil(L); lua_setglobal(L, name); @@ -212,8 +237,8 @@ struct EmptyMetaTable { void create_debug_action(lua_State* L, CephContext* cct); // set the packages search path according to: -// package.path = "/share/lua/5.3/?.lua" │ LuaRocks. -// package.cpath= "/lib/lua/5.3/?.so" +// package.path = "/share/lua/5.3/?.lua" +// package.cpath = "/lib/lua/5.3/?.so" void set_package_path(lua_State* L, const std::string& install_dir); // open standard lua libs and remove the following functions: @@ -227,9 +252,69 @@ void open_standard_libs(lua_State* L); typedef int MetaTableClosure(lua_State* L); +// copy the input iterator into a new iterator with memory allocated as userdata +// - allow for string conversion of the iterator (into its key) +// - storing the iterator in the metadata table to be used for iterator invalidation handling +template +typename MapType::iterator* create_iterator_metadata(lua_State* L, const typename MapType::iterator& it) { + using Iterator = typename MapType::iterator; + auto iter_buff = lua_newuserdata(L, sizeof(Iterator)); + const auto userdata_pos = lua_gettop(L); + // create metatable for userdata + [[maybe_unused]] const auto rc = luaL_newmetatable(L, typeid(typename MapType::key_type).name()); + const auto metatable_pos = lua_gettop(L); + auto new_it = new (iter_buff) Iterator(it); + // store the iterator pointer in the metatable + lua_pushliteral(L, "__iterator"); + lua_pushlightuserdata(L, new_it); + lua_rawset(L, metatable_pos); + // add "tostring" closure to metatable + lua_pushliteral(L, "__tostring"); + lua_pushlightuserdata(L, new_it); + lua_pushcclosure(L, [](lua_State* L) { + // the key of the table is expected to be convertible to char* + using Iterator = typename MapType::iterator; + static_assert(std::is_constructible()); + auto iter = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); + ceph_assert(iter); + pushstring(L, (*iter)->first); + return ONE_RETURNVAL; + }, ONE_UPVAL); + lua_rawset(L, metatable_pos); + // tie userdata and metatable + lua_setmetatable(L, userdata_pos); + return new_it; +} + +template +void update_erased_iterator(lua_State* L, const typename MapType::iterator& old_it, const typename MapType::iterator& new_it) { + // a metatable exists for the iterator + if (luaL_getmetatable(L, typeid(typename MapType::key_type).name()) != LUA_TNIL) { + const auto metatable_pos = lua_gettop(L); + lua_pushliteral(L, "__iterator"); + if (lua_rawget(L, metatable_pos) != LUA_TNIL) { + // an iterator was stored + auto stored_it = reinterpret_cast(lua_touserdata(L, -1)); + ceph_assert(stored_it); + if (old_it == *stored_it) { + // changed the stored iterator to the iteator + *stored_it = new_it; + } + } + } +} + +// __newindex implementation for any map type holding strings +// or other types constructable from "char*" +// this function allow deletion of an entry by setting "nil" to the entry +// it also limits the size of the entry: key + value cannot exceed MAX_LUA_VALUE_SIZE +// and limits the number of entries in the map, to not exceed MAX_LUA_KEY_ENTRIES template> int StringMapWriteableNewIndex(lua_State* L) { + static_assert(std::is_constructible()); + static_assert(std::is_constructible()); const auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); + ceph_assert(map); const char* index = luaL_checkstring(L, 2); @@ -244,12 +329,66 @@ int StringMapWriteableNewIndex(lua_State* L) { map->insert_or_assign(index, value); } } else { - map->erase(std::string(index)); + // erase the element. since in lua: "t[index] = nil" is removing the entry at "t[index]" + if (const auto it = map->find(index); it != map->end()) { + // index was found + update_erased_iterator(L, it, map->erase(it)); + } } return NO_RETURNVAL; } +// implements the lua next() function for iterating over a table +// first argument is a table and the second argument is an index in this table +// returns the next index of the table and its associated value +// when input index is nil, the function returns the initial value and index +// when the it reaches the last entry of the table it return nil as the index and value +template +int next(lua_State* L) { + using Iterator = typename MapType::iterator; + auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); + ceph_assert(map); + Iterator* next_it = nullptr; + + if (lua_isnil(L, 2)) { + // pop the 2 nils + lua_pop(L, 2); + // create userdata + next_it = create_iterator_metadata(L, map->begin()); + } else { + next_it = reinterpret_cast(lua_touserdata(L, 2)); + ceph_assert(next_it); + *next_it = std::next(*next_it); + } + + if (*next_it == map->end()) { + // index of the last element was provided + lua_pushnil(L); + lua_pushnil(L); + return TWO_RETURNVALS; + // return nil, nil + } + + // key (userdata iterator) is already on the stack + // push the value + using ValueType = typename MapType::mapped_type; + auto& value = (*next_it)->second; + if constexpr(std::is_constructible()) { + // as an std::string + pushstring(L, value); + } else if constexpr(is_variant()) { + // as an std::variant + std::visit([L](auto&& value) { pushvalue(L, value); }, value); + } else { + // as a metatable + create_metatable(L, false, &(value)); + } + // return key, value + + return TWO_RETURNVALS; +} + template, MetaTableClosure NewIndex=EmptyMetaTable::NewIndexClosure> struct StringMapMetaTable : public EmptyMetaTable { @@ -259,6 +398,7 @@ struct StringMapMetaTable : public EmptyMetaTable { static int IndexClosure(lua_State* L) { const auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); + ceph_assert(map); const char* index = luaL_checkstring(L, 2); @@ -266,7 +406,7 @@ struct StringMapMetaTable : public EmptyMetaTable { if (it == map->end()) { lua_pushnil(L); } else { - pushstring(L, it->second); + pushstring(L, it->second); } return ONE_RETURNVAL; } @@ -279,40 +419,13 @@ struct StringMapMetaTable : public EmptyMetaTable { auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); ceph_assert(map); lua_pushlightuserdata(L, map); - lua_pushcclosure(L, stateless_iter, ONE_UPVAL); // push the stateless iterator function - lua_pushnil(L); // indicate this is the first call - // return stateless_iter, nil + lua_pushcclosure(L, next, ONE_UPVAL); // push the "next()" function + lua_pushnil(L); // indicate this is the first call + // return next, nil return TWO_RETURNVALS; } - static int stateless_iter(lua_State* L) { - // based on: http://lua-users.org/wiki/GeneralizedPairsAndIpairs - auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); - typename MapType::const_iterator next_it; - if (lua_isnil(L, -1)) { - next_it = map->begin(); - } else { - const char* index = luaL_checkstring(L, 2); - const auto it = map->find(std::string(index)); - ceph_assert(it != map->end()); - next_it = std::next(it); - } - - if (next_it == map->end()) { - // index of the last element was provided - lua_pushnil(L); - lua_pushnil(L); - // return nil, nil - } else { - pushstring(L, next_it->first); - pushstring(L, next_it->second); - // return key, value - } - - return TWO_RETURNVALS; - } - static int LenClosure(lua_State* L) { const auto map = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); diff --git a/src/test/rgw/test_rgw_lua.cc b/src/test/rgw/test_rgw_lua.cc index a539c025b50..742a306f9a3 100644 --- a/src/test/rgw/test_rgw_lua.cc +++ b/src/test/rgw/test_rgw_lua.cc @@ -485,10 +485,64 @@ TEST(TestRGWLua, Metadata) ASSERT_EQ(rc, 0); } +TEST(TestRGWLua, WriteMetadata) +{ + const std::string script = R"( + -- change existing entry + Request.HTTP.Metadata["hello"] = "earth" + -- add new entry + Request.HTTP.Metadata["goodbye"] = "mars" + -- delete existing entry + Request.HTTP.Metadata["foo"] = nil + -- delete missing entry + Request.HTTP.Metadata["venus"] = nil + + assert(Request.HTTP.Metadata["hello"] == "earth") + assert(Request.HTTP.Metadata["goodbye"] == "mars") + assert(Request.HTTP.Metadata["foo"] == nil) + assert(Request.HTTP.Metadata["venus"] == nil) + )"; + + DEFINE_REQ_STATE; + s.info.x_meta_map["hello"] = "world"; + s.info.x_meta_map["foo"] = "bar"; + s.info.x_meta_map["ka"] = "boom"; + + const auto rc = lua::request::execute(nullptr, nullptr, nullptr, &s, nullptr, script); + ASSERT_EQ(rc, 0); +} + +TEST(TestRGWLua, MetadataIterateWrite) +{ + const std::string script = R"( + counter = 0 + for k,v in pairs(Request.HTTP.Metadata) do + counter = counter + 1 + print(k,v) + if tostring(k) == "c" then + Request.HTTP.Metadata["c"] = nil + end + end + assert(counter == 4) + )"; + + DEFINE_REQ_STATE; + s.info.x_meta_map["a"] = "1"; + s.info.x_meta_map["b"] = "2"; + s.info.x_meta_map["c"] = "3"; + s.info.x_meta_map["d"] = "4"; + s.info.x_meta_map["e"] = "5"; + + const auto rc = lua::request::execute(nullptr, nullptr, nullptr, &s, nullptr, script); + ASSERT_EQ(rc, 0); + ASSERT_EQ(s.info.x_meta_map.count("c"), 0); +} + TEST(TestRGWLua, Acl) { const std::string script = R"( - function print_grant(g) + function print_grant(k, g) + print("Grant Key: " .. tostring(k)) print("Grant Type: " .. g.Type) print("Grant Group Type: " .. g.GroupType) print("Grant Referer: " .. g.Referer) @@ -501,15 +555,16 @@ TEST(TestRGWLua, Acl) assert(Request.UserAcl.Owner.DisplayName == "jack black", Request.UserAcl.Owner.DisplayName) assert(Request.UserAcl.Owner.User.Id == "black", Request.UserAcl.Owner.User.Id) assert(Request.UserAcl.Owner.User.Tenant == "jack", Request.UserAcl.Owner.User.Tenant) - assert(#Request.UserAcl.Grants == 5) - print_grant(Request.UserAcl.Grants[""]) + assert(#Request.UserAcl.Grants == 7) + print_grant("", Request.UserAcl.Grants[""]) for k, v in pairs(Request.UserAcl.Grants) do - print_grant(v) - if k == "john$doe" then + if tostring(k) == "john$doe" then assert(v.Permission == 4) - elseif k == "jane$doe" then + elseif tostring(k) == "jane$doe" then assert(v.Permission == 1) - else + elseif tostring(k) == "kill$bill" then + assert(v.Permission == 6 or v.Permission == 7) + elseif tostring(k) ~= "" then assert(false) end end @@ -521,17 +576,21 @@ TEST(TestRGWLua, Acl) owner.set_name("jack black"); s.user_acl.reset(new RGWAccessControlPolicy(g_cct)); s.user_acl->set_owner(owner); - ACLGrant grant1, grant2, grant3, grant4, grant5; + ACLGrant grant1, grant2, grant3, grant4, grant5, grant6_1, grant6_2; grant1.set_canon(rgw_user("jane", "doe"), "her grant", 1); grant2.set_group(ACL_GROUP_ALL_USERS ,2); grant3.set_referer("http://localhost/ref2", 3); grant4.set_canon(rgw_user("john", "doe"), "his grant", 4); grant5.set_group(ACL_GROUP_AUTHENTICATED_USERS, 5); + grant6_1.set_canon(rgw_user("kill", "bill"), "his grant", 6); + grant6_2.set_canon(rgw_user("kill", "bill"), "her grant", 7); s.user_acl->get_acl().add_grant(&grant1); s.user_acl->get_acl().add_grant(&grant2); s.user_acl->get_acl().add_grant(&grant3); s.user_acl->get_acl().add_grant(&grant4); s.user_acl->get_acl().add_grant(&grant5); + s.user_acl->get_acl().add_grant(&grant6_1); + s.user_acl->get_acl().add_grant(&grant6_2); const auto rc = lua::request::execute(nullptr, nullptr, nullptr, &s, nullptr, script); ASSERT_EQ(rc, 0); } @@ -1089,6 +1148,36 @@ TEST(TestRGWLuaBackground, TableIterate) EXPECT_EQ(get_table_value(lua_background, "size"), 5); } +TEST(TestRGWLuaBackground, TableIterateWrite) +{ + MAKE_STORE; + TestBackground lua_background(store.get(), ""); + + const std::string request_script = R"( + RGW["a"] = 1 + RGW["b"] = 2 + RGW["c"] = 3 + RGW["d"] = 4 + RGW["e"] = 5 + counter = 0 + for k, v in pairs(RGW) do + counter = counter + 1 + print(k, v) + if tostring(k) == "c" then + RGW["c"] = nil + end + end + assert(counter == 4) + )"; + + DEFINE_REQ_STATE; + pe.lua.background = &lua_background; + + const auto rc = lua::request::execute(nullptr, nullptr, nullptr, &s, nullptr, request_script); + ASSERT_EQ(rc, 0); + EXPECT_EQ(lua_background.get_table_value("c"), TestBackground::empty_table_value); +} + TEST(TestRGWLuaBackground, TableIncrement) { MAKE_STORE;