merge release candidate v3.1.1 (#142)

patches and hotfixes!

CMakeLists.txt

@@ -28,6 +28,8 @@ set(SENTRY_BUILD_SHARED_LIBS OFF)
 
 add_compile_definitions(CPPHTTPLIB_OPENSSL_SUPPORT=1)
 
+option(WIN32_STATIC_RUNTIME "Build statically-linked runtime on windows (don't touch unless you know what you're doing)" ON)
+
 # ------------------------ APPLE ---------------------------------
 if(APPLE)
     if(IS_DIRECTORY /opt/homebrew/Cellar/lua@5.3/5.3.6)
@@ -46,18 +48,14 @@ if(APPLE)
         link_directories(/usr/local/opt/openssl@1.1/lib)
     endif()
 # ------------------------ WINDOWS ---------------------------------
-option(WIN32_STATIC_RUNTIME "Build statically-linked runtime on windows (don't touch unless you know what you're doing)" ON)
 elseif (WIN32)
-    # this has to happen before sentry, so that crashpad on windows links with these settings.
     if (WIN32_STATIC_RUNTIME)
-        message(STATUS "MSVC -> forcing use of statically-linked runtime.")
+        set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
-        STRING(REPLACE "/MD" "/MT" CMAKE_CXX_FLAGS_RELEASE ${CMAKE_CXX_FLAGS_RELEASE})
-        STRING(REPLACE "/MDd" "/MTd" CMAKE_CXX_FLAGS_DEBUG ${CMAKE_CXX_FLAGS_DEBUG})
     endif()
-# ------------------------ LINUX ---------------------------------
+# ------------------------ UNIX ------------------------------------
 elseif (UNIX)
     set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0 -g")
-    set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -O2 -fno-builtin")
+    set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -O2")
     option(SANITIZE "Turns on thread and UB sanitizers" OFF)
     if (SANITIZE)
         message(STATUS "sanitize is ON")
@@ -82,9 +80,6 @@ add_subdirectory("deps/sentry-native")
 
 # ------------------------ C++ SETUP ---------------------------------
 set(CMAKE_CXX_STANDARD 17)
-if (MSVC)
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj")
-endif ()
 
 # ------------------------ DEPENDENCIES ------------------------------
 message(STATUS "Adding local source dependencies")
@@ -177,6 +172,12 @@ if (UNIX)
         -fstack-protector
         -Wzero-as-null-pointer-constant
     )
+else()
+
+    set(BeamMP_CompileOptions
+        /bigobj
+        /INCREMENTAL:NO /NODEFAULTLIB:MSVCRT /NODEFAULTLIB:LIBCMT
+    )
 endif()
 
 set(BeamMP_Libraries

Changelog.md

@@ -1,3 +1,10 @@
+# v3.1.1
+
+- FIXED bug which caused GetPlayerIdentifiers, GetPlayerName, etc not to work in `onPlayerDisconnect`
+- FIXED some issues which could cause the server to crash when receiving malformed data
+- FIXED a bug which caused a server to crash during authentication when receiving malformed data
+- FIXED minor vulnerability in chat message handling
+- FIXED a minor formatting bug in the `status` command
 
 # v3.1.0
 

include/Common.h

@@ -137,7 +137,7 @@ private:
     static inline std::mutex mShutdownHandlersMutex {};
     static inline std::deque<TShutdownHandler> mShutdownHandlers {};
 
-    static inline Version mVersion { 3, 1, 0 };
+    static inline Version mVersion { 3, 1, 1 };
 };
 
 std::string ThreadName(bool DebugModeOverride = false);

include/TLuaEngine.h

@@ -76,7 +76,7 @@ public:
     };
 
     TLuaEngine();
-    ~TLuaEngine() noexcept {
+    virtual ~TLuaEngine() noexcept {
         beammp_debug("Lua Engine terminated");
     }
 
@@ -198,7 +198,7 @@ private:
     public:
         StateThreadData(const std::string& Name, TLuaStateId StateId, TLuaEngine& Engine);
         StateThreadData(const StateThreadData&) = delete;
-        ~StateThreadData() noexcept { beammp_debug("\"" + mStateId + "\" destroyed"); }
+        virtual ~StateThreadData() noexcept { beammp_debug("\"" + mStateId + "\" destroyed"); }
         [[nodiscard]] std::shared_ptr<TLuaResult> EnqueueScript(const TLuaChunk& Script);
         [[nodiscard]] std::shared_ptr<TLuaResult> EnqueueFunctionCall(const std::string& FunctionName, const std::vector<TLuaArgTypes>& Args);
         [[nodiscard]] std::shared_ptr<TLuaResult> EnqueueFunctionCallFromCustomEvent(const std::string& FunctionName, const std::vector<TLuaArgTypes>& Args, const std::string& EventName, CallStrategy Strategy);

src/Client.cpp

@@ -6,6 +6,7 @@
 #include <optional>
 
 void TClient::DeleteCar(int Ident) {
+    // TODO: Send delete packets
     std::unique_lock lock(mVehicleDataMutex);
     auto iter = std::find_if(mVehicleData.begin(), mVehicleData.end(), [&](auto& elem) {
         return Ident == elem.ID();

src/TConsole.cpp

@@ -198,10 +198,10 @@ bool TConsole::EnsureArgsCount(const std::vector<std::string>& args, size_t min,
         return EnsureArgsCount(args, min);
     } else {
         if (args.size() > max) {
-            Application::Console().WriteRaw("Too many arguments. At most " + std::to_string(max) + " arguments expected, got " + std::to_string(args.size()) + " instead.");
+            Application::Console().WriteRaw("Too many arguments. At most " + std::to_string(max) + " argument(s) expected, got " + std::to_string(args.size()) + " instead.");
             return false;
         } else if (args.size() < min) {
-            Application::Console().WriteRaw("Too few arguments. At least " + std::to_string(max) + " arguments expected, got " + std::to_string(args.size()) + " instead.");
+            Application::Console().WriteRaw("Too few arguments. At least " + std::to_string(min) + " argument(s) expected, got " + std::to_string(args.size()) + " instead.");
             return false;
         }
     }
@@ -350,9 +350,10 @@ std::tuple<std::string, std::vector<std::string>> TConsole::ParseCommand(const s
 }
 
 void TConsole::Command_Settings(const std::string&, const std::vector<std::string>& args) {
-    if (!EnsureArgsCount(args, 0)) {
+    if (!EnsureArgsCount(args, 1, 2)) {
         return;
     }
+    
 }
 
 void TConsole::Command_Say(const std::string& FullCmd) {

src/TNetwork.cpp

@@ -393,6 +393,12 @@ std::vector<uint8_t> TNetwork::TCPRcv(TClient& c) {
     }
     Header = *reinterpret_cast<int32_t*>(HeaderData.data());
 
+    if (Header < 0) {
+        ClientKick(c, "Invalid packet - header negative");
+        beammp_errorf("Client {} send negative TCP header, ignoring packet", c.GetID());
+        return {};
+    }
+
     std::vector<uint8_t> Data;
     // TODO: This is arbitrary, this needs to be handled another way
     if (Header < int32_t(100 * MB)) {
@@ -425,7 +431,7 @@ std::vector<uint8_t> TNetwork::TCPRcv(TClient& c) {
 void TNetwork::ClientKick(TClient& c, const std::string& R) {
     beammp_info("Client kicked: " + R);
     if (!TCPSend(c, StringToVector("K" + R))) {
-        beammp_debugf("tried to kick player '{}' (id {}), but was already connected", c.GetName(), c.GetID());
+        beammp_debugf("tried to kick player '{}' (id {}), but was already disconnected", c.GetName(), c.GetID());
     }
     c.Disconnect("Kicked");
 }
@@ -522,8 +528,14 @@ void TNetwork::UpdatePlayer(TClient& Client) {
 }
 
 void TNetwork::OnDisconnect(const std::weak_ptr<TClient>& ClientPtr) {
-    beammp_assert(!ClientPtr.expired());
+    std::shared_ptr<TClient> LockedClientPtr { nullptr };
-    auto LockedClientPtr = ClientPtr.lock();
+    try {
+        LockedClientPtr = ClientPtr.lock();
+    } catch (const std::exception&) {
+        beammp_warn("Client expired in OnDisconnect, this is unexpected");
+        return;
+    }
+    beammp_assert(LockedClientPtr != nullptr);
     TClient& c = *LockedClientPtr;
     beammp_info(c.GetName() + (" Connection Terminated"));
     std::string Packet;
@@ -540,7 +552,7 @@ void TNetwork::OnDisconnect(const std::weak_ptr<TClient>& ClientPtr) {
     SendToAll(&c, StringToVector(Packet), false, true);
     Packet.clear();
     auto Futures = LuaAPI::MP::Engine->TriggerEvent("onPlayerDisconnect", "", c.GetID());
-    LuaAPI::MP::Engine->ReportErrors(Futures);
+    LuaAPI::MP::Engine->WaitForAll(Futures);
     c.Disconnect("Already Disconnected (OnDisconnect)");
     mServer.RemoveClient(ClientPtr);
 }
@@ -860,11 +872,12 @@ void TNetwork::SendToAll(TClient* c, const std::vector<uint8_t>& Data, bool Self
     bool ret = true;
     mServer.ForEachClient([&](std::weak_ptr<TClient> ClientPtr) -> bool {
         std::shared_ptr<TClient> Client;
-        {
+        try {
             ReadLock Lock(mServer.GetClientMutex());
-            if (!ClientPtr.expired()) {
             Client = ClientPtr.lock();
-            } else
+        } catch (const std::exception&) {
+            // continue
+            beammp_warn("Client expired, shouldn't happen - if a client disconnected recently, you can ignore this");
             return true;
         }
         if (Self || Client.get() != c) {

src/TServer.cpp

@@ -1,6 +1,7 @@
 #include "TServer.h"
 #include "Client.h"
 #include "Common.h"
+#include "CustomAssert.h"
 #include "TNetwork.h"
 #include "TPPSMonitor.h"
 #include <TLuaPlugin.h>
@@ -94,13 +95,20 @@ TServer::TServer(const std::vector<std::string_view>& Arguments) {
 }
 
 void TServer::RemoveClient(const std::weak_ptr<TClient>& WeakClientPtr) {
-    if (!WeakClientPtr.expired()) {
+    std::shared_ptr<TClient> LockedClientPtr { nullptr };
-        TClient& Client = *WeakClientPtr.lock();
+    try {
+        LockedClientPtr = WeakClientPtr.lock();
+    } catch (const std::exception&) {
+        // silently fail, as there's nothing to do
+        return;
+    }
+    beammp_assert(LockedClientPtr != nullptr);
+    TClient& Client = *LockedClientPtr;
     beammp_debug("removing client " + Client.GetName() + " (" + std::to_string(ClientCount()) + ")");
+    // TODO: Send delete packets for all cars
     Client.ClearCars();
     WriteLock Lock(mClientsMutex);
     mClients.erase(WeakClientPtr.lock());
-    }
 }
 
 void TServer::ForEachClient(const std::function<bool(std::weak_ptr<TClient>)>& Fn) {
@@ -167,14 +175,20 @@ void TServer::GlobalParser(const std::weak_ptr<TClient>& Client, std::vector<uin
         }
         ParseVehicle(*LockedClient, StringPacket, Network);
         return;
-    case 'J':
-        Network.SendToAll(LockedClient.get(), Packet, false, true);
-        return;
     case 'C': {
         if (Packet.size() < 4 || std::find(Packet.begin() + 3, Packet.end(), ':') == Packet.end())
             break;
         const auto PacketAsString = std::string(reinterpret_cast<const char*>(Packet.data()), Packet.size());
-        auto Futures = LuaAPI::MP::Engine->TriggerEvent("onChatMessage", "", LockedClient->GetID(), LockedClient->GetName(), PacketAsString.substr(PacketAsString.find(':', 3) + 2));
+        std::string Message = "";
+        const auto ColonPos = PacketAsString.find(':', 3);
+        if (ColonPos != std::string::npos && ColonPos + 2 < PacketAsString.size()) {
+            Message = PacketAsString.substr(ColonPos + 2);
+        }
+        if (Message.empty()) {
+            beammp_debugf("Empty chat message received from '{}' ({}), ignoring it", LockedClient->GetName(), LockedClient->GetID());
+            return;
+        }
+        auto Futures = LuaAPI::MP::Engine->TriggerEvent("onChatMessage", "", LockedClient->GetID(), LockedClient->GetName(), Message);
         TLuaEngine::WaitForAll(Futures);
         LogChatMessage(LockedClient->GetName(), LockedClient->GetID(), PacketAsString.substr(PacketAsString.find(':', 3) + 1));
         if (std::any_of(Futures.begin(), Futures.end(),
@@ -185,7 +199,8 @@ void TServer::GlobalParser(const std::weak_ptr<TClient>& Client, std::vector<uin
                 })) {
             break;
         }
-        Network.SendToAll(nullptr, Packet, true, true);
+        std::string SanitizedPacket = fmt::format("C:{}: {}", LockedClient->GetName(), Message);
+        Network.SendToAll(nullptr, StringToVector(SanitizedPacket), true, true);
         return;
     }
     case 'E':
@@ -198,7 +213,6 @@ void TServer::GlobalParser(const std::weak_ptr<TClient>& Client, std::vector<uin
     case 'Z': // position packet
         PPSMonitor.IncrementInternalPPS();
         Network.SendToAll(LockedClient.get(), Packet, false, false);
-
         HandlePosition(*LockedClient, StringPacket);
     default:
         return;
@@ -208,6 +222,10 @@ void TServer::GlobalParser(const std::weak_ptr<TClient>& Client, std::vector<uin
 void TServer::HandleEvent(TClient& c, const std::string& RawData) {
     // E:Name:Data
     // Data is allowed to have ':'
+    if (RawData.size() < 2) {
+        beammp_debugf("Client '{}' ({}) tried to send an empty event, ignoring", c.GetName(), c.GetID());
+        return;
+    }
     auto NameDataSep = RawData.find(':', 2);
     if (NameDataSep == std::string::npos) {
         beammp_warn("received event in invalid format (missing ':'), got: '" + RawData + "'");