From 6a0ca0bff868a5a6511291619936aba568f38858 Mon Sep 17 00:00:00 2001 From: nerix Date: Sat, 6 Sep 2025 12:02:38 +0200 Subject: [PATCH] fix(plugins): handle non-string errors gracefully (#6441) --- CHANGELOG.md | 1 + src/controllers/plugins/SolTypes.cpp | 47 ++++++++++++++++++++++++++++ src/controllers/plugins/SolTypes.hpp | 5 +-- src/widgets/PluginRepl.cpp | 3 +- 4 files changed, 52 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4aa85e3..4dd97a08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unversioned +- Bugfix: Fixed crashes that could occur when Lua functions errored with values other than strings. (#6441) - Bugfix: Fixed zero-width global BTTV emotes not showing in the `:~` completions. (#6440) ## 2.5.4-beta.1 diff --git a/src/controllers/plugins/SolTypes.cpp b/src/controllers/plugins/SolTypes.cpp index 41d8062d..c66d8081 100644 --- a/src/controllers/plugins/SolTypes.cpp +++ b/src/controllers/plugins/SolTypes.cpp @@ -12,6 +12,8 @@ namespace chatterino::lua { +using namespace Qt::Literals; + Plugin *ThisPluginState::plugin() { if (this->plugptr_ != nullptr) @@ -27,6 +29,51 @@ Plugin *ThisPluginState::plugin() return pl; } +QString errorResultToString(const sol::protected_function_result &result) +{ + assert(!result.valid() && + "This function must be called on invalid/error results"); + + auto optString = sol::stack::check_get(result.lua_state(), -1); + if (optString) + { + return *std::move(optString); + } + + // If we get here, the stack didn't contain a string at the top. This is + // valid in Lua, but unconventional. Error handlers typically expect a + // string at the top of the stack. + // + // There can be many reasons for this; here are three: + // - A C++ function was not wrapped in a trampoline (i.e. try{} catch{}). + // sol usually does this for us, but there are some exceptions. + // If that's the case, then Lua will catch our error in a catch(...). + // It effectively swallows the error. This won't always cause us to end up + // here. For example, a function that takes a string as an argument will + // have this string at the top of the stack. When the error is swallowed, + // we'd return that argument as the error. Unfortunately, we can't detect + // this. + // The workaround here is to use luaL_error() instead of C++ exceptions. + // That function will eventually throw an error too, so the stack is + // properly unwound (requires Lua being compiled as C++). + // + // - The error is popped _during unwinding_ (due to RAII). + // If an error is thrown and a function in the C++ call stack has + // variables with a destructor that pops a value from the Lua stack, this + // might occur. + // You can detect where the error is removed by setting a breakpoint + // in lua_settop() (lapi.c) once the unwinding begins (most debuggers + // allow breaking on C++ exceptions). + // + // - One can also raise an error from Lua by calling + // `error(message[, level])`. The `message` is the "error object". As with + // `lua_error()`, the object passed doesn't need to be a string, but it's + // one by convention. If we get here because of this, that's not a bug. + return u"(no error message) " + "Unless an error without a message string was explicitly thrown, " + "this is a bug in Chatterino. Please report this."_s; +} + void logError(Plugin *plugin, QStringView context, const QString &msg) { QString fullMessage = context % u" - " % msg; diff --git a/src/controllers/plugins/SolTypes.hpp b/src/controllers/plugins/SolTypes.hpp index d7a05c7b..7a7401d2 100644 --- a/src/controllers/plugins/SolTypes.hpp +++ b/src/controllers/plugins/SolTypes.hpp @@ -60,6 +60,8 @@ private: lua_State *state_; }; +QString errorResultToString(const sol::protected_function_result &result); + /// @brief Attempts to call @a function with @a args /// /// @a T is expected to be returned. @@ -79,9 +81,8 @@ inline nonstd::expected_lite::expected tryCall(const auto &function, function(std::forward(args)...); if (!result.valid()) { - sol::error err = result; return nonstd::expected_lite::make_unexpected( - QString::fromUtf8(err.what())); + errorResultToString(result)); } if constexpr (std::is_same_v) diff --git a/src/widgets/PluginRepl.cpp b/src/widgets/PluginRepl.cpp index a4c8c026..2f68400b 100644 --- a/src/widgets/PluginRepl.cpp +++ b/src/widgets/PluginRepl.cpp @@ -596,8 +596,7 @@ void PluginRepl::logResult(const sol::protected_function_result &res, { if (!res.valid()) { - sol::error err = res; - this->log(lua::api::LogLevel::Critical, err.what()); + this->log(lua::api::LogLevel::Critical, lua::errorResultToString(res)); return; } if (res.return_count() == 0)