Fix memory leak in rollback (and more) (#16644)

builtin/settingtypes.txt

@@ -1057,10 +1057,6 @@ anticheat_flags (Anticheat flags) flags digging,interaction,movement digging,int
 #    Increase the value if players experience stuttery movement.
 anticheat_movement_tolerance (Anticheat movement tolerance) float 1.0 1.0
 
-#    If enabled, actions are recorded for rollback.
-#    This option is only read when server starts.
-enable_rollback_recording (Rollback recording) bool false
-
 [**Client-side Modding]
 
 #    Restricts the access of certain client-side functions on servers.
@@ -2249,6 +2245,11 @@ kick_msg_crash (Crash message) string This server has experienced an internal er
 #    Set this to true if your server is set up to restart automatically.
 ask_reconnect_on_crash (Ask to reconnect after crash) bool false
 
+#    If enabled, node and inventory actions are recorded for rollback.
+#    This option is only read on server start. Note that the engine will not
+#    automatically clean old entries from the rollback database.
+enable_rollback_recording (Rollback recording) bool false
+
 [**Server/Env Performance] [server]
 
 #    Length of a server tick (the interval at which everything is generally updated),

src/rollback_interface.h

@@ -22,12 +22,15 @@ struct RollbackNode
 	int param2 = 0;
 	std::string meta;
 
-	bool operator == (const RollbackNode &other)
+	bool operator == (const RollbackNode &other) const
 	{
 		return (name == other.name && param1 == other.param1 &&
 				param2 == other.param2 && meta == other.meta);
 	}
-	bool operator != (const RollbackNode &other) { return !(*this == other); }
+	bool operator != (const RollbackNode &other) const
+	{
+		return !(*this == other);
+	}
 
 	RollbackNode() = default;
 
@@ -37,15 +40,16 @@ struct RollbackNode
 
 struct RollbackAction
 {
-	enum Type{
+	enum Type : u8 {
 		TYPE_NOTHING,
 		TYPE_SET_NODE,
 		TYPE_MODIFY_INVENTORY_STACK,
-	} type = TYPE_NOTHING;
+	};
 
 	time_t unix_time = 0;
 	std::string actor;
 	bool actor_is_guess = false;
+	Type type = TYPE_NOTHING;
 
 	v3s16 p;
 	RollbackNode n_old;

src/server/rollback.cpp

@@ -17,7 +17,7 @@
 #include "sqlite3.h"
 #include "filesys.h"
 
-#define POINTS_PER_NODE (16.0)
+#define POINTS_PER_NODE (16.0f)
 
 #define SQLRES(f, good) \
 	if ((f) != (good)) {\
@@ -44,11 +44,11 @@ public:
 		return *this;
 	}
 
-	int id;
+	int id = 0;
 };
 
 struct ActionRow {
-	int          id;
+	int          id = 0;
 	int          actor;
 	time_t       timestamp;
 	int          type;
@@ -231,7 +231,6 @@ bool RollbackManager::createTables()
 		"CREATE INDEX IF NOT EXISTS `actionIndex` ON `action`(`x`,`y`,`z`,`timestamp`,`actor`);\n"
 		"CREATE INDEX IF NOT EXISTS `actionTimestampActorIndex` ON `action`(`timestamp`,`actor`);\n",
 		NULL, NULL, NULL));
-	verbosestream << "SQL Rollback: SQLite3 database structure was created" << std::endl;
 
 	return true;
 }
@@ -241,13 +240,10 @@ bool RollbackManager::initDatabase()
 {
 	verbosestream << "RollbackManager: Database connection setup" << std::endl;
 
-	bool needs_create = !fs::PathExists(database_path);
 	SQLOK(sqlite3_open_v2(database_path.c_str(), &db,
 			SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL));
 
-	if (needs_create) {
+	createTables();
-		createTables();
-	}
 
 	SQLOK(sqlite3_prepare_v2(db,
 		"INSERT INTO `action` (\n"
@@ -362,7 +358,7 @@ bool RollbackManager::initDatabase()
 	}
 	SQLOK(sqlite3_reset(stmt_knownNode_select));
 
-	return needs_create;
+	return true;
 }
 
 
@@ -522,7 +518,6 @@ ActionRow RollbackManager::actionRowFromRollbackAction(const RollbackAction & ac
 {
 	ActionRow row;
 
-	row.id        = 0;
 	row.actor     = getActorId(action.actor);
 	row.timestamp = action.unix_time;
 	row.type      = action.type;
@@ -589,7 +584,7 @@ const std::list<RollbackAction> RollbackManager::rollbackActionsFromActionRows(
 			break;
 
 		default:
-			throw ("W.T.F.");
+			assert(false);
 			break;
 		}
 
@@ -667,12 +662,10 @@ float RollbackManager::getSuspectNearness(bool is_guess, v3s16 suspect_p,
 	f -= 1 * (action_t - suspect_t);
 	// If is a guess, halve the points
 	if (is_guess) {
-		f *= 0.5;
+		f /= 2;
 	}
 	// Limit to 0
-	if (f < 0) {
+	f = MYMAX(f, 0);
-		f = 0;
-	}
 	return f;
 }
 
@@ -730,12 +723,11 @@ std::string RollbackManager::getSuspect(v3s16 p, float nearness_shortcut,
 	if (!current_actor.empty()) {
 		return current_actor;
 	}
-	int cur_time = time(0);
+	time_t cur_time = time(0);
 	time_t first_time = cur_time - (100 - min_nearness);
 	RollbackAction likely_suspect;
 	float likely_suspect_nearness = 0;
-	for (std::list<RollbackAction>::const_reverse_iterator
+	for (auto i = action_latest_buffer.rbegin();
-	     i = action_latest_buffer.rbegin();
 	     i != action_latest_buffer.rend(); ++i) {
 		if (i->unix_time < first_time) {
 			break;
@@ -795,15 +787,20 @@ void RollbackManager::addAction(const RollbackAction & action)
 	if (action_todisk_buffer.size() >= 500) {
 		flush();
 	}
+	// Cut off latest log sometimes
+	while (action_latest_buffer.size() >= 500) {
+		action_latest_buffer.pop_front();
+	}
 }
 
 std::list<RollbackAction> RollbackManager::getNodeActors(v3s16 pos, int range,
 		time_t seconds, int limit)
 {
-	flush();
 	time_t cur_time = time(0);
 	time_t first_time = cur_time - seconds;
 
+	flush();
+
 	return getActionsSince_range(first_time, pos, range, limit);
 }
 

src/server/rollback.h

@@ -9,6 +9,7 @@
 #include "rollback_interface.h"
 #include <list>
 #include <vector>
+#include <deque>
 #include "sqlite3.h"
 
 class IGameDef;
@@ -16,7 +17,7 @@ class IGameDef;
 struct ActionRow;
 struct Entity;
 
-class RollbackManager: public IRollbackManager
+class RollbackManager final : public IRollbackManager
 {
 public:
 	RollbackManager(const std::string & world_path, IGameDef * gamedef);
@@ -67,20 +68,20 @@ private:
 	std::string current_actor;
 	bool current_actor_is_guess = false;
 
-	std::list<RollbackAction> action_todisk_buffer;
+	std::vector<RollbackAction> action_todisk_buffer;
-	std::list<RollbackAction> action_latest_buffer;
+	std::deque<RollbackAction> action_latest_buffer;
 
 	std::string database_path;
-	sqlite3 * db;
+	sqlite3 *db = nullptr;
-	sqlite3_stmt * stmt_insert;
+	sqlite3_stmt *stmt_insert = nullptr;
-	sqlite3_stmt * stmt_replace;
+	sqlite3_stmt *stmt_replace = nullptr;
-	sqlite3_stmt * stmt_select;
+	sqlite3_stmt *stmt_select = nullptr;
-	sqlite3_stmt * stmt_select_range;
+	sqlite3_stmt *stmt_select_range = nullptr;
-	sqlite3_stmt * stmt_select_withActor;
+	sqlite3_stmt *stmt_select_withActor = nullptr;
-	sqlite3_stmt * stmt_knownActor_select;
+	sqlite3_stmt *stmt_knownActor_select = nullptr;
-	sqlite3_stmt * stmt_knownActor_insert;
+	sqlite3_stmt *stmt_knownActor_insert = nullptr;
-	sqlite3_stmt * stmt_knownNode_select;
+	sqlite3_stmt *stmt_knownNode_select = nullptr;
-	sqlite3_stmt * stmt_knownNode_insert;
+	sqlite3_stmt *stmt_knownNode_insert = nullptr;
 
 	std::vector<Entity> knownActors;
 	std::vector<Entity> knownNodes;