Leak a little less memory with dynamic allocations in Lua

CHANGELOG.md

@@ -47,6 +47,7 @@
 - Bugfix: Fixed tab visibility being controllable in the emote popup. (#5530)
 - Bugfix: Fixed account switch not being saved if no other settings were changed. (#5558)
 - Bugfix: Fixed some tooltips not being readable. (#5578)
+- Bugfix: Fixed Lua plugins leaking memory in error branches. (#5508)
 - Dev: Update Windows build from Qt 6.5.0 to Qt 6.7.1. (#5420)
 - Dev: Update vcpkg build Qt from 6.5.0 to 6.7.0, boost from 1.83.0 to 1.85.0, openssl from 3.1.3 to 3.3.0. (#5422)
 - Dev: Unsingletonize `ISoundController`. (#5462)

src/controllers/plugins/LuaAPI.cpp

@@ -8,6 +8,7 @@
 #    include "controllers/plugins/PluginController.hpp"
 #    include "messages/MessageBuilder.hpp"
 #    include "providers/twitch/TwitchIrcServer.hpp"
+#    include "util/drop.hpp"
 
 extern "C" {
 #    include <lauxlib.h>
@@ -73,15 +74,21 @@ int c2_register_command(lua_State *L)
     }
 
     QString name;
-    if (!lua::peek(L, &name, 1))
+    auto pres = lua::peek(L, &name, 1);
+    if (!pres)
     {
-        luaL_error(L, "cannot get command name (1st arg of register_command, "
-                      "expected a string)");
+        pres.errorReason.push_back(
+            QString("While getting command name (1st arg of register_command, "
+                    "expected a string got %1)")
+                .arg(luaL_typename(L, 1)));
+        drop(name);
+        pres.throwAsLuaError(L);
         return 0;
     }
     if (lua_isnoneornil(L, 2))
     {
-        luaL_error(L, "missing argument for register_command: function "
+        drop(name);
+        luaL_error(L, "Missing argument for register_command: function "
                       "\"pointer\"");
         return 0;
     }
@@ -106,10 +113,15 @@ int c2_register_callback(lua_State *L)
         return 0;
     }
     EventType evtType{};
-    if (!lua::peek(L, &evtType, 1))
+    auto pres = lua::peek(L, &evtType, 1);
+    if (!pres)
     {
-        luaL_error(L, "cannot get event name (1st arg of register_callback, "
-                      "expected a string)");
+        pres.errorReason.push_back(
+            QString("cannot get event name (1st arg of register_callback, "
+                    "expected a string, got %1)")
+                .arg(lua_typename(L, 1)));
+        // no types to drop yet
+        pres.throwAsLuaError(L);
         return 0;
     }
     if (lua_isnoneornil(L, 2))
@@ -139,14 +151,25 @@ int c2_log(lua_State *L)
         luaL_error(L, "c2_log: internal error: no plugin?");
         return 0;
     }
+    if (lua_gettop(L) < 2)
+    {
+        // no arguments
+        return luaL_error(L,
+                          "c2.log expects at least two arguments, a log level "
+                          "(c2.LogLevel) and an object to print "
+                          "(usually a string)");
+    }
     auto logc = lua_gettop(L) - 1;
-    // This is almost the expansion of qCDebug() macro, actual thing is wrapped in a for loop
     LogLevel lvl{};
-    if (!lua::pop(L, &lvl, 1))
+    auto pres = lua::pop(L, &lvl, 1);
+    if (!pres)
     {
-        luaL_error(L, "Invalid log level, use one from c2.LogLevel.");
+        pres.errorReason.emplace_back(
+            "While getting log level (1st argument of c2.log)");
+        pres.throwAsLuaError(L);
         return 0;
     }
+    // This is almost the expansion of qCDebug() macro, actual thing is wrapped in a for loop
     QDebug stream = qdebugStreamForLogLevel(lvl);
     logHelper(L, pl, stream, logc);
     return 0;
@@ -224,6 +247,8 @@ int g_load(lua_State *L)
         utf8->toUnicode(data.constData(), data.size(), &state);
         if (state.invalidChars != 0)
         {
+            drop(data);
+            drop(state);
             luaL_error(L, "invalid utf-8 in load() is not allowed");
             return 0;
         }
@@ -297,6 +322,10 @@ int loadfile(lua_State *L, const QString &str)
         return 2;
     }
 
+    drop(temp);
+    drop(info);
+    drop(datadir);
+    drop(dir);
     return luaL_error(L, "error loading module '%s' from file '%s':\n\t%s",
                       lua_tostring(L, 1), filename, lua_tostring(L, -1));
 }
@@ -306,10 +335,10 @@ int searcherAbsolute(lua_State *L)
     auto name = QString::fromUtf8(luaL_checkstring(L, 1));
     name = name.replace('.', QDir::separator());
 
-    QString filename;
     auto *pl = getApp()->getPlugins()->getPluginByStatePtr(L);
     if (pl == nullptr)
     {
+        drop(name);
         return luaL_error(L, "searcherAbsolute: internal error: no plugin?");
     }
 
@@ -332,7 +361,7 @@ int searcherRelative(lua_State *L)
         lua::push(
             L,
             QString(
-                "Unable to load relative to file:caller has no source file"));
+                "Unable to load relative to file: caller has no source file"));
         return 1;
     }
 

src/controllers/plugins/LuaUtilities.cpp

@@ -156,108 +156,127 @@ StackIdx push(lua_State *L, const api::CompletionEvent &ev)
     return idx;
 }
 
-bool peek(lua_State *L, int *out, StackIdx idx)
+PeekResult peek(lua_State *L, int *out, StackIdx idx)
 {
     StackGuard guard(L);
     if (lua_isnumber(L, idx) == 0)
     {
-        return false;
+        return {
+            false,
+            {QString("Expected an integer got %1").arg(luaL_typename(L, idx))}};
     }
 
     *out = lua_tointeger(L, idx);
-    return true;
+    return {};
 }
 
-bool peek(lua_State *L, bool *out, StackIdx idx)
+PeekResult peek(lua_State *L, bool *out, StackIdx idx)
 {
     StackGuard guard(L);
     if (!lua_isboolean(L, idx))
     {
-        return false;
+        return {
+            false,
+            {QString("Expected a boolean got %1").arg(luaL_typename(L, idx))}};
     }
 
     *out = bool(lua_toboolean(L, idx));
-    return true;
+    return {};
 }
 
-bool peek(lua_State *L, double *out, StackIdx idx)
+PeekResult peek(lua_State *L, double *out, StackIdx idx)
 {
     StackGuard guard(L);
     int ok{0};
     auto v = lua_tonumberx(L, idx, &ok);
     if (ok != 0)
     {
         *out = v;
+        return {};
     }
-    return ok != 0;
+
+    return PeekResult::ofTypeError(L, idx, "integer or float");
 }
 
-bool peek(lua_State *L, QString *out, StackIdx idx)
+PeekResult peek(lua_State *L, QString *out, StackIdx idx)
 {
     StackGuard guard(L);
     size_t len{0};
     const char *str = lua_tolstring(L, idx, &len);
     if (str == nullptr)
     {
-        return false;
+        return PeekResult::ofTypeError(L, idx, "string");
     }
     if (len >= INT_MAX)
     {
-        assert(false && "string longer than INT_MAX, shit's fucked, yo");
+        return {false,
+                {QString("Strings bigger than 2.1 gigabytes are not allowed")}};
     }
     *out = QString::fromUtf8(str, int(len));
-    return true;
+    return {};
 }
 
-bool peek(lua_State *L, QByteArray *out, StackIdx idx)
+PeekResult peek(lua_State *L, QByteArray *out, StackIdx idx)
 {
     StackGuard guard(L);
     size_t len{0};
     const char *str = lua_tolstring(L, idx, &len);
     if (str == nullptr)
     {
-        return false;
+        return PeekResult::ofTypeError(L, idx, "string");
     }
     if (len >= INT_MAX)
     {
-        assert(false && "string longer than INT_MAX, shit's fucked, yo");
+        return {false,
+                {QString("Strings bigger than 2.1 gigabytes are not allowed")}};
     }
     *out = QByteArray(str, int(len));
-    return true;
+    return {};
 }
 
-bool peek(lua_State *L, std::string *out, StackIdx idx)
+PeekResult peek(lua_State *L, std::string *out, StackIdx idx)
 {
     StackGuard guard(L);
     size_t len{0};
     const char *str = lua_tolstring(L, idx, &len);
     if (str == nullptr)
     {
-        return false;
+        return PeekResult::ofTypeError(L, idx, "string");
     }
     if (len >= INT_MAX)
     {
-        assert(false && "string longer than INT_MAX, shit's fucked, yo");
+        return {false,
+                {QString("Strings bigger than 2.1 gigabytes are not allowed")}};
     }
     *out = std::string(str, len);
-    return true;
+    return {};
 }
 
-bool peek(lua_State *L, api::CompletionList *out, StackIdx idx)
+PeekResult peek(lua_State *L, api::CompletionList *out, StackIdx idx)
 {
     StackGuard guard(L);
     int typ = lua_getfield(L, idx, "values");
     if (typ != LUA_TTABLE)
     {
         lua_pop(L, 1);
-        return false;
+        auto res = PeekResult::ofTypeError(L, idx, "string");
+        res.errorReason.emplace_back("While processing CompletionList.values");
+        return res;
     }
-    if (!lua::pop(L, &out->values, -1))
+    auto pres = lua::pop(L, &out->values, -1);
+    if (!pres)
     {
-        return false;
+        pres.errorReason.emplace_back("While processing CompletionList.values");
+        return pres;
     }
     lua_getfield(L, idx, "hide_others");
-    return lua::pop(L, &out->hideOthers);
+    pres = lua::pop(L, &out->hideOthers);
+    if (!pres)
+    {
+        pres.errorReason.emplace_back(
+            "While processing CompletionList.hide_others");
+    }
+    return pres;
 }
 
 QString toString(lua_State *L, StackIdx idx)
@@ -266,5 +285,37 @@ QString toString(lua_State *L, StackIdx idx)
     const auto *ptr = luaL_tolstring(L, idx, &len);
     return QString::fromUtf8(ptr, int(len));
 }
+
+void PeekResult::throwAsLuaError(lua_State *L)
+{
+    // This uses lua buffers to ensure deallocation of the error string
+    luaL_Buffer buf;
+    luaL_buffinit(L, &buf);
+    bool first = true;
+    for (const auto &s : this->errorReason)
+    {
+        if (!first)
+        {
+            luaL_addchar(&buf, '\n');
+        }
+        first = false;
+        luaL_addstring(&buf, s.toStdString().c_str());
+    }
+    // Remove our copy
+    this->errorReason.clear();
+
+    luaL_pushresult(&buf);
+    lua_error(L);  // This call never returns
+    assert(false && "unreachable");
+}
+
+PeekResult PeekResult::ofTypeError(lua_State *L, StackIdx idx,
+                                   const QString &expect)
+{
+    return PeekResult{
+        .ok = false,
+        .errorReason = {
+            QString("Expected %1, got %2").arg(expect, luaL_typename(L, idx))}};
+}
 }  // namespace chatterino::lua
 #endif