fix: disable assert for not debug compilaton and add logs (opentibiab…

…r#2182)

• Added the NDEBUG macro to the processor definitions to disable assert
in non-debug compilations.
• Implemented logging and return statements to handle errors instead of
using assert. This is necessary to prevent abrupt interruptions in
production servers.
• Made several corrections and adjustments in the build solution,
including the use of AddressSanitizer (ASan) in debug builds.

Resolves opentibiabr#2156

Using assert is not advisable in production because when an assertion
fails, the program is immediately terminated, causing uncontrolled
disruptions on the server. Instead, it's preferable to use proper error
handling mechanisms, such as logging and exceptions, to deal with issues
in a controlled manner and maintain system stability in production.

CMakePresets.json

@@ -7,56 +7,29 @@
   },
   "configurePresets": [
     {
-      "name": "windows-release",
-      "displayName": "Windows - Release",
-      "description": "Sets Ninja generator, compilers, build and install directory and set build type as release",
+      "name": "base",
+      "hidden": true,
       "generator": "Ninja",
       "binaryDir": "${sourceDir}/build/${presetName}",
       "cacheVariables": {
-        "CMAKE_TOOLCHAIN_FILE": {
-          "value": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake",
-          "type": "FILEPATH"
-        },
+        "CMAKE_TOOLCHAIN_FILE": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake",
         "BUILD_STATIC_LIBRARY": "ON",
-        "VCPKG_TARGET_TRIPLET": "x64-windows-static",
-        "CMAKE_BUILD_TYPE": "RelWithDebInfo",
-        "OPTIONS_ENABLE_SCCACHE": "ON"
-      },
-      "architecture": {
-        "value": "x64",
-        "strategy": "external"
-      },
-      "vendor": {
-        "microsoft.com/VisualStudioSettings/CMake/1.0": {
-          "hostOS": ["Windows"]
-        }
-      },
-      "condition": {
-        "type": "equals",
-        "lhs": "${hostSystemName}",
-        "rhs": "Windows"
+        "SPEED_UP_BUILD_UNITY": "ON",
+        "OPTIONS_ENABLE_SCCACHE": "ON",
+        "CMAKE_BUILD_TYPE": "RelWithDebInfo"
       }
     },
     {
-      "name": "linux-release",
-      "displayName": "Linux - Release",
-      "description": "Sets Ninja generator, compilers, build and install directory and set build type as release",
-      "generator": "Ninja",
-      "binaryDir": "${sourceDir}/build/${presetName}",
+      "name": "windows-release",
+      "inherits": "base",
+      "displayName": "Windows - Release",
+      "description": "Windows Release Build",
       "cacheVariables": {
-        "CMAKE_TOOLCHAIN_FILE": {
-          "value": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake",
-          "type": "FILEPATH"
-        },
-        "BUILD_STATIC_LIBRARY": "ON",
-        "CMAKE_BUILD_TYPE": "RelWithDebInfo",
-        "OPTIONS_ENABLE_CCACHE": "ON",
-        "RUN_TESTS_AFTER_BUILD": "OFF"
+        "VCPKG_TARGET_TRIPLET": "x64-windows-static"
       },
-      "condition": {
-        "type": "equals",
-        "lhs": "${hostSystemName}",
-        "rhs": "Linux"
+      "architecture": {
+        "value": "x64",
+        "strategy": "external"
       }
     },
     {
@@ -69,10 +42,6 @@
         "DEBUG_LOG": "ON",
         "BUILD_STATIC_LIBRARY": "OFF",
         "VCPKG_TARGET_TRIPLET": "x64-windows"
-      },
-      "architecture": {
-        "value": "x64",
-        "strategy": "external"
       }
     },
     {
@@ -86,10 +55,24 @@
         "ASAN_ENABLED": "OFF",
         "BUILD_STATIC_LIBRARY": "OFF",
         "VCPKG_TARGET_TRIPLET": "x64-windows"
+      }
+    },
+    {
+      "name": "linux-release",
+      "inherits": "base",
+      "displayName": "Linux - Release",
+      "description": "Sets Ninja generator, compilers, build and install directory and set build type as release",
+      "cacheVariables": {
+        "CMAKE_TOOLCHAIN_FILE": {
+          "value": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake",
+          "type": "FILEPATH"
+        },
+        "RUN_TESTS_AFTER_BUILD": "OFF"
       },
-      "architecture": {
-        "value": "x64",
-        "strategy": "external"
+      "condition": {
+        "type": "equals",
+        "lhs": "${hostSystemName}",
+        "rhs": "Linux"
       }
     },
     {

cmake/modules/CanaryLib.cmake

@@ -28,7 +28,7 @@ if (NOT SPEED_UP_BUILD_UNITY)
 endif()
 
 if(NOT SPEED_UP_BUILD_UNITY AND USE_PRECOMPILED_HEADERS)
-    target_compile_definitions(${PROJECT_NAME}_lib PUBLIC -DUSE_PRECOMPILED_HEADER)
+    target_compile_definitions(${PROJECT_NAME}_lib PUBLIC -DUSE_PRECOMPILED_HEADERS)
 endif()
 
 # *****************************************************************************
@@ -38,6 +38,14 @@ if (CMAKE_COMPILER_IS_GNUCXX)
     target_compile_options(${PROJECT_NAME}_lib PRIVATE -Wno-deprecated-declarations)
 endif()
 
+# Sets the NDEBUG macro for RelWithDebInfo and Release configurations.
+# This disables assertions in these configurations, optimizing the code for performance
+# and reducing debugging overhead, while keeping debug information available for diagnostics.
+target_compile_definitions(${PROJECT_NAME}_lib PUBLIC
+    $<$<CONFIG:RelWithDebInfo>:NDEBUG>
+    $<$<CONFIG:Release>:NDEBUG>
+)
+
 # === IPO ===
 if(MSVC)
     target_compile_options(${PROJECT_NAME}_lib PRIVATE "/GL")

src/creatures/combat/combat.cpp

@@ -548,7 +548,12 @@ CallBack* Combat::getCallback(CallBackParam_t key) {
 }
 
 void Combat::CombatHealthFunc(std::shared_ptr<Creature> caster, std::shared_ptr<Creature> target, const CombatParams &params, CombatDamage* data) {
+	if (!data) {
+		g_logger().error("[{}]: CombatDamage is nullptr", __FUNCTION__);
+		return;
+	}
 	assert(data);
+
 	CombatDamage damage = *data;
 
 	std::shared_ptr<Player> attackerPlayer = nullptr;
@@ -656,6 +661,11 @@ CombatDamage Combat::applyImbuementElementalDamage(std::shared_ptr<Player> attac
 }
 
 void Combat::CombatManaFunc(std::shared_ptr<Creature> caster, std::shared_ptr<Creature> target, const CombatParams &params, CombatDamage* data) {
+	if (!data) {
+		g_logger().error("[{}]: CombatDamage is nullptr", __FUNCTION__);
+		return;
+	}
+
 	assert(data);
 	CombatDamage damage = *data;
 	if (damage.primary.value < 0) {

src/creatures/monsters/monster.cpp

@@ -285,7 +285,12 @@ void Monster::onCreatureSay(std::shared_ptr<Creature> creature, SpeakClasses typ
 }
 
 void Monster::addFriend(const std::shared_ptr<Creature> &creature) {
-	assert(creature.get() != this);
+	if (creature == getMonster()) {
+		g_logger().error("[{}]: adding creature is same of monster", __FUNCTION__);
+		return;
+	}
+
+	assert(creature != getMonster());
 	friendList.try_emplace(creature->getID(), creature);
 }
 
@@ -297,7 +302,12 @@ void Monster::removeFriend(const std::shared_ptr<Creature> &creature) {
 }
 
 bool Monster::addTarget(const std::shared_ptr<Creature> &creature, bool pushFront /* = false*/) {
-	assert(creature.get() != this);
+	if (creature == getMonster()) {
+		g_logger().error("[{}]: adding creature is same of monster", __FUNCTION__);
+		return false;
+	}
+
+	assert(creature != getMonster());
 
 	const auto &it = getTargetIterator(creature);
 	if (it != targetList.end()) {

src/creatures/players/player.cpp

@@ -4109,16 +4109,11 @@ void Player::postAddNotification(std::shared_ptr<Thing> thing, std::shared_ptr<C
 	}
 
 	bool requireListUpdate = true;
-
 	if (link == LINK_OWNER || link == LINK_TOPPARENT) {
 		std::shared_ptr<Item> i = (oldParent ? oldParent->getItem() : nullptr);
-
-		// Check if we owned the old container too, so we don't need to do anything,
-		// as the list was updated in postRemoveNotification
-		assert(i ? i->getContainer() != nullptr : true);
-
-		if (i) {
-			requireListUpdate = i->getContainer()->getHoldingPlayer() != getPlayer();
+		const auto &container = i ? i->getContainer() : nullptr;
+		if (container) {
+			requireListUpdate = container->getHoldingPlayer() != getPlayer();
 		} else {
 			requireListUpdate = oldParent != getPlayer();
 		}
@@ -4170,15 +4165,9 @@ void Player::postRemoveNotification(std::shared_ptr<Thing> thing, std::shared_pt
 
 	if (link == LINK_OWNER || link == LINK_TOPPARENT) {
 		std::shared_ptr<Item> i = (newParent ? newParent->getItem() : nullptr);
-
-		// Check if we owned the old container too, so we don't need to do anything,
-		// as the list was updated in postRemoveNotification
-		assert(i ? i->getContainer() != nullptr : true);
-
-		if (i) {
-			if (auto container = i->getContainer()) {
-				requireListUpdate = container->getHoldingPlayer() != getPlayer();
-			}
+		const auto &container = i ? i->getContainer() : nullptr;
+		if (container) {
+			requireListUpdate = container->getHoldingPlayer() != getPlayer();
 		} else {
 			requireListUpdate = newParent != getPlayer();
 		}

src/game/scheduling/task.cpp

@@ -8,11 +8,34 @@
  */
 
 #include "pch.hpp"
+
 #include "task.hpp"
+
 #include "lib/logging/log_with_spd_log.hpp"
 #include "lib/metrics/metrics.hpp"
+
 std::atomic_uint_fast64_t Task::LAST_EVENT_ID = 0;
 
+Task::Task(uint32_t expiresAfterMs, std::function<void(void)> &&f, std::string_view context) :
+	func(std::move(f)), context(context), utime(OTSYS_TIME()), expiration(expiresAfterMs > 0 ? OTSYS_TIME() + expiresAfterMs : 0) {
+	if (this->context.empty()) {
+		g_logger().error("[{}]: task context cannot be empty!", __FUNCTION__);
+		return;
+	}
+
+	assert(!this->context.empty() && "Context cannot be empty!");
+}
+
+Task::Task(std::function<void(void)> &&f, std::string_view context, uint32_t delay, bool cycle /* = false*/, bool log /*= true*/) :
+	func(std::move(f)), context(context), utime(OTSYS_TIME() + delay), delay(delay), cycle(cycle), log(log) {
+	if (this->context.empty()) {
+		g_logger().error("[{}]: task context cannot be empty!", __FUNCTION__);
+		return;
+	}
+
+	assert(!this->context.empty() && "Context cannot be empty!");
+}
+
 bool Task::execute() const {
 	metrics::task_latency measure(context);
 	if (isCanceled()) {

src/game/scheduling/task.hpp

@@ -13,15 +13,9 @@
 
 class Task {
 public:
-	Task(uint32_t expiresAfterMs, std::function<void(void)> &&f, std::string_view context) :
-		func(std::move(f)), context(context), utime(OTSYS_TIME()), expiration(expiresAfterMs > 0 ? OTSYS_TIME() + expiresAfterMs : 0) {
-		assert(!this->context.empty() && "Context cannot be empty!");
-	}
+	Task(uint32_t expiresAfterMs, std::function<void(void)> &&f, std::string_view context);
 
-	Task(std::function<void(void)> &&f, std::string_view context, uint32_t delay, bool cycle = false, bool log = true) :
-		func(std::move(f)), context(context), utime(OTSYS_TIME() + delay), delay(delay), cycle(cycle), log(log) {
-		assert(!this->context.empty() && "Context cannot be empty!");
-	}
+	Task(std::function<void(void)> &&f, std::string_view context, uint32_t delay, bool cycle = false, bool log = true);
 
 	~Task() = default;
 

src/items/tile.cpp

@@ -57,6 +57,11 @@ bool Tile::hasProperty(ItemProperty prop) const {
 }
 
 bool Tile::hasProperty(std::shared_ptr<Item> exclude, ItemProperty prop) const {
+	if (!exclude) {
+		g_logger().error("[{}]: exclude is nullptr", __FUNCTION__);
+		return false;
+	}
+
 	assert(exclude);
 
 	if (ground && exclude != ground && ground->hasProperty(prop)) {

src/lua/functions/lua_functions_loader.cpp

@@ -148,6 +148,11 @@ void LuaFunctionsLoader::reportError(const char* function, const std::string &er
 int LuaFunctionsLoader::luaErrorHandler(lua_State* L) {
 	const std::string &errorMessage = popString(L);
 	auto interface = getScriptEnv()->getScriptInterface();
+	if (!interface) {
+		g_logger().error("[{}]: LuaScriptInterface not found, error: {}", __FUNCTION__, errorMessage);
+		return 0;
+	}
+
 	assert(interface); // This fires if the ScriptEnvironment hasn't been setup
 	pushString(L, interface->getStackTrace(errorMessage));
 	return 1;

src/lua/functions/lua_functions_loader.hpp

@@ -183,6 +183,11 @@ class LuaFunctionsLoader {
 	static int protectedCall(lua_State* L, int nargs, int nresults);
 
 	static ScriptEnvironment* getScriptEnv() {
+		if (scriptEnvIndex < 0 || scriptEnvIndex >= 16) {
+			g_logger().error("[{}]: scriptEnvIndex out of bounds!", __FUNCTION__);
+			return nullptr;
+		}
+
 		assert(scriptEnvIndex >= 0 && scriptEnvIndex < 16);
 		return scriptEnv + scriptEnvIndex;
 	}
@@ -192,6 +197,11 @@ class LuaFunctionsLoader {
 	}
 
 	static void resetScriptEnv() {
+		if (scriptEnvIndex < 0) {
+			g_logger().error("[{}]: scriptEnvIndex out of bounds!", __FUNCTION__);
+			return;
+		}
+
 		assert(scriptEnvIndex >= 0);
 		scriptEnv[scriptEnvIndex--].resetEnv();
 	}

src/map/utils/astarnodes.cpp

@@ -66,13 +66,23 @@ AStarNode* AStarNodes::getBestNode() {
 
 void AStarNodes::closeNode(const AStarNode* node) {
 	size_t index = node - nodes;
+	if (index >= MAX_NODES) {
+		g_logger().error("[{}]: node index out of bounds!", __FUNCTION__);
+		return;
+	}
+
 	assert(index < MAX_NODES);
 	openNodes[index] = false;
 	++closedNodes;
 }
 
 void AStarNodes::openNode(const AStarNode* node) {
 	size_t index = node - nodes;
+	if (index >= MAX_NODES) {
+		g_logger().error("[{}]: node index out of bounds!", __FUNCTION__);
+		return;
+	}
+
 	assert(index < MAX_NODES);
 	if (!openNodes[index]) {
 		openNodes[index] = true;

src/map/utils/qtreenode.cpp

@@ -80,12 +80,22 @@ void QTreeLeafNode::addCreature(const std::shared_ptr<Creature> &c) {
 
 void QTreeLeafNode::removeCreature(std::shared_ptr<Creature> c) {
 	auto iter = std::find(creature_list.begin(), creature_list.end(), c);
+	if (iter == creature_list.end()) {
+		g_logger().error("[{}]: Creature not found in creature_list!", __FUNCTION__);
+		return;
+	}
+
 	assert(iter != creature_list.end());
 	*iter = creature_list.back();
 	creature_list.pop_back();
 
 	if (c->getPlayer()) {
 		iter = std::find(player_list.begin(), player_list.end(), c);
+		if (iter == player_list.end()) {
+			g_logger().error("[{}]: Player not found in player_list!", __FUNCTION__);
+			return;
+		}
+
 		assert(iter != player_list.end());
 		*iter = player_list.back();
 		player_list.pop_back();

src/server/network/message/outputmessage.hpp

@@ -56,6 +56,11 @@ class OutputMessage : public NetworkMessage {
 private:
 	template <typename T>
 	void add_header(T addHeader) {
+		if (outputBufferStart < sizeof(T)) {
+			g_logger().error("[{}]: Insufficient buffer space for header!", __FUNCTION__);
+			return;
+		}
+
 		assert(outputBufferStart >= sizeof(T));
 		outputBufferStart -= sizeof(T);
 		memcpy(buffer + outputBufferStart, &addHeader, sizeof(T));

src/server/server.cpp

@@ -28,6 +28,11 @@ void ServiceManager::die() {
 }
 
 void ServiceManager::run() {
+	if (running) {
+		g_logger().error("ServiceManager is already running!", __FUNCTION__);
+		return;
+	}
+
 	assert(!running);
 	running = true;
 	io_service.run();