Merge pull request #3197 from secondlife/maxim/viewer-3119

#3119 crash when lua requests debug settings value

Author: maxim-productengine

indra/llcommon/lua_function.cpp

@@ -764,6 +764,34 @@ int lua_metaipair(lua_State* L)
 
 } // anonymous namespace
 
+int LuaState::push_debug_traceback()
+{
+    // 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");
+    if (!lua_istable(mState, -1))
+    {
+        lua_pop(mState, 1);
+        LL_WARNS("Lua") << "'debug' table not found" << LL_ENDL;
+        return 0;
+    }
+    lua_getfield(mState, -1, "traceback");
+    if (!lua_isfunction(mState, -1))
+    {
+        lua_pop(mState, 2);
+        LL_WARNS("Lua") << "'traceback' func not found" << LL_ENDL;
+        return 0;
+    }
+    // ditch "debug"
+    lua_remove(mState, -2);
+    return lua_gettop(mState);
+}
+
 LuaState::~LuaState()
 {
     // If we're unwinding the stack due to an exception, don't bother trying
@@ -784,6 +812,8 @@ LuaState::~LuaState()
     // stack contains Registry.atexit
     if (lua_istable(mState, -1))
     {
+        int atexit = lua_gettop(mState);
+
         // 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.
@@ -793,36 +823,25 @@ LuaState::~LuaState()
         LL_DEBUGS("Lua") << LLCoros::getName() << ": 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
-        // 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()
-
+        // TODO: 'debug' global shouldn't be overwritten and should be accessible at this stage
+        S32 debug_traceback_idx = push_debug_traceback();
+        // if debug_traceback is true, stack now contains atexit, /debug.traceback()/
+        // otherwise just atexit
         for (int i(len); i >= 1; --i)
         {
             lua_pushinteger(mState, i);
-            // stack contains Registry.atexit, debug.traceback(), i
-            lua_gettable(mState, -3);
-            // stack contains Registry.atexit, debug.traceback(), atexit[i]
+            // stack contains Registry.atexit, /debug.traceback()/, i
+            lua_gettable(mState, atexit);
+            // 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. Pass debug.traceback() as
             // the error handler function.
-            LL_DEBUGS("Lua") << LLCoros::getName()
-                             << ": calling atexit(" << i << ")" << LL_ENDL;
-            if (lua_pcall(mState, 0, 0, -2) != LUA_OK)
+            LL_DEBUGS("Lua") << LLCoros::getName() << ": calling atexit(" << i << ")" << LL_ENDL;
+            if (lua_pcall(mState, 0, 0, debug_traceback_idx) != LUA_OK)
             {
                 auto error{ lua_tostdstring(mState, -1) };
-                LL_WARNS("Lua") << LLCoros::getName()
-                                << ": atexit(" << i << ") error: " << error << LL_ENDL;
+                LL_WARNS("Lua") << LLCoros::getName() << ": atexit(" << i << ") error: " << error << LL_ENDL;
                 // pop error message
                 lua_pop(mState, 1);
             }
@@ -831,7 +850,8 @@ LuaState::~LuaState()
             // stack contains atexit, debug.traceback()
         }
         // pop debug.traceback()
-        lua_pop(mState, 1);
+        if (debug_traceback_idx)
+            lua_remove(mState, debug_traceback_idx);
     }
     // pop Registry.atexit (either table or nil)
     lua_pop(mState, 1);

indra/llcommon/lua_function.h

@@ -122,6 +122,8 @@ class LuaState
     void set_interrupts_counter(S32 counter);
     void check_interrupts_counter();
 
+    int push_debug_traceback();
+
 private:
     /*---------------------------- feature flag ----------------------------*/
     bool mFeature{ false };

indra/newview/scripts/lua/test_debugsettings.lua

@@ -0,0 +1,9 @@
+local LLDebugSettings = require 'LLDebugSettings'
+local SETTING_NAME = '360CaptureCameraFOV'
+LL.atexit(function() LL.print_info('Finished') end)
+
+LL.print_info('Updating ' .. SETTING_NAME)
+LLDebugSettings.set(SETTING_NAME, 100)
+-- 'inadvertently' using Luau library as variable name to hide it
+debug = LLDebugSettings.get(SETTING_NAME)
+LL.print_info(debug)