fix(plugins): don't crash on invalid plugin meta (#7166)

This makes our plugin storage use a variant for the different plugin
states:

- UnloadedPlugin (used if the plugin's meta was invalid)
- PluginPtr (a normal valid plugin, which may still contain lua errors)

How to test the crash that doesn't crash with this PR:
Add an invalid permission in your `info.json` file

Co-authored-by: Mm2PL <mm2pl+gh@kotmisia.pl>

src/controllers/plugins/Plugin.hpp

@@ -18,10 +18,13 @@
 #    include <semver/semver.hpp>
 #    include <sol/forward.hpp>
 
+#    include <cassert>
 #    include <memory>
 #    include <optional>
 #    include <unordered_map>
 #    include <unordered_set>
+#    include <utility>
+#    include <variant>
 #    include <vector>
 
 struct lua_State;
@@ -37,6 +40,25 @@ struct SignalCallback;
 
 namespace chatterino {
 
+/// A plugin that hasn't been loaded.
+///
+/// Most likely, its metadata is invalid.
+class UnloadedPlugin
+{
+public:
+    QString id;
+    PluginMeta meta;
+    QDir loadDirectory;
+
+    UnloadedPlugin(QString id_, PluginMeta meta_, const QDir &loadDirectory_)
+        : id(std::move(id_))
+        , meta(std::move(meta_))
+        , loadDirectory(loadDirectory_)
+    {
+    }
+};
+
+/// A plugin with a valid lua state
 class Plugin
 {
 public:
@@ -49,8 +71,12 @@ public:
         , meta(std::move(meta))
         , loadDirectory_(loadDirectory)
         , state_(state)
-        , selfRef_(state ? this : nullptr)
+        , selfRef_(this)
     {
+        // The PluginMeta here must be valid, otherwise it should be initialized
+        // as an UnloadedPlugin
+        assert(this->meta.isValid());
+        assert(this->state_ != nullptr);
     }
 
     ~Plugin();
@@ -148,5 +174,10 @@ private:
     friend class PluginController;
     friend class PluginControllerAccess;  // this is for tests
 };
+
+using PluginPtr = std::unique_ptr<Plugin>;
+using AnyPlugin = std::variant<PluginPtr, UnloadedPlugin>;
+
 }  // namespace chatterino
+
 #endif

src/controllers/plugins/PluginController.cpp

@@ -32,6 +32,7 @@
 #    include "singletons/Paths.hpp"
 #    include "singletons/Settings.hpp"
 #    include "singletons/WindowManager.hpp"
+#    include "util/Variant.hpp"
 #    include "widgets/splits/SplitContainer.hpp"
 #    include "widgets/Window.hpp"
 
@@ -131,9 +132,10 @@ bool PluginController::tryLoadFromDir(const QDir &pluginDir)
         {
             qCWarning(chatterinoLua) << "- " << why;
         }
-        auto plugin = std::make_unique<Plugin>(pluginDir.dirName(), nullptr,
-                                               meta, pluginDir);
-        this->plugins_.insert({pluginDir.dirName(), std::move(plugin)});
+
+        this->plugins_.insert(
+            {pluginDir.dirName(),
+             UnloadedPlugin{pluginDir.dirName(), meta, pluginDir}});
         return false;
     }
     this->load(index, pluginDir, meta);
@@ -357,11 +359,26 @@ bool PluginController::reload(const QString &id)
         return false;
     }
 
-    for (const auto &[cmd, _] : it->second->ownedCommands)
+    if (const auto *oPlugin = std::get_if<PluginPtr>(&it->second))
     {
-        getApp()->getCommands()->unregisterPluginCommand(cmd);
+        const auto &plugin = *oPlugin;
+
+        for (const auto &[cmd, _] : plugin->ownedCommands)
+        {
+            getApp()->getCommands()->unregisterPluginCommand(cmd);
+        }
     }
-    QDir loadDir = it->second->loadDirectory_;
+
+    const auto loadDir = std::visit(variant::Overloaded{
+                                        [&](const PluginPtr &plugin) {
+                                            return plugin->loadDirectory_;
+                                        },
+                                        [&](const UnloadedPlugin &plugin) {
+                                            return plugin.loadDirectory;
+                                        },
+                                    },
+                                    it->second);
+
     // Since Plugin owns the state, it will clean up everything related to it
     this->plugins_.erase(id);
     this->queueChangeNotification();
@@ -372,8 +389,15 @@ bool PluginController::reload(const QString &id)
 QString PluginController::tryExecPluginCommand(const QString &commandName,
                                                const CommandContext &ctx)
 {
-    for (auto &[name, plugin] : this->plugins_)
+    for (const auto &[name, anyPlugin] : this->allPlugins())
     {
+        const auto *oPl = std::get_if<PluginPtr>(&anyPlugin);
+        if (oPl == nullptr)
+        {
+            continue;
+        }
+        const auto &plugin = *oPl;
+
         if (auto it = plugin->ownedCommands.find(commandName);
             it != plugin->ownedCommands.end())
         {
@@ -421,8 +445,15 @@ Plugin *PluginController::getPluginByStatePtr(lua_State *L)
     auto *mainL = lua_tothread(L, -1);
     lua_pop(L, 1);
     L = mainL;
-    for (auto &[name, plugin] : this->plugins_)
+    for (const auto &[name, anyPlugin] : this->allPlugins())
     {
+        const auto *oPl = std::get_if<PluginPtr>(&anyPlugin);
+        if (oPl == nullptr)
+        {
+            continue;
+        }
+        const auto &plugin = *oPl;
+
         if (plugin->state_ == L)
         {
             return plugin.get();
@@ -431,8 +462,19 @@ Plugin *PluginController::getPluginByStatePtr(lua_State *L)
     return nullptr;
 }
 
-const std::map<QString, std::unique_ptr<Plugin>> &PluginController::plugins()
-    const
+void PluginController::forEachPlugin(
+    FunctionRef<void(const std::unique_ptr<Plugin> &)> cb) const
+{
+    for (const auto &[_, anyPlugin] : this->allPlugins())
+    {
+        if (const auto *plugin = std::get_if<PluginPtr>(&anyPlugin))
+        {
+            cb(*plugin);
+        }
+    }
+}
+
+const std::map<QString, AnyPlugin> &PluginController::allPlugins() const
 {
     return this->plugins_;
 }
@@ -443,8 +485,16 @@ std::pair<bool, QStringList> PluginController::updateCustomCompletions(
 {
     QStringList results;
 
-    for (const auto &[name, pl] : this->plugins())
+    for (const auto &[name, anyPlugin] : this->allPlugins())
     {
+        const auto *oPl = std::get_if<PluginPtr>(&anyPlugin);
+        if (oPl == nullptr)
+        {
+            continue;
+        }
+
+        const auto &pl = *oPl;
+
         if (!pl->error().isNull() || pl->state_ == nullptr)
         {
             continue;

src/controllers/plugins/PluginController.hpp

@@ -9,6 +9,7 @@
 #    include "common/websockets/WebSocketPool.hpp"
 #    include "controllers/commands/CommandContext.hpp"
 #    include "controllers/plugins/Plugin.hpp"
+#    include "util/FunctionRef.hpp"
 
 #    include <pajlada/signals/signal.hpp>
 #    include <QDir>
@@ -45,8 +46,11 @@ public:
     // This is required to be public because of c functions
     Plugin *getPluginByStatePtr(lua_State *L);
 
-    // TODO: make a function that iterates plugins that aren't errored/enabled
-    const std::map<QString, std::unique_ptr<Plugin>> &plugins() const;
+    /// Run `cb` on every loaded plugin, including those with load errors
+    void forEachPlugin(
+        FunctionRef<void(const std::unique_ptr<Plugin> &)>) const;
+
+    const std::map<QString, AnyPlugin> &allPlugins() const;
 
     /**
      * @brief Reload plugin given by id
@@ -86,7 +90,7 @@ private:
 
     void queueChangeNotification();
 
-    std::map<QString, std::unique_ptr<Plugin>> plugins_;
+    std::map<QString, AnyPlugin> plugins_;
     WebSocketPool webSocketPool_;
 
     std::vector<

src/widgets/PluginRepl.cpp

@@ -665,16 +665,24 @@ void PluginRepl::log(std::optional<lua::api::LogLevel> level,
 
 void PluginRepl::tryUpdate()
 {
-    auto it = getApp()->getPlugins()->plugins().find(this->id);
-    if (it == getApp()->getPlugins()->plugins().end())
+    auto it = getApp()->getPlugins()->allPlugins().find(this->id);
+    if (it == getApp()->getPlugins()->allPlugins().end())
     {
         return;
     }
+
+    const auto *oPl = std::get_if<PluginPtr>(&it->second);
+    if (oPl == nullptr)
+    {
+        return;
+    }
+    const auto &pl = *oPl;
+
     if (!PluginController::isPluginEnabled(this->id))
     {
         return;
     }
-    this->setPlugin(it->second.get());
+    this->setPlugin(pl.get());
 }
 
 void PluginRepl::setPlugin(Plugin *plugin)

src/widgets/settingspages/PluginsPage.cpp

@@ -8,12 +8,14 @@
 #    include "Application.hpp"
 #    include "common/Args.hpp"
 #    include "controllers/accounts/AccountController.hpp"
+#    include "controllers/plugins/Plugin.hpp"
 #    include "controllers/plugins/PluginController.hpp"
 #    include "singletons/Paths.hpp"
 #    include "singletons/Settings.hpp"
 #    include "util/Helpers.hpp"
 #    include "util/LayoutCreator.hpp"
 #    include "util/RemoveScrollAreaBackground.hpp"
+#    include "util/Variant.hpp"
 #    include "widgets/PluginRepl.hpp"
 #    include "widgets/settingspages/SettingWidget.hpp"
 
@@ -121,54 +123,53 @@ void PluginsPage::rebuildContent()
     this->scrollAreaWidget_.append(this->dataFrame_);
     auto layout = frame.setLayoutType<QVBoxLayout>();
     layout->setParent(this->dataFrame_);
-    for (const auto &[id, plugin] : getApp()->getPlugins()->plugins())
+    for (const auto &[id, plugin] : getApp()->getPlugins()->allPlugins())
     {
+        const auto &meta = std::visit(variant::Overloaded{
+                                          [&](const PluginPtr &plugin) {
+                                              return plugin->meta;
+                                          },
+                                          [&](const UnloadedPlugin &plugin) {
+                                              return plugin.meta;
+                                          },
+                                      },
+                                      plugin);
+
         auto groupHeaderText =
             QString("%1 (%2, from %3)")
-                .arg(plugin->meta.name,
-                     QString::fromStdString(plugin->meta.version.to_string()),
-                     id);
+                .arg(meta.name,
+                     QString::fromStdString(meta.version.to_string()), id);
         auto groupBox = layout.emplace<QGroupBox>(groupHeaderText);
         groupBox->setParent(this->dataFrame_);
+
         auto pluginEntry = groupBox.setLayoutType<QFormLayout>();
         pluginEntry->setParent(groupBox.getElement());
 
-        if (!plugin->meta.isValid())
+        if (!meta.isValid())
         {
             QString errors = "<ul>";
-            for (const auto &err : plugin->meta.errors)
+            for (const auto &err : meta.errors)
             {
                 errors += "<li>" + err.toHtmlEscaped() + "</li>";
             }
             errors += "</ul>";
 
-            auto *warningLabel = new QLabel(
-                "There were errors while loading metadata for this plugin:" +
-                    errors,
-                this->dataFrame_);
+            auto *warningLabel = new QLabel("There were errors while loading "
+                                            "metadata for this plugin:" +
+                                                errors,
+                                            this->dataFrame_);
             warningLabel->setTextFormat(Qt::RichText);
             warningLabel->setStyleSheet("color: #f00");
             pluginEntry->addRow(warningLabel);
         }
-        if (!plugin->error().isNull())
-        {
-            auto *errorLabel =
-                new QLabel("There was an error while loading this plugin: " +
-                               plugin->error(),
-                           this->dataFrame_);
-            errorLabel->setStyleSheet("color: #f00");
-            errorLabel->setWordWrap(true);
-            pluginEntry->addRow(errorLabel);
-        }
 
-        auto *description =
-            new QLabel(plugin->meta.description, this->dataFrame_);
+        auto *description = new QLabel(meta.description, this->dataFrame_);
         description->setWordWrap(true);
         description->setStyleSheet("color: #bbb");
         pluginEntry->addRow(description);
 
         QString authorsTxt;
-        for (const auto &author : plugin->meta.authors)
+        for (const auto &author : meta.authors)
         {
             if (!authorsTxt.isEmpty())
             {
@@ -180,32 +181,49 @@ void PluginsPage::rebuildContent()
         pluginEntry->addRow("Authors",
                             new QLabel(authorsTxt, this->dataFrame_));
 
-        if (!plugin->meta.homepage.isEmpty())
+        if (!meta.homepage.isEmpty())
         {
-            auto *homepage = new QLabel(formatRichLink(plugin->meta.homepage),
-                                        this->dataFrame_);
+            auto *homepage =
+                new QLabel(formatRichLink(meta.homepage), this->dataFrame_);
             homepage->setOpenExternalLinks(true);
             pluginEntry->addRow("Homepage", homepage);
         }
         pluginEntry->addRow("License",
-                            new QLabel(plugin->meta.license, this->dataFrame_));
+                            new QLabel(meta.license, this->dataFrame_));
 
-        QString commandsTxt;
-        for (const auto &cmdName : plugin->listRegisteredCommands())
+        if (const auto *oValidPlugin = std::get_if<PluginPtr>(&plugin))
         {
-            if (!commandsTxt.isEmpty())
+            const auto &validPlugin = *oValidPlugin;
+
+            if (!validPlugin->error().isNull())
             {
-                commandsTxt += ", ";
+                auto *errorLabel = new QLabel(
+                    "There was an error while loading this plugin: " +
+                        validPlugin->error(),
+                    this->dataFrame_);
+                errorLabel->setStyleSheet("color: #f00");
+                errorLabel->setWordWrap(true);
+                pluginEntry->addRow(errorLabel);
             }
 
-            commandsTxt += cmdName;
+            QString commandsTxt;
+            for (const auto &cmdName : validPlugin->listRegisteredCommands())
+            {
+                if (!commandsTxt.isEmpty())
+                {
+                    commandsTxt += ", ";
+                }
+
+                commandsTxt += cmdName;
+            }
+            pluginEntry->addRow("Commands",
+                                new QLabel(commandsTxt, this->dataFrame_));
         }
-        pluginEntry->addRow("Commands",
-                            new QLabel(commandsTxt, this->dataFrame_));
-        if (!plugin->meta.permissions.empty())
+
+        if (!meta.permissions.empty())
         {
             QString perms = "<ul>";
-            for (const auto &perm : plugin->meta.permissions)
+            for (const auto &perm : meta.permissions)
             {
                 perms += "<li>" + perm.toHtml() + "</li>";
             }
@@ -217,30 +235,34 @@ void PluginsPage::rebuildContent()
             pluginEntry->addRow(lbl);
         }
 
-        if (plugin->meta.isValid())
+        QString toggleTxt = "Enable";
+        if (PluginController::isPluginEnabled(id))
         {
-            QString toggleTxt = "Enable";
-            if (PluginController::isPluginEnabled(id))
-            {
-                toggleTxt = "Disable";
-            }
+            toggleTxt = "Disable";
+        }
 
-            auto *toggleButton = new QPushButton(toggleTxt, this->dataFrame_);
-            QObject::connect(
-                toggleButton, &QPushButton::pressed, [name = id, this]() {
-                    QStringList val = getSettings()->enabledPlugins;
-                    if (PluginController::isPluginEnabled(name))
-                    {
-                        val.removeAll(name);
-                    }
-                    else
-                    {
-                        val.push_back(name);
-                    }
-                    getSettings()->enabledPlugins.setValue(val);
-                    getApp()->getPlugins()->reload(name);
-                });
-            pluginEntry->addRow(toggleButton);
+        auto *toggleButton = new QPushButton(toggleTxt, this->dataFrame_);
+        QObject::connect(toggleButton, &QPushButton::pressed, [name = id]() {
+            QStringList val = getSettings()->enabledPlugins;
+            if (PluginController::isPluginEnabled(name))
+            {
+                val.removeAll(name);
+            }
+            else
+            {
+                val.push_back(name);
+            }
+            getSettings()->enabledPlugins.setValue(val);
+            getApp()->getPlugins()->reload(name);
+        });
+        pluginEntry->addRow(toggleButton);
+        if (meta.isValid())
+        {
+            toggleButton->setEnabled(true);
+        }
+        else
+        {
+            toggleButton->setEnabled(false);
         }
 
         auto *reloadButton = new QPushButton("Reload", this->dataFrame_);

tests/src/Plugins.cpp

@@ -206,7 +206,7 @@ public:
         getApp()->getPlugins()->openLibrariesFor(plugin);
     }
 
-    static std::map<QString, std::unique_ptr<Plugin>> &plugins()
+    static std::map<QString, AnyPlugin> &plugins()
     {
         return getApp()->getPlugins()->plugins_;
     }