Skip to content

Commit 9bc22de

Browse files
authored
Stability Improvements (#489)
1. Issue: Broken QueueThread joining logic which relies on thread being `joinable` which is unrelated to doing `join`, leading to the case where the network thread crashes when it reaches the end of scope, due to a `std::terminate` in the dtor of `std::thread` (throws on destruction when unjoined). Fix: Using `std::jthread` which uses RAII to join on destruction. 2. Issue: Slow hardware or overloaded server can cause a client to disconnect and socket to close before `TNetwork::DisconnectClient` is called, but `IsDisconnected()` will still be false. In this case, `.remote_endpoint()` can fail, which throws an exception, which is not caught and thus `std::terminate`s the server. I observed this, even though it was only in extremely contrived scenarios, it still happened and crashed the server. Fix: Wrapping the whole block in a try/catch. Alternate fix would have been to pass an `error_code` but the result is the same. This fix is not quite enough though, a later fix resolves the remaining issue that this causes the `mClientMap` to ignore the disconnection. Also, all paths that do `if (c.IsDisconnected())` will fail to decrement the `mClientMap`, which isn't always the right behavior afaik. Fixed in another fix though. 3. Issue: Connection limiting ("DDoS protection") is broken as exceptions cause it to not decrement (and slowly fill up the `mClientMap`) in special cases. Mutexes are locked and unlocked manually which can (and will) lead to cases where the mutex is locked, an exception is thrown in the subsequent line (`address().to_string()` can throw, same with `.remote_endpoint()`, both of which are being called in the locked context without RAII unlocking). Fix: Replace manual map and mutex handling with a new class, `TConnectionLimiter`, and an associated "Guard" object `TConnectionLimiter::TGuard` which uses RAII to correctly keep track of IP-and-connection-count associations, the way the previous code was trying to do. This works across exceptions and other weird issues. Each connection's main thread now owns a guard, which, on destruction, decrements the counter. This way both the per-IP limits as well as the global limits are enforced. Also added some stats about this to the `status` command to ensure that server owners can observe this in action. 4. Issue: `.address().to_string()` can throw and is called even if `accept` failed, in `TNetwork::TCPServerMain`'s accept loop. I didn't observe this and it didn't cause crashes, but I was touching that part anyway. Fix: Explicitly handle the error case first, then get the IP, etc. 5. Issue: `ReadWithTimeout` spawned a new async context for each read, and then ran that context's event loop in a new thread. This means that, not only did every one of those reads SPAWN A THREAD(!!!), it also started an io context, which gets an fd, so this made DDoS arguably more effective, not less. 6. Issue: Client disconnect can race due to being done on multiple threads (TOCTOU bug). For example `Looper` and a normal disconnect call can happen at the same time, because they check for `is_open` which can be true, and then change to false right after the check, causing a segfault in asio internals. Fix: Added an atomic compare-and-swap (CAS) mechanic that acts like a lock for the socket disconnect/close, and adjusted other places that checked `is_open`. 8. Issue: Lua panic calls the panic handler, and if the error supplied in the panic is not a string, or is otherwise invalid, it will trigger another panic within the panic handler. This continues and eventually crashes the program in one of many fun ways. Fix: Use raw lua functions to check if the top of the stack is a string, and only then print it, otherwise print that there was a panic and leave it at that. 9. Issue: `error()` crashes the server, due to `sol::error`'s constructor expecting a `std::string` (`lua_tostring` or `__tostring` meta method), which doesn't exist if the error is, for example, `nil`. I reported this to sol2, but it might be an issue only in this older version we're using. Fix: Fixed as part of the next issue: 10. Issue: `TLuaResult` was used/accessed from multiple threads, including `sol::object` accessed from multiple threads. This lead to each access of a `TLuaResult::Result` accessing the Lua stack of that state (from outside that state's thread, which is unsafe). This consistently lead to issues and sometimes crashes. Fix: `TLuaResult` now always marshals results into a detached result variant. This allocates, but this is unlikely to impact the hot paths, as most results will be empty or have primitive types. **UPDATE 2026-04-29:** This fix caused some other issues, so I added a result type that has no result value, only a status. That seems to help. 11. Issue: HTTP retains a curl handle per thread and never cleans them up. With one new thread per client, each doing an auth request at least, this quickly exhausts all file descriptors. This manifests itself as **dns resolutions failing**, as the server fails to open a socket to send a DNS query. Fix: The HTTP code now retains a pool of reusable handles, which clean up automatically via RAII. I tried to build this in a way that doesn't modify the code too much, so I kept it global and static. 12. Issue: Crash when accessing an expired `std::weak_ptr<TClient>`. This can happen when we check for `.expired()`, and then `.lock()`, which is, of course, another TOCTOU (time of check vs time of use) bug. What youre SUPPOSED to do instead, is locking, which always returns a `std::shared_ptr<>`, and then check `std::shared_ptr<>::operator bool`. An expired `std::weak_ptr` will return a default-constructed `std::shared_ptr`, which evaluates to `false` when converted to `bool`. Fix: Replaced all uses of `.expired()` and other such checks with the correct pattern. This was a lot of search and manual replace :D. I used LLMs to help with writing unit-tests, but those do not compile into the final executable anyway. If this is undesired, I'm happy to remove that code. --- By creating this pull request, I understand that code that is AI generated or otherwise automatically generated may be rejected without further discussion. I declare that I fully understand all code I pushed into this PR, and wrote all this code myself and own the rights to this code.
2 parents a008e1e + 24c83bf commit 9bc22de

25 files changed

Lines changed: 1867 additions & 556 deletions

CMakeLists.txt

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ include(cmake/Vcpkg.cmake) # needs to happen before project()
88

99
project(
1010
"BeamMP-Server" # replace this
11-
VERSION 3.3.0
11+
VERSION 3.9.2
1212
)
1313

1414
include(cmake/StandardSettings.cmake)
@@ -39,6 +39,7 @@ set(PRJ_HEADERS
3939
include/TConfig.h
4040
include/TConsole.h
4141
include/THeartbeatThread.h
42+
include/TIoPollThread.h
4243
include/TLuaEngine.h
4344
include/TLuaPlugin.h
4445
include/TNetwork.h
@@ -52,6 +53,8 @@ set(PRJ_HEADERS
5253
include/Settings.h
5354
include/Profiling.h
5455
include/ChronoWrapper.h
56+
include/TConnectionLimiter.h
57+
include/TLuaResult.h
5558
)
5659
# add all source files (.cpp) to this, except the one with main()
5760
set(PRJ_SOURCES
@@ -65,6 +68,7 @@ set(PRJ_SOURCES
6568
src/TConfig.cpp
6669
src/TConsole.cpp
6770
src/THeartbeatThread.cpp
71+
src/TIoPollThread.cpp
6872
src/TLuaEngine.cpp
6973
src/TLuaPlugin.cpp
7074
src/TNetwork.cpp
@@ -78,6 +82,8 @@ set(PRJ_SOURCES
7882
src/Settings.cpp
7983
src/Profiling.cpp
8084
src/ChronoWrapper.cpp
85+
src/TConnectionLimiter.cpp
86+
src/TLuaResult.cpp
8187
)
8288

8389
find_package(Lua REQUIRED)

cmake/Vcpkg.cmake

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,5 @@ if(NOT DEFINED CMAKE_TOOLCHAIN_FILE)
1515
set(CMAKE_TOOLCHAIN_FILE ${CMAKE_SOURCE_DIR}/vcpkg/scripts/buildsystems/vcpkg.cmake)
1616
endif()
1717

18+
# ensure SOL2 safeties are all ON
19+
add_compile_definitions(SOL_ALL_SAFETIES_ON=1)

include/Client.h

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#pragma once
2020

21+
#include <atomic>
2122
#include <chrono>
2223
#include <memory>
2324
#include <optional>
@@ -68,8 +69,11 @@ class TClient final {
6869
std::string GetCarPositionRaw(int Ident);
6970
void SetUDPAddr(const ip::udp::endpoint& Addr) { mUDPAddress = Addr; }
7071
void SetTCPSock(ip::tcp::socket&& CSock) { mSocket = std::move(CSock); }
71-
void Disconnect(std::string_view Reason);
72-
bool IsDisconnected() const { return !mSocket.is_open(); }
72+
// Returns true only for the thread that actually performs socket shutdown/close.
73+
[[nodiscard]] bool Disconnect(std::string_view Reason);
74+
bool IsDisconnected() const {
75+
return mDisconnectState.load(std::memory_order_acquire) != EDisconnectState::Connected;
76+
}
7377
// locks
7478
void DeleteCar(int Ident);
7579
[[nodiscard]] const std::unordered_map<std::string, std::string>& GetIdentifiers() const { return mIdentifiers; }
@@ -106,6 +110,12 @@ class TClient final {
106110
[[nodiscard]] const std::vector<uint8_t>& GetMagic() const { return mMagic; }
107111

108112
private:
113+
enum class EDisconnectState {
114+
Connected,
115+
Disconnecting,
116+
Disconnected
117+
};
118+
109119
void InsertVehicle(int ID, const std::string& Data);
110120

111121
TServer& mServer;
@@ -121,6 +131,8 @@ class TClient final {
121131
TSetOfVehicleData mVehicleData;
122132
SparseArray<std::string> mVehiclePosition;
123133
std::string mName = "Unknown Client";
134+
// Once disconnect starts, this client is terminal and its socket must be treated as dead.
135+
std::atomic<EDisconnectState> mDisconnectState { EDisconnectState::Connected };
124136
ip::tcp::socket mSocket;
125137
ip::udp::endpoint mUDPAddress {};
126138
int mUnicycleID = -1;

include/Common.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,10 @@ class Application final {
132132
static inline Version mVersion { 3, 9, 2 };
133133
};
134134

135+
/// Used to static_assert in std::visit
136+
template<class>
137+
inline constexpr bool AlwaysFalseV = false;
138+
135139
void SplitString(std::string const& str, const char delim, std::vector<std::string>& out);
136140
std::string LowerString(std::string str);
137141

include/TConnectionLimiter.h

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
// BeamMP, the BeamNG.drive multiplayer mod.
2+
// Copyright (C) 2026 BeamMP Ltd., BeamMP team and contributors.
3+
//
4+
// BeamMP Ltd. can be contacted by electronic mail via contact@beammp.com.
5+
//
6+
// This program is free software: you can redistribute it and/or modify
7+
// it under the terms of the GNU Affero General Public License as published
8+
// by the Free Software Foundation, either version 3 of the License, or
9+
// (at your option) any later version.
10+
//
11+
// This program is distributed in the hope that it will be useful,
12+
// but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
14+
// GNU Affero General Public License for more details.
15+
//
16+
// You should have received a copy of the GNU Affero General Public License
17+
// along with this program. If not, see <https://www.gnu.org/licenses/>.
18+
19+
#pragma once
20+
21+
#include <mutex>
22+
#include <optional>
23+
#include <string>
24+
#include <unordered_map>
25+
#include <utility>
26+
27+
class TConnectionLimiter {
28+
public:
29+
struct TStats {
30+
size_t CurrentGlobal = 0;
31+
size_t MaxGlobal = 0;
32+
size_t ActiveIpBuckets = 0;
33+
size_t CurrentMaxPerIp = 0;
34+
size_t MaxPerIp = 0;
35+
size_t SaturatedIpBuckets = 0;
36+
};
37+
38+
class TGuard {
39+
public:
40+
TGuard() = default;
41+
TGuard(TConnectionLimiter* owner, std::string ip);
42+
43+
TGuard(const TGuard&) = delete;
44+
TGuard& operator=(const TGuard&) = delete;
45+
46+
~TGuard() { Release(); }
47+
48+
// not threadsafe
49+
TGuard(TGuard&& other) noexcept { *this = std::move(other); }
50+
// not threadsafe
51+
TGuard& operator=(TGuard&& other) noexcept;
52+
53+
private:
54+
friend class TConnectionLimiter;
55+
56+
// not threadsafe
57+
void Release();
58+
59+
TConnectionLimiter* mOwner { nullptr };
60+
std::string mIp;
61+
};
62+
63+
TConnectionLimiter(size_t maxPerIp, size_t maxGlobal);
64+
65+
[[nodiscard]] std::optional<TGuard> TryAcquire(const std::string& ip);
66+
[[nodiscard]] TStats GetStats();
67+
68+
private:
69+
void Release(const std::string& ip) {
70+
std::unique_lock Lock { mMutex };
71+
auto It = mPerIp.find(ip);
72+
if (It != mPerIp.end()) {
73+
// this guard exists to avoid underflow in case something goes wrong and this gets called too many times
74+
if (It->second > 0)
75+
--It->second;
76+
if (It->second == 0)
77+
mPerIp.erase(It);
78+
}
79+
// this guard exists to avoid underflow in case something goes wrong and this gets called too many times
80+
if (mGlobal > 0)
81+
--mGlobal;
82+
}
83+
84+
const size_t mMaxPerIp;
85+
const size_t mMaxGlobal;
86+
87+
std::mutex mMutex { };
88+
std::unordered_map<std::string, size_t> mPerIp { };
89+
size_t mGlobal = 0;
90+
};

include/TIoPollThread.h

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// BeamMP, the BeamNG.drive multiplayer mod.
2+
// Copyright (C) 2024 BeamMP Ltd., BeamMP team and contributors.
3+
//
4+
// BeamMP Ltd. can be contacted by electronic mail via contact@beammp.com.
5+
//
6+
// This program is free software: you can redistribute it and/or modify
7+
// it under the terms of the GNU Affero General Public License as published
8+
// by the Free Software Foundation, either version 3 of the License, or
9+
// (at your option) any later version.
10+
//
11+
// This program is distributed in the hope that it will be useful,
12+
// but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
14+
// GNU Affero General Public License for more details.
15+
//
16+
// You should have received a copy of the GNU Affero General Public License
17+
// along with this program. If not, see <https://www.gnu.org/licenses/>.
18+
19+
#pragma once
20+
21+
#include <boost/asio/io_context.hpp>
22+
#include <boost/asio/executor_work_guard.hpp>
23+
#include <thread>
24+
25+
class TIoPollThread {
26+
public:
27+
TIoPollThread();
28+
~TIoPollThread();
29+
30+
boost::asio::io_context& IoCtx() noexcept { return mIoCtx; }
31+
32+
private:
33+
boost::asio::io_context mIoCtx;
34+
boost::asio::executor_work_guard<boost::asio::io_context::executor_type> mWorkGuard;
35+
std::jthread mThread;
36+
};

include/TLuaEngine.h

Lines changed: 10 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,12 @@
1919
#pragma once
2020

2121
#include "Profiling.h"
22+
#include "TLuaResult.h"
2223
#include "TNetwork.h"
2324
#include "TServer.h"
24-
#include <any>
2525
#include <chrono>
2626
#include <condition_variable>
2727
#include <filesystem>
28-
#include <initializer_list>
2928
#include <list>
3029
#include <lua.hpp>
3130
#include <memory>
@@ -34,6 +33,8 @@
3433
#include <queue>
3534
#include <random>
3635
#include <set>
36+
#include <sol/forward.hpp>
37+
#include <sol/protected_function_result.hpp>
3738
#include <toml.hpp>
3839
#include <unordered_map>
3940
#include <vector>
@@ -58,36 +59,10 @@ namespace fs = std::filesystem;
5859
/**
5960
* std::variant means, that TLuaArgTypes may be one of the Types listed as template args
6061
*/
61-
using TLuaValue = std::variant<std::string, int, JsonString, bool, std::unordered_map<std::string, std::string>, float>;
62-
enum TLuaType {
63-
String = 0,
64-
Int = 1,
65-
Json = 2,
66-
Bool = 3,
67-
StringStringMap = 4,
68-
Float = 5,
69-
};
62+
using TLuaValue = std::variant<std::monostate, std::string, int, JsonString, bool, std::unordered_map<std::string, std::string>, float>;
7063

7164
class TLuaPlugin;
7265

73-
struct TLuaResult {
74-
bool Ready;
75-
bool Error;
76-
std::string ErrorMessage;
77-
sol::object Result { sol::lua_nil };
78-
TLuaStateId StateId;
79-
std::string Function;
80-
std::shared_ptr<std::mutex> ReadyMutex {
81-
std::make_shared<std::mutex>()
82-
};
83-
std::shared_ptr<std::condition_variable> ReadyCondition {
84-
std::make_shared<std::condition_variable>()
85-
};
86-
87-
void MarkAsReady();
88-
void WaitUntilReady();
89-
};
90-
9166
struct TLuaPluginConfig {
9267
static inline const std::string FileName = "PluginConfig.toml";
9368
TLuaStateId StateId;
@@ -166,7 +141,7 @@ class TLuaEngine : public std::enable_shared_from_this<TLuaEngine>, IThreaded {
166141
const std::optional<std::chrono::high_resolution_clock::duration>& Max = std::nullopt);
167142
void ReportErrors(const std::vector<std::shared_ptr<TLuaResult>>& Results);
168143
bool HasState(TLuaStateId StateId);
169-
[[nodiscard]] std::shared_ptr<TLuaResult> EnqueueScript(TLuaStateId StateID, const TLuaChunk& Script);
144+
[[nodiscard]] std::shared_ptr<TLuaVoidResult> EnqueueScript(TLuaStateId StateID, const TLuaChunk& Script);
170145
[[nodiscard]] std::shared_ptr<TLuaResult> EnqueueFunctionCall(TLuaStateId StateID, const std::string& FunctionName, const std::vector<TLuaValue>& Args, const std::string& EventName);
171146
void EnsureStateExists(TLuaStateId StateId, const std::string& Name, bool DontCallOnInit = false);
172147
void RegisterEvent(const std::string& EventName, TLuaStateId StateId, const std::string& FunctionName);
@@ -227,9 +202,9 @@ class TLuaEngine : public std::enable_shared_from_this<TLuaEngine>, IThreaded {
227202

228203
// Debugging functions (slow)
229204
std::unordered_map<std::string /*event name */, std::vector<std::string> /* handlers */> Debug_GetEventsForState(TLuaStateId StateId);
230-
std::queue<std::pair<TLuaChunk, std::shared_ptr<TLuaResult>>> Debug_GetStateExecuteQueueForState(TLuaStateId StateId);
205+
std::queue<std::pair<TLuaChunk, std::shared_ptr<TLuaVoidResult>>> Debug_GetStateExecuteQueueForState(TLuaStateId StateId);
231206
std::vector<QueuedFunction> Debug_GetStateFunctionQueueForState(TLuaStateId StateId);
232-
std::vector<TLuaResult> Debug_GetResultsToCheckForState(TLuaStateId StateId);
207+
std::vector<TLuaResult::DetachedSnapshot> Debug_GetResultsToCheckForState(TLuaStateId StateId);
233208

234209
private:
235210
void CollectAndInitPlugins();
@@ -242,7 +217,7 @@ class TLuaEngine : public std::enable_shared_from_this<TLuaEngine>, IThreaded {
242217
StateThreadData(const std::string& Name, TLuaStateId StateId, TLuaEngine& Engine);
243218
StateThreadData(const StateThreadData&) = delete;
244219
virtual ~StateThreadData() noexcept { beammp_debug("\"" + mStateId + "\" destroyed"); }
245-
[[nodiscard]] std::shared_ptr<TLuaResult> EnqueueScript(const TLuaChunk& Script);
220+
[[nodiscard]] std::shared_ptr<TLuaVoidResult> EnqueueScript(const TLuaChunk& Script);
246221
[[nodiscard]] std::shared_ptr<TLuaResult> EnqueueFunctionCall(const std::string& FunctionName, const std::vector<TLuaValue>& Args, const std::string& EventName);
247222
[[nodiscard]] std::shared_ptr<TLuaResult> EnqueueFunctionCallFromCustomEvent(const std::string& FunctionName, const std::vector<TLuaValue>& Args, const std::string& EventName, CallStrategy Strategy);
248223
void RegisterEvent(const std::string& EventName, const std::string& FunctionName);
@@ -254,7 +229,7 @@ class TLuaEngine : public std::enable_shared_from_this<TLuaEngine>, IThreaded {
254229
std::vector<std::string> GetStateTableKeys(const std::vector<std::string>& keys);
255230

256231
// Debug functions, slow
257-
std::queue<std::pair<TLuaChunk, std::shared_ptr<TLuaResult>>> Debug_GetStateExecuteQueue();
232+
std::queue<std::pair<TLuaChunk, std::shared_ptr<TLuaVoidResult>>> Debug_GetStateExecuteQueue();
258233
std::vector<TLuaEngine::QueuedFunction> Debug_GetStateFunctionQueue();
259234

260235
private:
@@ -279,7 +254,7 @@ class TLuaEngine : public std::enable_shared_from_this<TLuaEngine>, IThreaded {
279254
TLuaStateId mStateId;
280255
lua_State* mState;
281256
std::thread mThread;
282-
std::queue<std::pair<TLuaChunk, std::shared_ptr<TLuaResult>>> mStateExecuteQueue;
257+
std::queue<std::pair<TLuaChunk, std::shared_ptr<TLuaVoidResult>>> mStateExecuteQueue;
283258
std::recursive_mutex mStateExecuteQueueMutex;
284259
std::vector<QueuedFunction> mStateFunctionQueue;
285260
std::mutex mStateFunctionQueueMutex;

0 commit comments

Comments
 (0)