From: Yuval Lifshitz Date: Thu, 15 Jun 2023 10:51:47 +0000 (+0000) Subject: rgw/lua: destroy iterator when table iteration completes X-Git-Tag: v19.0.0~909^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=45d233535ad585f72fb5218e452131d225206afd;p=ceph.git rgw/lua: destroy iterator when table iteration completes also, prevent from dtor being called twice by reusing the iterator and protect against nested loops Signed-off-by: Yuval Lifshitz --- diff --git a/src/rgw/rgw_lua_utils.cc b/src/rgw/rgw_lua_utils.cc index 3cfbf3aea2096..1dd214b7babb3 100644 --- a/src/rgw/rgw_lua_utils.cc +++ b/src/rgw/rgw_lua_utils.cc @@ -31,14 +31,31 @@ void create_debug_action(lua_State* L, CephContext* cct) { lua_setglobal(L, RGWDebugLogAction); } + void stack_dump(lua_State* L) { - int top = lua_gettop(L); + const auto top = lua_gettop(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 << "][" << lua_typename(L, lua_type(L, i)) << "]: " - << luaL_tolstring(L, i, NULL) << std::endl; - lua_pop(L, 1); + std::cout << "[" << i << "," << j << "][" << luaL_typename(L, i) << "]: "; + switch (lua_type(L, i)) { + case LUA_TNUMBER: + std::cout << lua_tonumber(L, i); + break; + case LUA_TSTRING: + std::cout << lua_tostring(L, i); + break; + case LUA_TBOOLEAN: + std::cout << (lua_toboolean(L, i) ? "true" : "false"); + break; + case LUA_TNIL: + std::cout << "nil"; + break; + default: + std::cout << lua_topointer(L, i); + break; + } + std::cout << std::endl; } std::cout << "--------------- Stack Dump Finished ---------------" << std::endl; } diff --git a/src/rgw/rgw_lua_utils.h b/src/rgw/rgw_lua_utils.h index 4906fff236a6c..4bf9a77f0bff6 100644 --- a/src/rgw/rgw_lua_utils.h +++ b/src/rgw/rgw_lua_utils.h @@ -255,25 +255,53 @@ 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 +// - since we have only one iterator per map/table we don't allow for nested loops or another iterationn +// after breaking out of an iteration template -typename MapType::iterator* create_iterator_metadata(lua_State* L, const typename MapType::iterator& it) { +typename MapType::iterator* create_iterator_metadata(lua_State* L, const typename MapType::iterator& start_it, const typename MapType::iterator& end_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()); + // metatable is created before the userdata to save on allocation if the metatable already exists + const auto metatable_is_new = luaL_newmetatable(L, typeid(typename MapType::key_type).name()); const auto metatable_pos = lua_gettop(L); - auto new_it = new (iter_buff) Iterator(it); + int userdata_pos; + Iterator* new_it = nullptr; + if (!metatable_is_new) { + // metatable already exists + lua_pushliteral(L, "__iterator"); + const auto type = lua_rawget(L, metatable_pos); + ceph_assert(type != LUA_TNIL); + auto old_it = reinterpret_cast(lua_touserdata(L, -1)); + // verify we are not mid-iteration + if (*old_it != end_it) { + luaL_error(L, "Trying to iterate '%s' before previous iteration finished", + typeid(typename MapType::key_type).name()); + return nullptr; + } + // we use the same memory buffer + new_it = old_it; + *new_it = start_it; + // push the userdata so it could be tied to the metatable + lua_pushlightuserdata(L, new_it); + userdata_pos = lua_gettop(L); + } else { + // new metatable + auto it_buff = lua_newuserdata(L, sizeof(Iterator)); + userdata_pos = lua_gettop(L); + new_it = new (it_buff) Iterator(start_it); + } + // push the metatable again so it could be tied to the userdata + lua_pushvalue(L, metatable_pos); // store the iterator pointer in the metatable lua_pushliteral(L, "__iterator"); lua_pushlightuserdata(L, new_it); lua_rawset(L, metatable_pos); + // add indication that we are "mid iteration" // 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); @@ -281,6 +309,16 @@ typename MapType::iterator* create_iterator_metadata(lua_State* L, const typenam return ONE_RETURNVAL; }, ONE_UPVAL); lua_rawset(L, metatable_pos); + // define a finalizer of the iterator + lua_pushliteral(L, "__gc"); + lua_pushlightuserdata(L, new_it); + lua_pushcclosure(L, [](lua_State* L) { + auto iter = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(FIRST_UPVAL))); + ceph_assert(iter); + iter->~Iterator(); + return NO_RETURNVAL; + }, ONE_UPVAL); + lua_rawset(L, metatable_pos); // tie userdata and metatable lua_setmetatable(L, userdata_pos); return new_it; @@ -355,7 +393,7 @@ int next(lua_State* L) { // pop the 2 nils lua_pop(L, 2); // create userdata - next_it = create_iterator_metadata(L, map->begin()); + next_it = create_iterator_metadata(L, map->begin(), map->end()); } else { next_it = reinterpret_cast(lua_touserdata(L, 2)); ceph_assert(next_it); @@ -375,8 +413,8 @@ int next(lua_State* L) { using ValueType = typename MapType::mapped_type; auto& value = (*next_it)->second; if constexpr(std::is_constructible()) { - // as an std::string - pushstring(L, value); + // 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); @@ -385,7 +423,6 @@ int next(lua_State* L) { create_metatable(L, false, &(value)); } // return key, value - return TWO_RETURNVALS; } diff --git a/src/test/rgw/test_rgw_lua.cc b/src/test/rgw/test_rgw_lua.cc index 742a306f9a35e..cfe737611a795 100644 --- a/src/test/rgw/test_rgw_lua.cc +++ b/src/test/rgw/test_rgw_lua.cc @@ -521,9 +521,20 @@ TEST(TestRGWLua, MetadataIterateWrite) print(k,v) if tostring(k) == "c" then Request.HTTP.Metadata["c"] = nil + print("'c' is deleted and 'd' is skipped") end end - assert(counter == 4) + assert(counter == 6) + counter = 0 + for k,v in pairs(Request.HTTP.Metadata) do + counter = counter + 1 + print(k,v) + if tostring(k) == "d" then + Request.HTTP.Metadata["e"] = nil + print("'e' is deleted") + end + end + assert(counter == 5) )"; DEFINE_REQ_STATE; @@ -532,12 +543,81 @@ TEST(TestRGWLua, MetadataIterateWrite) s.info.x_meta_map["c"] = "3"; s.info.x_meta_map["d"] = "4"; s.info.x_meta_map["e"] = "5"; + s.info.x_meta_map["f"] = "6"; + s.info.x_meta_map["g"] = "7"; 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, MetadataIterator) +{ + + 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"; + s.info.x_meta_map["f"] = "6"; + s.info.x_meta_map["g"] = "7"; + + std::string script = R"( + print("nested loop") + counter = 0 + for k1,v1 in pairs(Request.HTTP.Metadata) do + print(tostring(k1)..","..v1.." outer loop "..tostring(counter)) + for k2,v2 in pairs(Request.HTTP.Metadata) do + print(k2,v2) + end + counter = counter + 1 + end + )"; + + auto rc = lua::request::execute(nullptr, nullptr, nullptr, &s, nullptr, script); + ASSERT_NE(rc, 0); + + script = R"( + print("break loop") + counter = 0 + for k,v in pairs(Request.HTTP.Metadata) do + counter = counter + 1 + print(k,v) + if counter == 3 then + break + end + counter = counter + 1 + end + print("full loop") + for k,v in pairs(Request.HTTP.Metadata) do + print(k,v) + end + )"; + + rc = lua::request::execute(nullptr, nullptr, nullptr, &s, nullptr, script); + ASSERT_NE(rc, 0); + + script = R"( + print("2 loops") + counter = 0 + for k,v in pairs(Request.HTTP.Metadata) do + print(k,v) + counter = counter + 1 + end + assert(counter == #Request.HTTP.Metadata) + counter = 0 + for k,v in pairs(Request.HTTP.Metadata) do + print(k,v) + counter = counter + 1 + end + assert(counter == #Request.HTTP.Metadata) + )"; + + rc = lua::request::execute(nullptr, nullptr, nullptr, &s, nullptr, script); + ASSERT_EQ(rc, 0); +} + TEST(TestRGWLua, Acl) { const std::string script = R"(