Chore: Tidy SQL usage in login, search, and world servers

sql/synergy_recipes.sql

@@ -65,8 +65,8 @@ SET @RANK_GRANDMASTER = 14;
 SET @RANK_LEGEND      = 15;
 
 DELIMITER $$
-DROP TRIGGER IF EXISTS ensure_ingredients_are_ordered;
-CREATE TRIGGER ensure_ingredients_are_ordered
+DROP TRIGGER IF EXISTS ensure_synergy_ingredients_are_ordered;
+CREATE TRIGGER ensure_synergy_ingredients_are_ordered
      BEFORE INSERT ON synergy_recipes FOR EACH ROW BEGIN
           IF NEW.ingredient2 > 0 AND NEW.ingredient1 > NEW.ingredient2
           THEN

sql/synth_recipes.sql

@@ -52,8 +52,8 @@ CREATE TABLE `synth_recipes` (
 /*!40101 SET character_set_client = @saved_cs_client */;
 
 DELIMITER $$
-DROP TRIGGER IF EXISTS ensure_ingredients_are_ordered;
-CREATE TRIGGER ensure_ingredients_are_ordered
+DROP TRIGGER IF EXISTS ensure_synth_ingredients_are_ordered;
+CREATE TRIGGER ensure_synth_ingredients_are_ordered
      BEFORE INSERT ON synth_recipes FOR EACH ROW BEGIN
           IF NEW.Ingredient2 > 0 AND NEW.Ingredient1 > NEW.Ingredient2
           THEN

src/common/database.cpp

@@ -51,6 +51,8 @@ namespace
         "Connection refused",
         "Can't connect to server",
     };
+
+    bool timersEnabled = false;
 } // namespace
 
 auto db::getConnection() -> std::unique_ptr<sql::Connection>
@@ -127,7 +129,7 @@ auto db::detail::timer(std::string const& query) -> xi::final_action<std::functi
     {
         const auto end      = hires_clock::now();
         const auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(end - start).count();
-        if (settings::get<bool>("logging.SQL_SLOW_QUERY_LOG_ENABLE"))
+        if (timersEnabled && settings::get<bool>("logging.SQL_SLOW_QUERY_LOG_ENABLE"))
         {
             if (duration > settings::get<uint32>("logging.SQL_SLOW_QUERY_ERROR_TIME"))
             {
@@ -287,6 +289,43 @@ void db::checkCharset()
     }
 }
 
+void db::checkTriggers()
+{
+    const auto triggerQuery = "SHOW TRIGGERS WHERE `Trigger` LIKE ?";
+
+    const auto triggers = {
+        "account_delete",
+        "session_delete",
+        "auction_house_list",
+        "auction_house_buy",
+        "char_insert",
+        "char_delete",
+        "delivery_box_insert",
+        "ensure_synth_ingredients_are_ordered",
+        "ensure_synergy_ingredients_are_ordered",
+    };
+
+    bool foundError = false;
+
+    for (const auto& trigger : triggers)
+    {
+        auto rset = preparedStmt(triggerQuery, trigger);
+        if (!rset || rset->rowsCount() == 0)
+        {
+            ShowWarning(fmt::format("Missing trigger: {}", trigger));
+            foundError = true;
+        }
+    }
+
+    if (foundError)
+    {
+        ShowCriticalFmt("Missing triggers can result in data corruption or loss of data!!!");
+        ShowCriticalFmt("Please ensure all triggers are present in the database (re-run dbtool.py).");
+        std::this_thread::sleep_for(1s);
+        std::terminate();
+    }
+}
+
 bool db::setAutoCommit(bool value)
 {
     TracyZoneScoped;
@@ -352,3 +391,8 @@ bool db::transactionRollback()
 
     return true;
 }
+
+void db::enableTimers()
+{
+    timersEnabled = true;
+}

src/common/database.h

@@ -232,8 +232,8 @@ namespace db
                 {
                     if (resultSet->isNull(key.c_str()))
                     {
-                        ShowError("ResultSetWrapper::get: key %s is null", key.c_str());
-                        ShowError("Query: %s", query.c_str());
+                        ShowErrorFmt("ResultSetWrapper::get: key {} is null", key.c_str());
+                        ShowErrorFmt("Query: {}", query.c_str());
                         return value;
                     }
                 }
@@ -484,8 +484,8 @@ namespace db
         }
         catch (const std::exception& e)
         {
-            ShowError("Query Failed: %s", e.what());
-            ShowError("Query Failed: %s", str(query.c_str()));
+            ShowErrorFmt("Query Failed: {}", query);
+            ShowErrorFmt("{}", e.what());
         }
 
         return nullptr;
@@ -511,8 +511,10 @@ namespace db
                 auto& lazyPreparedStatements = state.lazyPreparedStatements;
 
                 // If we don't have it, lazily make it
+                // cppcheck-suppress stlFindInsert
                 if (lazyPreparedStatements.find(rawQuery) == lazyPreparedStatements.end())
                 {
+                    // cppcheck-suppress stlFindInsert
                     lazyPreparedStatements[rawQuery] = std::unique_ptr<sql::PreparedStatement>(state.connection->prepareStatement(rawQuery.c_str()));
                 }
 
@@ -547,8 +549,8 @@ namespace db
                 {
                     if (!detail::isConnectionIssue(e))
                     {
-                        ShowError("Query Failed: %s", rawQuery.c_str());
-                        ShowError(e.what());
+                        ShowErrorFmt("Query Failed: {}", rawQuery.c_str());
+                        ShowErrorFmt("{}", e.what());
                         return nullptr;
                     }
                 }
@@ -583,23 +585,29 @@ namespace db
                 auto& lazyPreparedStatements = state.lazyPreparedStatements;
 
                 // If we don't have it, lazily make it
+                // cppcheck-suppress stlFindInsert
                 if (lazyPreparedStatements.find(rawQuery) == lazyPreparedStatements.end())
                 {
                     try
                     {
+                        // cppcheck-suppress stlFindInsert
                         lazyPreparedStatements[rawQuery] = std::unique_ptr<sql::PreparedStatement>(state.connection->prepareStatement(rawQuery.c_str()));
                     }
                     catch (const std::exception& e)
                     {
-                        ShowError("Failed to lazy prepare query: %s", str(rawQuery.c_str()));
-                        ShowError(e.what());
+                        ShowErrorFmt("Failed to lazy prepare query: {}", rawQuery, rawQuery.size());
+                        ShowErrorFmt("{}", e.what());
                         return { nullptr, 0 };
                     }
                 }
 
-                auto rowCountQuery = "SELECT ROW_COUNT() AS count";
+                const auto rowCountQuery = "SELECT ROW_COUNT() AS count";
+
+                // If we don't have it, lazily make it
+                // cppcheck-suppress stlFindInsert
                 if (lazyPreparedStatements.find(rowCountQuery) == lazyPreparedStatements.end())
                 {
+                    // cppcheck-suppress stlFindInsert
                     lazyPreparedStatements[rowCountQuery] = std::unique_ptr<sql::PreparedStatement>(state.connection->prepareStatement(rowCountQuery));
                 }
 
@@ -618,7 +626,7 @@ namespace db
                 auto rset2      = std::unique_ptr<sql::ResultSet>(countStmt->executeQuery());
                 if (!rset2 || !rset2->next())
                 {
-                    ShowError("Failed to get row count");
+                    ShowErrorFmt("Failed to get row count");
                     return { nullptr, 0 };
                 }
                 auto rowCount = rset2->getUInt("count");
@@ -641,8 +649,8 @@ namespace db
                 {
                     if (!detail::isConnectionIssue(e))
                     {
-                        ShowError("Query Failed: %s", rawQuery.c_str());
-                        ShowError(e.what());
+                        ShowErrorFmt("Query Failed: {}", rawQuery.c_str());
+                        ShowErrorFmt("{}", e.what());
                         return { nullptr, 0 };
                     }
                 }
@@ -705,11 +713,14 @@ namespace db
     auto getDriverVersion() -> std::string;
 
     void checkCharset();
+    void checkTriggers();
 
     bool setAutoCommit(bool value);
     bool getAutoCommit();
 
     bool transactionStart();
     bool transactionCommit();
     bool transactionRollback();
+
+    void enableTimers();
 } // namespace db

src/common/logging.h

@@ -64,15 +64,60 @@ namespace logging
 
 // clang-format off
 
-// Helper to allow pointers to numeric types to be formatted
-// as strings.
-// TODO: All need for this should eventually be removed!
-// TODO: Any place we use this it is indicating some smelly and unsafe code.
-//     : When replacing, the surrounding code should be audited!
 template <typename T>
-std::string str(T v)
+std::string asUntrustedString(const T* ptr, std::size_t lengthBeforeNullTerm)
 {
-    return std::string(reinterpret_cast<const char*>(v));
+    if (!ptr)
+    {
+        return "NULL_PTR";
+    }
+
+    std::vector<char> buffer(lengthBeforeNullTerm + 1, '\0');
+    std::memcpy(buffer.data(), ptr, lengthBeforeNullTerm);
+
+    std::string_view view(buffer.data(), lengthBeforeNullTerm);
+    auto nullPos = view.find('\0');
+    if (nullPos != std::string_view::npos)
+    {
+        view = view.substr(0, nullPos);
+    }
+
+    std::string extractedString(view);
+    for (const char c : extractedString)
+    {
+        if (!std::isprint(c))
+        {
+            return "INVALID_STRING";
+        }
+    }
+
+    return extractedString;
+}
+
+template <typename T>
+std::string asUntrustedCharacterName(const T* ptr)
+{
+    static constexpr size_t MaxNameLength = 15;
+
+    const auto untrustedString = asUntrustedString(ptr, MaxNameLength);
+
+    if (untrustedString.size() < 3)
+    {
+        return "NAME_TOO_SHORT";
+    }
+
+    return untrustedString;
+}
+
+template <typename T>
+std::string asTrustedString(const T* ptr)
+{
+    if (!ptr)
+    {
+        return {};
+    }
+
+    return std::string(reinterpret_cast<const char*>(ptr));
 }
 
 // Helper for allowing `enum` and `enum class` types to be formatted

src/common/socket.cpp

@@ -321,7 +321,7 @@ std::string ip2str(uint32 ip)
     uint32 reversed_ip = htonl(ip);
     char   address[INET_ADDRSTRLEN];
     inet_ntop(AF_INET, &reversed_ip, address, INET_ADDRSTRLEN);
-    return fmt::format("{}", str(address));
+    return fmt::format("{}", asTrustedString(address));
 }
 
 uint32 str2ip(const char* ip_str)

src/common/sql.cpp

@@ -294,7 +294,7 @@ std::string SqlConnection::EscapeString(std::string const& input)
     std::string escaped_full_string;
     escaped_full_string.reserve(input.size() * 2 + 1);
     EscapeString(escaped_full_string.data(), input.data());
-    return escaped_full_string;
+    return std::string(escaped_full_string.data()); // Rebuild so the size etc. is correct
 }
 
 int32 SqlConnection::QueryStr(const char* query)

src/common/xi.h

@@ -37,18 +37,29 @@
 
 namespace xi
 {
-
     // A wrapper around std::optional to allow usage of object.apply([](auto& obj) { ... });
     // https://en.cppreference.com/w/cpp/utility/optional
     template <typename T>
     class optional
     {
     public:
         constexpr optional() = default;
-        constexpr explicit optional(std::nullopt_t) noexcept
+
+        constexpr optional(std::nullopt_t) noexcept
         : m_value(std::nullopt)
         {
         }
+
+        constexpr optional(T&& value)
+        : m_value(std::forward<T>(value))
+        {
+        }
+
+        constexpr optional(const T& value)
+        : m_value(value)
+        {
+        }
+
         constexpr optional(const optional& other)                = default;
         constexpr optional(optional&& other) noexcept            = default;
         constexpr optional& operator=(const optional& other)     = default;
@@ -68,21 +79,23 @@ namespace xi
         }
 
         template <typename F>
-        constexpr void apply(F&& f) &
+        constexpr bool apply(F&& f) &
         {
             if (m_value)
             {
                 f(*m_value);
             }
+            return m_value.has_value();
         }
 
         template <typename F>
-        constexpr void apply(F&& f) const&
+        constexpr bool apply(F&& f) const&
         {
             if (m_value)
             {
                 f(*m_value);
             }
+            return m_value.has_value();
         }
 
         constexpr explicit operator bool() const noexcept
@@ -132,7 +145,7 @@ namespace xi
         }
 
     private:
-        std::optional<T> m_value;
+        std::optional<T> m_value = std::nullopt;
     };
 
     // TODO: A wrapper around std::variant to allow usage of: