From b3fb23ee0c6d33f5eba3502328ffb0011b5f25fb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 15 Aug 2024 09:49:34 -0400 Subject: Introduce lluau_checkstack(L, n); use instead of luaL_checkstack(). luaL_checkstack() accepts a third parameter which is included in the stack overflow error message. We've been passing nullptr, leading to messages of the form "stack overflow ((null))". lluau_checkstack() implicitly passes __FUNCTION__, so we can distinguish which underlying luaL_checkstack() call encountered the stack overflow condition. Also, when calling each atexit() function, pass Luau's debug.traceback() function as the lua_pcall() error handler. This should help diagnose errors in atexit() functions. --- indra/llcommon/lua_function.cpp | 57 ++++++++++++++++++++++++++--------------- indra/llcommon/lua_function.h | 13 ++++++---- indra/newview/llluamanager.cpp | 6 ++--- 3 files changed, 47 insertions(+), 29 deletions(-) diff --git a/indra/llcommon/lua_function.cpp b/indra/llcommon/lua_function.cpp index b173d17ede..defadea358 100644 --- a/indra/llcommon/lua_function.cpp +++ b/indra/llcommon/lua_function.cpp @@ -127,7 +127,7 @@ std::string lua_tostdstring(lua_State* L, int index) void lua_pushstdstring(lua_State* L, const std::string& str) { - luaL_checkstack(L, 1, nullptr); + lluau_checkstack(L, 1); lua_pushlstring(L, str.c_str(), str.length()); } @@ -240,10 +240,10 @@ LLSD lua_tollsd(lua_State* L, int index) // undefined LLSD. Naturally, though, those won't survive a second // round trip. - // This is the most important of the luaL_checkstack() calls because a + // This is the most important of the lluau_checkstack() calls because a // deeply nested Lua structure will enter this case at each level, and // we'll need another 2 stack slots to traverse each nested table. - luaL_checkstack(L, 2, nullptr); + lluau_checkstack(L, 2); // BEFORE we push nil to initialize the lua_next() traversal, convert // 'index' to absolute! Our caller might have passed a relative index; // we do, below: lua_tollsd(L, -1). If 'index' is -1, then when we @@ -400,7 +400,7 @@ LLSD lua_tollsd(lua_State* L, int index) void lua_pushllsd(lua_State* L, const LLSD& data) { // might need 2 slots for array or map - luaL_checkstack(L, 2, nullptr); + lluau_checkstack(L, 2); switch (data.type()) { case LLSD::TypeUndefined: @@ -487,39 +487,54 @@ LuaState::LuaState(script_finished_fn cb): LuaState::~LuaState() { - // We're just about to destroy this lua_State mState. lua_close() doesn't - // implicitly garbage-collect everything, so (for instance) any lingering - // objects with __gc metadata methods aren't cleaned up. This is why we - // provide atexit(). - luaL_checkstack(mState, 2, nullptr); + // We're just about to destroy this lua_State mState. Did this Lua chunk + // register any atexit() functions? + lluau_checkstack(mState, 3); // look up Registry.atexit lua_getfield(mState, LUA_REGISTRYINDEX, "atexit"); // stack contains Registry.atexit if (lua_istable(mState, -1)) { + // Push debug.traceback() onto the stack as lua_pcall()'s error + // handler function. On error, lua_pcall() calls the specified error + // handler function with the original error message; the message + // returned by the error handler is then returned by lua_pcall(). + // Luau's debug.traceback() is called with a message to prepend to the + // returned traceback string. Almost as if they'd been designed to + // work together... + lua_getglobal(mState, "debug"); + lua_getfield(mState, -1, "traceback"); + // ditch "debug" + lua_remove(mState, -2); + // stack now contains atexit, debug.traceback() + // We happen to know that Registry.atexit is built by appending array // entries using table.insert(). That's important because it means // there are no holes, and therefore lua_objlen() should be correct. // That's important because we walk the atexit table backwards, to // destroy last the things we created (passed to LL.atexit()) first. - for (int i(lua_objlen(mState, -1)); i >= 1; --i) + for (int i(lua_objlen(mState, -2)); i >= 1; --i) { lua_pushinteger(mState, i); - // stack contains Registry.atexit, i - lua_gettable(mState, -2); - // stack contains Registry.atexit, atexit[i] + // stack contains Registry.atexit, debug.traceback(), i + lua_gettable(mState, -3); + // stack contains Registry.atexit, debug.traceback(), atexit[i] // Call atexit[i](), no args, no return values. // Use lua_pcall() because errors in any one atexit() function - // shouldn't cancel the rest of them. - if (lua_pcall(mState, 0, 0, 0) != LUA_OK) + // shouldn't cancel the rest of them. Pass debug.traceback() as + // the error handler function. + if (lua_pcall(mState, 0, 0, -2) != LUA_OK) { auto error{ lua_tostdstring(mState, -1) }; LL_WARNS("Lua") << "atexit() function error: " << error << LL_ENDL; // pop error message lua_pop(mState, 1); } - // lua_pcall() has already popped atexit[i]: stack contains atexit + // lua_pcall() has already popped atexit[i]: + // stack contains atexit, debug.traceback() } + // pop debug.traceback() + lua_pop(mState, 1); } // pop Registry.atexit (either table or nil) lua_pop(mState, 1); @@ -625,7 +640,7 @@ std::pair LuaState::expr(const std::string& desc, const std::string& LuaListener& LuaState::obtainListener(lua_State* L) { - luaL_checkstack(L, 2, nullptr); + lluau_checkstack(L, 2); lua_getfield(L, LUA_REGISTRYINDEX, "LuaListener"); // compare lua_type() because lua_isuserdata() also accepts light userdata if (lua_type(L, -1) != LUA_TUSERDATA) @@ -655,7 +670,7 @@ LuaListener& LuaState::obtainListener(lua_State* L) lua_function(atexit, "atexit(function): " "register Lua function to be called at script termination") { - luaL_checkstack(L, 4, nullptr); + lluau_checkstack(L, 4); // look up the global name "table" lua_getglobal(L, "table"); // stack contains function, table @@ -705,7 +720,7 @@ LuaFunction::LuaFunction(const std::string_view& name, lua_CFunction function, void LuaFunction::init(lua_State* L) { const auto& [registry, lookup] = getRState(); - luaL_checkstack(L, 2, nullptr); + lluau_checkstack(L, 2); // create LL table -- // it happens that we know exactly how many non-array members we want lua_createtable(L, 0, int(narrow(lookup.size()))); @@ -743,7 +758,7 @@ std::pair LuaFunction::getState() *****************************************************************************/ lua_function(source_path, "source_path(): return the source path of the running Lua script") { - luaL_checkstack(L, 1, nullptr); + lluau_checkstack(L, 1); lua_pushstdstring(L, lluau::source_path(L).u8string()); return 1; } @@ -753,7 +768,7 @@ lua_function(source_path, "source_path(): return the source path of the running *****************************************************************************/ lua_function(source_dir, "source_dir(): return the source directory of the running Lua script") { - luaL_checkstack(L, 1, nullptr); + lluau_checkstack(L, 1); lua_pushstdstring(L, lluau::source_path(L).parent_path().u8string()); return 1; } diff --git a/indra/llcommon/lua_function.h b/indra/llcommon/lua_function.h index 7b59af30f5..c1a4d736a0 100644 --- a/indra/llcommon/lua_function.h +++ b/indra/llcommon/lua_function.h @@ -65,6 +65,9 @@ namespace lluau void check_interrupts_counter(lua_State* L); } // namespace lluau +// must be a macro because __FUNCTION__ is context-sensitive +#define lluau_checkstack(L, n) luaL_checkstack((L), (n), __FUNCTION__) + std::string lua_tostdstring(lua_State* L, int index); void lua_pushstdstring(lua_State* L, const std::string& str); LLSD lua_tollsd(lua_State* L, int index); @@ -294,7 +297,7 @@ auto lua_to(lua_State* L, int index) template auto lua_getfieldv(lua_State* L, int index, const char* k) { - luaL_checkstack(L, 1, nullptr); + lluau_checkstack(L, 1); lua_getfield(L, index, k); LuaPopper pop(L, 1); return lua_to(L, -1); @@ -304,7 +307,7 @@ auto lua_getfieldv(lua_State* L, int index, const char* k) template auto lua_setfieldv(lua_State* L, int index, const char* k, const T& value) { - luaL_checkstack(L, 1, nullptr); + lluau_checkstack(L, 1); lua_push(L, value); lua_setfield(L, index, k); } @@ -313,7 +316,7 @@ auto lua_setfieldv(lua_State* L, int index, const char* k, const T& value) template auto lua_rawgetfield(lua_State* L, int index, const std::string_view& k) { - luaL_checkstack(L, 1, nullptr); + lluau_checkstack(L, 1); lua_pushlstring(L, k.data(), k.length()); lua_rawget(L, index); LuaPopper pop(L, 1); @@ -324,7 +327,7 @@ auto lua_rawgetfield(lua_State* L, int index, const std::string_view& k) template void lua_rawsetfield(lua_State* L, int index, const std::string_view& k, const T& value) { - luaL_checkstack(L, 2, nullptr); + lluau_checkstack(L, 2); lua_pushlstring(L, k.data(), k.length()); lua_push(L, value); lua_rawset(L, index); @@ -430,7 +433,7 @@ DistinctInt TypeTag::value; template void lua_emplace(lua_State* L, ARGS&&... args) { - luaL_checkstack(L, 1, nullptr); + lluau_checkstack(L, 1); int tag{ TypeTag::value }; if (! lua_getuserdatadtor(L, tag)) { diff --git a/indra/newview/llluamanager.cpp b/indra/newview/llluamanager.cpp index 7014c59e4e..0b1a73f4e9 100644 --- a/indra/newview/llluamanager.cpp +++ b/indra/newview/llluamanager.cpp @@ -66,7 +66,7 @@ std::string lua_print_msg(lua_State* L, const std::string_view& level) { // On top of existing Lua arguments, we're going to push tostring() and // duplicate each existing stack entry so we can stringize each one. - luaL_checkstack(L, 2, nullptr); + lluau_checkstack(L, 2); luaL_where(L, 1); // start with the 'where' info at the top of the stack std::ostringstream out; @@ -139,7 +139,7 @@ lua_function(get_event_pumps, "Events posted to replypump are queued for get_event_next().\n" "post_on(commandpump, ...) to engage LLEventAPI operations (see helpleap()).") { - luaL_checkstack(L, 2, nullptr); + lluau_checkstack(L, 2); auto& listener{ LuaState::obtainListener(L) }; // return the reply pump name and the command pump name on caller's lua_State lua_pushstdstring(L, listener.getReplyName()); @@ -153,7 +153,7 @@ lua_function(get_event_next, "is returned by get_event_pumps(). Blocks the calling chunk until an\n" "event becomes available.") { - luaL_checkstack(L, 2, nullptr); + lluau_checkstack(L, 2); auto& listener{ LuaState::obtainListener(L) }; const auto& [pump, data]{ listener.getNext() }; lua_pushstdstring(L, pump); -- cgit v1.2.3 From a5337adf79b6a49166a15836ca2822adbb50d8c3 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Aug 2024 19:38:10 -0400 Subject: Add LL::scope_exit --- indra/llcommon/scope_exit.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 indra/llcommon/scope_exit.h diff --git a/indra/llcommon/scope_exit.h b/indra/llcommon/scope_exit.h new file mode 100644 index 0000000000..00fab069c4 --- /dev/null +++ b/indra/llcommon/scope_exit.h @@ -0,0 +1,34 @@ +/** + * @file scope_exit.h + * @author Nat Goodspeed + * @date 2024-08-15 + * @brief Cheap imitation of std::experimental::scope_exit + * + * $LicenseInfo:firstyear=2024&license=viewerlgpl$ + * Copyright (c) 2024, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_SCOPE_EXIT_H) +#define LL_SCOPE_EXIT_H + +#include + +namespace LL +{ + +class scope_exit +{ +public: + scope_exit(const std::function& func): mFunc(func) {} + scope_exit(const scope_exit&) = delete; + scope_exit& operator=(const scope_exit&) = delete; + ~scope_exit() { mFunc(); } + +private: + std::function mFunc; +}; + +} // namespace LL + +#endif /* ! defined(LL_SCOPE_EXIT_H) */ -- cgit v1.2.3 From 68313aa2defcc7d6e5ebbd96344d78f3a31fdb9a Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Aug 2024 20:55:48 -0400 Subject: Fix TempSet to use type VAR to store mOldValue. In fact we set mOldValue from mVar, and restore mVar from mOldValue, so the VAR type makes the most sense. The previous way, you'd get actual errors if you tried to use TempSet(pointervar, nullptr): that declared mOldValue to be nullptr_t, which you can't initialize from mVar. --- indra/llcommon/tempset.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/indra/llcommon/tempset.h b/indra/llcommon/tempset.h index e1496bd5fc..07e607a576 100755 --- a/indra/llcommon/tempset.h +++ b/indra/llcommon/tempset.h @@ -35,7 +35,7 @@ public: private: VAR& mVar; - VALUE mOldValue; + VAR mOldValue; }; #endif /* ! defined(LL_TEMPSET_H) */ -- cgit v1.2.3 From 376c890c095fbc59b83402255cc1036c411150b9 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Aug 2024 21:12:25 -0400 Subject: Fix for #2237: intermittent Lua data stack overflow. Use a static unordered_map to allow a function receiving (lua_State* L) to look up the LuaState instance managing that lua_State. We've thought about this from time to time already. LuaState's constructor creates the map entry; its destructor removes it; the new static getParent(lua_State* L) method performs the lookup. Migrate lluau::set_interrupts_counter() and check_interrupts_counter() into LuaState member functions. Add a new mInterrupts counter for them. Importantly, LuaState::check_interrupts_counter(), which is indirectly called by a lua_callbacks().interrupt function, no longer performs any Lua stack operations. Empirically, it seems the Lua engine is capable of interrupting itself at a moment when re-entry confuses it. Change previous lluau::set_interrupts_counter(L, 0) calls to LuaState::getParent(L).set_interrupts_counter(0). Also add LuaStackDelta class, and a lua_checkdelta() helper macro, to verify that the Lua data stack depth on exit from a block differs from the depth on entry by exactly the expected amount. Sprinkle lua_checkdelta() macros in likely places. --- indra/llcommon/lua_function.cpp | 126 +++++++++++++++++++++++++++++++--------- indra/llcommon/lua_function.h | 47 +++++++++++++-- indra/newview/llluamanager.cpp | 5 ++ 3 files changed, 146 insertions(+), 32 deletions(-) diff --git a/indra/llcommon/lua_function.cpp b/indra/llcommon/lua_function.cpp index defadea358..ad77a1e040 100644 --- a/indra/llcommon/lua_function.cpp +++ b/indra/llcommon/lua_function.cpp @@ -21,6 +21,7 @@ #include #include // std::unique_ptr #include +#include // external library headers // other Linden headers #include "fsyspath.h" @@ -54,7 +55,10 @@ namespace }; } // anonymous namespace -int lluau::dostring(lua_State* L, const std::string& desc, const std::string& text) +namespace lluau +{ + +int dostring(lua_State* L, const std::string& desc, const std::string& text) { auto r = loadstring(L, desc, text); if (r != LUA_OK) @@ -66,7 +70,7 @@ int lluau::dostring(lua_State* L, const std::string& desc, const std::string& te return lua_pcall(L, 0, LUA_MULTRET, 0); } -int lluau::loadstring(lua_State *L, const std::string &desc, const std::string &text) +int loadstring(lua_State *L, const std::string &desc, const std::string &text) { size_t bytecodeSize = 0; // The char* returned by luau_compile() must be freed by calling free(). @@ -76,7 +80,7 @@ int lluau::loadstring(lua_State *L, const std::string &desc, const std::string & return luau_load(L, desc.data(), bytecode.get(), bytecodeSize, 0); } -fsyspath lluau::source_path(lua_State* L) +fsyspath source_path(lua_State* L) { //Luau lua_Debug and lua_getinfo() are different compared to default Lua: //see https://github.com/luau-lang/luau/blob/80928acb92d1e4b6db16bada6d21b1fb6fa66265/VM/include/lua.h @@ -93,33 +97,14 @@ fsyspath lluau::source_path(lua_State* L) return ar.source; } -void lluau::set_interrupts_counter(lua_State *L, S32 counter) -{ - lua_rawsetfield(L, LUA_REGISTRYINDEX, "_INTERRUPTS"sv, lua_Integer(counter)); -} - -void lluau::check_interrupts_counter(lua_State* L) -{ - auto counter = lua_rawgetfield(L, LUA_REGISTRYINDEX, "_INTERRUPTS"sv); - - set_interrupts_counter(L, ++counter); - if (counter > INTERRUPTS_MAX_LIMIT) - { - lluau::error(L, "Possible infinite loop, terminated."); - } - else if (counter % INTERRUPTS_SUSPEND_LIMIT == 0) - { - LL_DEBUGS("Lua") << LLCoros::getName() << " suspending at " << counter << " interrupts" - << LL_ENDL; - llcoro::suspend(); - } -} +} // namespace lluau /***************************************************************************** * Lua <=> C++ conversions *****************************************************************************/ std::string lua_tostdstring(lua_State* L, int index) { + lua_checkdelta(L); size_t len; const char* strval{ lua_tolstring(L, index, &len) }; return { strval, len }; @@ -127,6 +112,7 @@ std::string lua_tostdstring(lua_State* L, int index) void lua_pushstdstring(lua_State* L, const std::string& str) { + lua_checkdelta(L, 1); lluau_checkstack(L, 1); lua_pushlstring(L, str.c_str(), str.length()); } @@ -148,6 +134,7 @@ void lua_pushstdstring(lua_State* L, const std::string& str) // reached by that block raises a Lua error. LLSD lua_tollsd(lua_State* L, int index) { + lua_checkdelta(L); switch (lua_type(L, index)) { case LUA_TNONE: @@ -399,6 +386,7 @@ LLSD lua_tollsd(lua_State* L, int index) // stack a Lua object corresponding to the passed LLSD object. void lua_pushllsd(lua_State* L, const LLSD& data) { + lua_checkdelta(L, 1); // might need 2 slots for array or map lluau_checkstack(L, 2); switch (data.type()) @@ -471,10 +459,23 @@ void lua_pushllsd(lua_State* L, const LLSD& data) /***************************************************************************** * LuaState class *****************************************************************************/ +namespace +{ + +// If we find we're running Lua scripts from more than one thread, sLuaStateMap +// should be thread_local. Until then, avoid the overhead. +using LuaStateMap = std::unordered_map; +static LuaStateMap sLuaStateMap; + +} // anonymous namespace + LuaState::LuaState(script_finished_fn cb): mCallback(cb), mState(luaL_newstate()) { + // Ensure that we can always find this LuaState instance, given the + // lua_State we just created or any of its coroutines. + sLuaStateMap.emplace(mState, this); luaL_openlibs(mState); // publish to this new lua_State all the LL entry points we defined using // the lua_function() macro @@ -545,7 +546,9 @@ LuaState::~LuaState() { // mError potentially set by previous checkLua() call(s) mCallback(mError); - } + } + // with the demise of this LuaState, remove sLuaStateMap entry + sLuaStateMap.erase(mState); } bool LuaState::checkLua(const std::string& desc, int r) @@ -563,7 +566,7 @@ bool LuaState::checkLua(const std::string& desc, int r) std::pair LuaState::expr(const std::string& desc, const std::string& text) { - lluau::set_interrupts_counter(mState, 0); + set_interrupts_counter(0); lua_callbacks(mState)->interrupt = [](lua_State *L, int gc) { @@ -572,7 +575,7 @@ std::pair LuaState::expr(const std::string& desc, const std::string& return; LLCoros::checkStop(); - lluau::check_interrupts_counter(L); + LuaState::getParent(L).check_interrupts_counter(); }; LL_INFOS("Lua") << desc << " run" << LL_ENDL; @@ -664,12 +667,53 @@ LuaListener& LuaState::obtainListener(lua_State* L) return *listener; } +LuaState& LuaState::getParent(lua_State* L) +{ + // Look up the LuaState instance associated with the *script*, not the + // specific Lua *coroutine*. In other words, first find this lua_State's + // main thread. + auto found{ sLuaStateMap.find(lua_mainthread(L)) }; + // Our constructor creates the map entry, our destructor deletes it. As + // long as the LuaState exists, we should be able to find it. And we + // SHOULD only be talking to a lua_State managed by a LuaState instance. + llassert(found != sLuaStateMap.end()); + return *found->second; +} + +void LuaState::set_interrupts_counter(S32 counter) +{ + mInterrupts = counter; +} + +void LuaState::check_interrupts_counter() +{ + // The official way to manage data associated with a lua_State is to store + // it *as* Lua data within the lua_State. But this method is called by the + // Lua engine via lua_callbacks(L)->interrupt, and empirically we've hit + // mysterious Lua data stack overflows trying to use stack-based Lua data + // access functions in that situation. It seems the Lua engine is capable + // of interrupting itself at a moment when re-entry is not valid. So only + // touch data in this LuaState. + ++mInterrupts; + if (mInterrupts > INTERRUPTS_MAX_LIMIT) + { + lluau::error(mState, "Possible infinite loop, terminated."); + } + else if (mInterrupts % INTERRUPTS_SUSPEND_LIMIT == 0) + { + LL_DEBUGS("Lua") << LLCoros::getName() << " suspending at " << mInterrupts + << " interrupts" << LL_ENDL; + llcoro::suspend(); + } +} + /***************************************************************************** * atexit() *****************************************************************************/ lua_function(atexit, "atexit(function): " "register Lua function to be called at script termination") { + lua_checkdelta(L, -1); lluau_checkstack(L, 4); // look up the global name "table" lua_getglobal(L, "table"); @@ -758,6 +802,7 @@ std::pair LuaFunction::getState() *****************************************************************************/ lua_function(source_path, "source_path(): return the source path of the running Lua script") { + lua_checkdelta(L, 1); lluau_checkstack(L, 1); lua_pushstdstring(L, lluau::source_path(L).u8string()); return 1; @@ -768,6 +813,7 @@ lua_function(source_path, "source_path(): return the source path of the running *****************************************************************************/ lua_function(source_dir, "source_dir(): return the source directory of the running Lua script") { + lua_checkdelta(L, 1); lluau_checkstack(L, 1); lua_pushstdstring(L, lluau::source_path(L).parent_path().u8string()); return 1; @@ -779,6 +825,7 @@ lua_function(source_dir, "source_dir(): return the source directory of the runni lua_function(abspath, "abspath(path): " "for given filesystem path relative to running script, return absolute path") { + lua_checkdelta(L); auto path{ lua_tostdstring(L, 1) }; lua_pop(L, 1); lua_pushstdstring(L, (lluau::source_path(L).parent_path() / path).u8string()); @@ -790,6 +837,7 @@ lua_function(abspath, "abspath(path): " *****************************************************************************/ lua_function(check_stop, "check_stop(): ensure that a Lua script responds to viewer shutdown") { + lua_checkdelta(L); LLCoros::checkStop(); return 0; } @@ -994,3 +1042,27 @@ std::ostream& operator<<(std::ostream& out, const lua_stack& self) out << ']'; return out; } + +/***************************************************************************** +* LuaStackDelta +*****************************************************************************/ +LuaStackDelta::LuaStackDelta(lua_State* L, const std::string& where, int delta): + L(L), + mWhere(where), + mDepth(lua_gettop(L)), + mDelta(delta) +{} + +LuaStackDelta::~LuaStackDelta() +{ + auto depth{ lua_gettop(L) }; + if (mDepth + mDelta != depth) + { + LL_ERRS("Lua") << mWhere << ": Lua stack went from " << mDepth << " to " << depth; + if (mDelta) + { + LL_CONT << ", rather than expected " << (mDepth + mDelta) << " (" << mDelta << ")"; + } + LL_ENDL; + } +} diff --git a/indra/llcommon/lua_function.h b/indra/llcommon/lua_function.h index c1a4d736a0..6965e206ab 100644 --- a/indra/llcommon/lua_function.h +++ b/indra/llcommon/lua_function.h @@ -60,13 +60,10 @@ namespace lluau int loadstring(lua_State* L, const std::string& desc, const std::string& text); fsyspath source_path(lua_State* L); - - void set_interrupts_counter(lua_State *L, S32 counter); - void check_interrupts_counter(lua_State* L); } // namespace lluau -// must be a macro because __FUNCTION__ is context-sensitive -#define lluau_checkstack(L, n) luaL_checkstack((L), (n), __FUNCTION__) +// must be a macro because LL_PRETTY_FUNCTION is context-sensitive +#define lluau_checkstack(L, n) luaL_checkstack((L), (n), LL_PRETTY_FUNCTION) std::string lua_tostdstring(lua_State* L, int index); void lua_pushstdstring(lua_State* L, const std::string& str); @@ -110,10 +107,18 @@ public: // Find or create LuaListener for passed lua_State. static LuaListener& obtainListener(lua_State* L); + // Given lua_State* L, return the LuaState object managing (the main Lua + // thread for) L. + static LuaState& getParent(lua_State* L); + + void set_interrupts_counter(S32 counter); + void check_interrupts_counter(); + private: script_finished_fn mCallback; lua_State* mState; std::string mError; + S32 mInterrupts{ 0 }; }; /***************************************************************************** @@ -172,6 +177,32 @@ private: int mIndex; }; +/***************************************************************************** +* LuaStackDelta +*****************************************************************************/ +/** + * Instantiate LuaStackDelta in a block to compare the Lua data stack depth on + * entry (LuaStackDelta construction) and exit. Optionally, pass the expected + * depth increment. (But be aware that LuaStackDelta cannot observe the effect + * of a LuaPopper or LuaRemover declared previously in the same block.) + */ +class LuaStackDelta +{ +public: + LuaStackDelta(lua_State* L, const std::string& where, int delta=0); + LuaStackDelta(const LuaStackDelta&) = delete; + LuaStackDelta& operator=(const LuaStackDelta&) = delete; + + ~LuaStackDelta(); + +private: + lua_State* L; + std::string mWhere; + int mDepth, mDelta; +}; + +#define lua_checkdelta(L, ...) LuaStackDelta delta(L, LL_PRETTY_FUNCTION, ##__VA_ARGS__) + /***************************************************************************** * lua_push() wrappers for generic code *****************************************************************************/ @@ -297,6 +328,7 @@ auto lua_to(lua_State* L, int index) template auto lua_getfieldv(lua_State* L, int index, const char* k) { + lua_checkdelta(L); lluau_checkstack(L, 1); lua_getfield(L, index, k); LuaPopper pop(L, 1); @@ -307,6 +339,7 @@ auto lua_getfieldv(lua_State* L, int index, const char* k) template auto lua_setfieldv(lua_State* L, int index, const char* k, const T& value) { + lua_checkdelta(L); lluau_checkstack(L, 1); lua_push(L, value); lua_setfield(L, index, k); @@ -316,6 +349,7 @@ auto lua_setfieldv(lua_State* L, int index, const char* k, const T& value) template auto lua_rawgetfield(lua_State* L, int index, const std::string_view& k) { + lua_checkdelta(L); lluau_checkstack(L, 1); lua_pushlstring(L, k.data(), k.length()); lua_rawget(L, index); @@ -327,6 +361,7 @@ auto lua_rawgetfield(lua_State* L, int index, const std::string_view& k) template void lua_rawsetfield(lua_State* L, int index, const std::string_view& k, const T& value) { + lua_checkdelta(L); lluau_checkstack(L, 2); lua_pushlstring(L, k.data(), k.length()); lua_push(L, value); @@ -433,6 +468,7 @@ DistinctInt TypeTag::value; template void lua_emplace(lua_State* L, ARGS&&... args) { + lua_checkdelta(L, 1); lluau_checkstack(L, 1); int tag{ TypeTag::value }; if (! lua_getuserdatadtor(L, tag)) @@ -467,6 +503,7 @@ void lua_emplace(lua_State* L, ARGS&&... args) template T* lua_toclass(lua_State* L, int index) { + lua_checkdelta(L); // get void* pointer to userdata (if that's what it is) void* ptr{ lua_touserdatatagged(L, index, TypeTag::value) }; // Derive the T* from ptr. If in future lua_emplace() must manually diff --git a/indra/newview/llluamanager.cpp b/indra/newview/llluamanager.cpp index 0b1a73f4e9..6de8829308 100644 --- a/indra/newview/llluamanager.cpp +++ b/indra/newview/llluamanager.cpp @@ -53,6 +53,7 @@ std::map LLLUAmanager::sScriptNames; lua_function(sleep, "sleep(seconds): pause the running coroutine") { + lua_checkdelta(L, -1); F32 seconds = lua_tonumber(L, -1); lua_pop(L, 1); llcoro::suspendUntilTimeout(seconds); @@ -125,6 +126,7 @@ lua_function(print_warning, "print_warning(args...): WARNING level logging") lua_function(post_on, "post_on(pumpname, data): post specified data to specified LLEventPump") { + lua_checkdelta(L, -2); std::string pumpname{ lua_tostdstring(L, 1) }; LLSD data{ lua_tollsd(L, 2) }; lua_pop(L, 2); @@ -139,6 +141,7 @@ lua_function(get_event_pumps, "Events posted to replypump are queued for get_event_next().\n" "post_on(commandpump, ...) to engage LLEventAPI operations (see helpleap()).") { + lua_checkdelta(L, 2); lluau_checkstack(L, 2); auto& listener{ LuaState::obtainListener(L) }; // return the reply pump name and the command pump name on caller's lua_State @@ -153,6 +156,7 @@ lua_function(get_event_next, "is returned by get_event_pumps(). Blocks the calling chunk until an\n" "event becomes available.") { + lua_checkdelta(L, 2); lluau_checkstack(L, 2); auto& listener{ LuaState::obtainListener(L) }; const auto& [pump, data]{ listener.getNext() }; @@ -271,6 +275,7 @@ std::string read_file(const std::string &name) lua_function(require, "require(module_name) : load module_name.lua from known places") { + lua_checkdelta(L); std::string name = lua_tostdstring(L, 1); lua_pop(L, 1); -- cgit v1.2.3 From d37aa5c1823fcb202e87b1457842e49655c72b95 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Aug 2024 21:22:46 -0400 Subject: Defend timers.Timer(iterate=True) against long callbacks. Specifically, defend against a callback that runs so long it suspends at a point after the next timer tick. --- indra/newview/scripts/lua/require/timers.lua | 34 +++++++++++++++++++++------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/indra/newview/scripts/lua/require/timers.lua b/indra/newview/scripts/lua/require/timers.lua index e4938078dc..ab1615ffbf 100644 --- a/indra/newview/scripts/lua/require/timers.lua +++ b/indra/newview/scripts/lua/require/timers.lua @@ -34,35 +34,53 @@ function timers.Timer:new(delay, callback, iterate) callback = callback or function() obj:tick() end - local first = true + local calls = 0 if iterate then + -- With iterative timers, beware of running a timer callback which + -- performs async actions lasting longer than the timer interval. The + -- lengthy callback suspends, allowing leap to retrieve the next + -- event, which is a timer tick. leap calls a new instance of the + -- callback, even though the previous callback call is still + -- suspended... etc. 'in_callback' defends against that recursive + -- case. Rather than re-enter the suspended callback, drop the + -- too-soon timer event. (We could count the too-soon timer events and + -- iterate calling the callback, but it's a bathtub problem: the + -- callback could end up getting farther and farther behind.) + local in_callback = false obj.id = leap.eventstream( 'Timers', {op='scheduleEvery', every=delay}, function (event) local reqid = event.reqid - if first then - first = false + calls += 1 + if calls == 1 then dbg('timer(%s) first callback', reqid) -- discard the first (immediate) response: don't call callback return nil else - dbg('timer(%s) nth callback', reqid) - return callback(event) + if in_callback then + dbg('dropping timer(%s) callback %d', reqid, calls) + else + dbg('timer(%s) callback %d', reqid, calls) + in_callback = true + local ret = callback(event) + in_callback = false + return ret + end end end ).reqid - else + else -- (not iterate) obj.id = leap.eventstream( 'Timers', {op='scheduleAfter', after=delay}, function (event) + calls += 1 -- Arrange to return nil the first time, true the second. This -- callback is called immediately with the response to -- 'scheduleAfter', and if we immediately returned true, we'd -- be done, and the subsequent timer event would be discarded. - if first then - first = false + if calls == 1 then -- Caller doesn't expect an immediate callback. return nil else -- cgit v1.2.3 From 409745eb370d7609df70da81584142dffbfe9d3f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Aug 2024 21:41:26 -0400 Subject: Fix a couple more set_interrupts_counter() calls. --- indra/newview/llluamanager.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/indra/newview/llluamanager.cpp b/indra/newview/llluamanager.cpp index 6de8829308..22b51d7b72 100644 --- a/indra/newview/llluamanager.cpp +++ b/indra/newview/llluamanager.cpp @@ -57,7 +57,7 @@ lua_function(sleep, "sleep(seconds): pause the running coroutine") F32 seconds = lua_tonumber(L, -1); lua_pop(L, 1); llcoro::suspendUntilTimeout(seconds); - lluau::set_interrupts_counter(L, 0); + LuaState::getParent(L).set_interrupts_counter(0); return 0; }; @@ -162,7 +162,7 @@ lua_function(get_event_next, const auto& [pump, data]{ listener.getNext() }; lua_pushstdstring(L, pump); lua_pushllsd(L, data); - lluau::set_interrupts_counter(L, 0); + LuaState::getParent(L).set_interrupts_counter(0); return 2; } -- cgit v1.2.3 From f4b650b100c120ed99208545864b0a7f36ce058d Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 21 Aug 2024 09:12:52 -0400 Subject: Suppress ~LuaStackDelta() verification during stack unwinding. Otherwise, an exception raised in the block containing a LuaStackDelta instance -- that might be caught -- would result in an LL_ERRS() crash. We can't expect a block exited via exception to keep its contract wrt the Lua data stack. --- indra/llcommon/lua_function.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/indra/llcommon/lua_function.cpp b/indra/llcommon/lua_function.cpp index ad77a1e040..1e9bbdd651 100644 --- a/indra/llcommon/lua_function.cpp +++ b/indra/llcommon/lua_function.cpp @@ -1056,7 +1056,10 @@ LuaStackDelta::LuaStackDelta(lua_State* L, const std::string& where, int delta): LuaStackDelta::~LuaStackDelta() { auto depth{ lua_gettop(L) }; - if (mDepth + mDelta != depth) + // If we're unwinding the stack due to an exception, then of course we + // can't expect the logic in the block containing this LuaStackDelta + // instance to keep its contract wrt the Lua data stack. + if (std::uncaught_exceptions() == 0 && mDepth + mDelta != depth) { LL_ERRS("Lua") << mWhere << ": Lua stack went from " << mDepth << " to " << depth; if (mDelta) -- cgit v1.2.3 From f2abd050bce3c4132f12d962fc6436d1f06666bd Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 21 Aug 2024 09:29:26 -0400 Subject: Improve diagnostic output for Lua atexit() functions. --- indra/llcommon/lua_function.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/indra/llcommon/lua_function.cpp b/indra/llcommon/lua_function.cpp index 1e9bbdd651..452bc24b16 100644 --- a/indra/llcommon/lua_function.cpp +++ b/indra/llcommon/lua_function.cpp @@ -496,6 +496,14 @@ LuaState::~LuaState() // stack contains Registry.atexit if (lua_istable(mState, -1)) { + // We happen to know that Registry.atexit is built by appending array + // entries using table.insert(). That's important because it means + // there are no holes, and therefore lua_objlen() should be correct. + // That's important because we walk the atexit table backwards, to + // destroy last the things we created (passed to LL.atexit()) first. + int len(lua_objlen(mState, -1)); + LL_DEBUGS("Lua") << "Registry.atexit is a table with " << len << " entries" << LL_ENDL; + // Push debug.traceback() onto the stack as lua_pcall()'s error // handler function. On error, lua_pcall() calls the specified error // handler function with the original error message; the message @@ -509,12 +517,7 @@ LuaState::~LuaState() lua_remove(mState, -2); // stack now contains atexit, debug.traceback() - // We happen to know that Registry.atexit is built by appending array - // entries using table.insert(). That's important because it means - // there are no holes, and therefore lua_objlen() should be correct. - // That's important because we walk the atexit table backwards, to - // destroy last the things we created (passed to LL.atexit()) first. - for (int i(lua_objlen(mState, -2)); i >= 1; --i) + for (int i(len); i >= 1; --i) { lua_pushinteger(mState, i); // stack contains Registry.atexit, debug.traceback(), i @@ -524,13 +527,15 @@ LuaState::~LuaState() // Use lua_pcall() because errors in any one atexit() function // shouldn't cancel the rest of them. Pass debug.traceback() as // the error handler function. + LL_DEBUGS("Lua") << "Calling atexit(" << i << ")" << LL_ENDL; if (lua_pcall(mState, 0, 0, -2) != LUA_OK) { auto error{ lua_tostdstring(mState, -1) }; - LL_WARNS("Lua") << "atexit() function error: " << error << LL_ENDL; + LL_WARNS("Lua") << "atexit(" << i << ") error: " << error << LL_ENDL; // pop error message lua_pop(mState, 1); } + LL_DEBUGS("Lua") << "atexit(" << i << ") done" << LL_ENDL; // lua_pcall() has already popped atexit[i]: // stack contains atexit, debug.traceback() } -- cgit v1.2.3