From b3f70b1b2bfdcc0e4ac4e1a61d5f41289381e315 Mon Sep 17 00:00:00 2001 From: Rushaway Date: Fri, 18 Sep 2026 22:46:10 +0200 Subject: [PATCH 1/7] fix(game): clean up nested DataPack handles in the ban flow Closes the 4 remaining DataPack leaks identified in sbpp/sourcebans-pp#1570 (comment) after the #21 port: the outer PlayerDataPack always owns a nested reasonPack allocated in PrepareBan/CreateBan, but four sites only freed (or skipped freeing) the outer pack, leaking the nested one on every affected code path. - VerifyInsert success path: the current transaction's ReasonPack was only deleted when a stale PlayerDataPack[admin] happened to exist (true for menu-selected reasons, false for command-issued bans with a reason), so command bans leaked it every time. Delete it unconditionally, and clean up the stale PlayerDataPack[admin] with CleanupBanDataPack() instead of a shallow delete. - VerifyInsert disconnected-client early return: leaked both dataPack and its nested reasonPack since neither was freed before the return. - OnMapEnd: replaced the shallow `delete PlayerDataPack[i]` (with a stale "need to close reason pack" comment) with CleanupBanDataPack(). - ReasonSelected's MenuCancel_Disconnected branch: same shallow-delete bug, now matches the already-correct cleanup in HackingSelected. Co-Authored-By: Claude Sonnet 5 --- game/addons/sourcemod/scripting/sbpp_main.sp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/game/addons/sourcemod/scripting/sbpp_main.sp b/game/addons/sourcemod/scripting/sbpp_main.sp index 734a8a724..3e05700e8 100644 --- a/game/addons/sourcemod/scripting/sbpp_main.sp +++ b/game/addons/sourcemod/scripting/sbpp_main.sp @@ -284,8 +284,8 @@ public void OnMapEnd() { if (PlayerDataPack[i] != null) { - /* Need to close reason pack */ - delete PlayerDataPack[i]; + CleanupBanDataPack(PlayerDataPack[i]); + PlayerDataPack[i] = null; } } } @@ -871,7 +871,8 @@ public int ReasonSelected(Menu menu, MenuAction action, int param1, int param2) { if (PlayerDataPack[param1] != null) { - delete PlayerDataPack[param1]; + CleanupBanDataPack(PlayerDataPack[param1]); + PlayerDataPack[param1] = null; } } @@ -1212,7 +1213,10 @@ public void VerifyInsert(Database db, DBResultSet results, const char[] error, D int client = dataPack.ReadCell(); if (!IsClientConnected(client) || IsFakeClient(client)) + { + CleanupBanDataPack(dataPack); return; + } dataPack.ReadCell(); // admin userid @@ -1247,10 +1251,12 @@ public void VerifyInsert(Database db, DBResultSet results, const char[] error, D LogAction(admin, client, "%t", "Ban Log", admin, client, time, Reason); + delete ReasonPack; + if (PlayerDataPack[admin] != INVALID_HANDLE) { - delete PlayerDataPack[admin]; - delete ReasonPack; + CleanupBanDataPack(PlayerDataPack[admin]); + PlayerDataPack[admin] = null; } // Kick player From 9ffc85998fb59edb7722946786cb5db6e52afb1c Mon Sep 17 00:00:00 2001 From: Rushaway Date: Fri, 18 Sep 2026 22:57:01 +0200 Subject: [PATCH 2/7] fix(game): stop double-freeing DataPack in Query_UnBlockSelect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of a broader DataPack-lifecycle audit prompted by sbpp/sourcebans-pp#1570; the other scripting/*.sp files (checker, sleuth, report, admcfg) were audited and found clean. When an unsilence (TYPE_UNSILENCE) also needs to clear a leftover temp mute/gag, Query_UnBlockSelect reuses its own dataPack and hands it to TempUnBlock(), which unconditionally deletes it after reading the fields it needs. Execution then fell through to this function's own unconditional `if (dataPack != null) delete dataPack;` at the end, freeing the same handle a second time — SourcePawn doesn't null out other copies of a Handle when it's deleted, so the leftover local still compared non-null. This fires on every unsilence where the target still has an active temp mute or gag after the DB-backed one is cleared, and either throws an invalid-handle error or frees an unrelated handle if the slot was recycled in between. Return immediately after handing the pack to TempUnBlock(), matching the same ownership-transfer pattern already used a few lines above for the query-failed/no-results early return. Co-Authored-By: Claude Sonnet 5 --- game/addons/sourcemod/scripting/sbpp_comms.sp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/game/addons/sourcemod/scripting/sbpp_comms.sp b/game/addons/sourcemod/scripting/sbpp_comms.sp index c50e82832..e70c6b047 100644 --- a/game/addons/sourcemod/scripting/sbpp_comms.sp +++ b/game/addons/sourcemod/scripting/sbpp_comms.sp @@ -1627,12 +1627,14 @@ public void Query_UnBlockSelect(Database db, DBResultSet results, const char[] e if (g_MuteType[target] > bNot) { dataPack.WriteCell(TYPE_UNMUTE); - TempUnBlock(dataPack); + TempUnBlock(dataPack); // Datapack closed inside. + return; } else if (g_GagType[target] > bNot) { dataPack.WriteCell(TYPE_UNGAG); - TempUnBlock(dataPack); + TempUnBlock(dataPack); // Datapack closed inside. + return; } } } From 3b6d07e48010da919e62cf60502d5406da3f0e25 Mon Sep 17 00:00:00 2001 From: Rushaway Date: Sat, 19 Sep 2026 00:10:24 +0200 Subject: [PATCH 3/7] fix(game): close out remaining Timer/DataPack/KeyValues lifecycle bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Broader sweep across every scripting/*.sp plugin for handle leaks (DataPack, ArrayList, StringMap, Timer, Menu, KeyValues, SMCParser), prompted by continuing the sbpp/sourcebans-pp#1570 cleanup. checker, sleuth, report and admcfg came back clean. Found and fixed: sbpp_comms.sp: - CreateMuteExpireTimer/CreateGagExpireTimer overwrote g_hMuteExpireTimer[target]/g_hGagExpireTimer[target] with a new Timer Handle without closing a still-pending previous one first (e.g. Query_VerifyBlock's connect-time loop can call PerformMute/ PerformGag more than once for the same client if the DB has more than one active row for them). Now calls the existing CloseMuteExpireTimer/CloseGagExpireTimer helpers first. sbpp_main.sp: - OnClientDisconnect deleted the pending PlayerRecheck[client] retry timer but never reset the slot to INVALID_HANDLE, so a later disconnect of a different player reusing that client index would delete the same stale handle value again. - VerifyInsert's DB-insert-failure branch and CreateBan's empty-reason branch both overwrote/nulled PlayerDataPack[admin] without freeing whatever pack (and its nested reasonPack) might already be sitting there from an earlier, still-unresolved no-reason ban by the same admin. Both now call CleanupBanDataPack() first, matching the pattern VerifyInsert's success path already used. - ParseBackupConfig_Overrides leaked its KeyValues handle on both early-return paths (missing/empty overrides_backup.cfg) — hit every time the DB is unreachable at startup or the overrides query fails. - SelectUnbanCallback leaked dataPack in the (defensive/edge-case) branch where RowCount > 0 but FetchRow() still returns false. Co-Authored-By: Claude Sonnet 5 --- game/addons/sourcemod/scripting/sbpp_comms.sp | 4 ++++ game/addons/sourcemod/scripting/sbpp_main.sp | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/game/addons/sourcemod/scripting/sbpp_comms.sp b/game/addons/sourcemod/scripting/sbpp_comms.sp index e70c6b047..094b512e4 100644 --- a/game/addons/sourcemod/scripting/sbpp_comms.sp +++ b/game/addons/sourcemod/scripting/sbpp_comms.sp @@ -3138,6 +3138,8 @@ stock void CreateMuteExpireTimer(int target, int remainingTime = 0) { if (g_iMuteLength[target] > 0) { + CloseMuteExpireTimer(target); + DataPack dataPack; if (remainingTime) @@ -3154,6 +3156,8 @@ stock void CreateGagExpireTimer(int target, int remainingTime = 0) { if (g_iGagLength[target] > 0) { + CloseGagExpireTimer(target); + DataPack dataPack; if (remainingTime) diff --git a/game/addons/sourcemod/scripting/sbpp_main.sp b/game/addons/sourcemod/scripting/sbpp_main.sp index 3e05700e8..cbb4754d3 100644 --- a/game/addons/sourcemod/scripting/sbpp_main.sp +++ b/game/addons/sourcemod/scripting/sbpp_main.sp @@ -305,6 +305,7 @@ public void OnClientDisconnect(int client) if (PlayerRecheck[client] != INVALID_HANDLE) { delete PlayerRecheck[client]; + PlayerRecheck[client] = INVALID_HANDLE; } FormatEx(g_sSteamIDs[client], sizeof(g_sSteamIDs[]), "\0"); @@ -1204,6 +1205,10 @@ public void VerifyInsert(Database db, DBResultSet results, const char[] error, D dataPack.Reset(); reasonPack.Reset(); + if (PlayerDataPack[admin] != null) + { + CleanupBanDataPack(PlayerDataPack[admin]); + } PlayerDataPack[admin] = null; UTIL_InsertTempBan(time, name, auth, ip, reason, adminAuth, adminIp, dataPack); return; @@ -1455,6 +1460,10 @@ public void SelectUnbanCallback(Database db, DBResultSet results, const char[] e db.Query(InsertUnbanCallback, query, dataPack); } + else + { + delete dataPack; + } return; } @@ -2858,6 +2867,10 @@ public bool CreateBan(int client, int target, int time, const char[] reason) } } else { // We need a reason so offer the administrator a menu of reasons + if (PlayerDataPack[admin] != null) + { + CleanupBanDataPack(PlayerDataPack[admin]); + } PlayerDataPack[admin] = dataPack; DisplayMenu(ReasonMenuHandle, admin, MENU_TIME_FOREVER); ReplyToCommand(admin, "%s%t", Prefix, "Check Menu"); @@ -3177,10 +3190,16 @@ stock void ParseBackupConfig_Overrides() KeyValues hKV = new KeyValues("SB_Overrides"); if (!hKV.ImportFromFile(overridesLoc)) + { + delete hKV; return; + } if (!hKV.GotoFirstSubKey()) + { + delete hKV; return; + } char sSection[16], sFlags[32], sName[MAX_NAME_LENGTH]; OverrideType type; From 794a55b7282ef6534b38190e330ebdbd003ffe1b Mon Sep 17 00:00:00 2001 From: Rushaway Date: Tue, 22 Sep 2026 10:49:12 +0200 Subject: [PATCH 4/7] fix(game): resolve ban-flow admin/target by userid, not raw client index Companion to the DataPack/Timer/KeyValues handle-leak sweep already on this branch: upstream sbpp/sourcebans-pp#1571 ("close DataPack ownership leaks") also fixed a second, distinct bug class in the same functions that this branch's earlier commits didn't cover - stale client-index reuse across an async DB round trip. Every ban command (sm_ban, sm_banip, sm_addban, sm_unban) and the target-list/time/reason menu flow packs the issuing admin's (and, for the menu flow, the target's) raw client index into a DataPack or a per-slot global array, then reads it back after an async SQL query or after the admin steps through several menu screens. SourceMod reuses a freed client slot immediately, so if the original admin or target disconnects in that window and a different player connects into the same slot, the stale index now points at an unrelated player. The existing IsClientInGame()/IsClientConnected() guards can't tell "still the same player" from "someone else now" - only a userid survives a disconnect/reconnect as a stable identity. sbpp_comms.sp already carries this pattern (Query_UnBlockSelect reads adminUserID/targetUserID and resolves via GetClientOfUserId()); this port brings sbpp_main.sp's ban flow in line with it: - New g_BanTargetUserId[] parallels g_BanTarget[]/g_BanTime[] so the target-list -> time -> reason menu chain (ReasonSelected, HackingSelected, ChatHook's own-reason path) re-validates the target's userid in PrepareBan() before banning, instead of trusting a live-looking but possibly-reused client index. - CommandBanIp/CommandUnban/CommandAddBan now pack the admin's userid instead of the raw index; SelectBanIpCallback, InsertBanIpCallback, SelectUnbanCallback, InsertUnbanCallback, SelectAddbanCallback and InsertAddbanCallback all resolve it back via GetClientOfUserId() before using it for ShowActivity2()/LogAction()/PrintToChat() - the existing "admin && IsClientInGame(admin)" guards stay correct as-is since GetClientOfUserId() returns 0 for a slot that moved on. - VerifyInsert (both the success path and the primary-DB-failure path that falls back to the SQLite queue) and UTIL_InsertTempBan now re-resolve admin from the adminUserId field the pack already carried but discarded, and UTIL_InsertTempBan additionally re-resolves the target from targetUserId before deciding whether to kick them. - Native_SBBanPlayer's PrepareBan() call passes the target's current userid, which is a no-op validation there since the call is synchronous - kept for signature consistency and because the native is public API third-party plugins call. Co-Authored-By: Claude Sonnet 5 --- game/addons/sourcemod/scripting/sbpp_main.sp | 92 ++++++++++++++------ 1 file changed, 66 insertions(+), 26 deletions(-) diff --git a/game/addons/sourcemod/scripting/sbpp_main.sp b/game/addons/sourcemod/scripting/sbpp_main.sp index cbb4754d3..1d86bbc28 100644 --- a/game/addons/sourcemod/scripting/sbpp_main.sp +++ b/game/addons/sourcemod/scripting/sbpp_main.sp @@ -104,6 +104,7 @@ bool int g_BanTarget[MAXPLAYERS + 1] = { -1, ... } , g_BanTime[MAXPLAYERS + 1] = { -1, ... } + , g_BanTargetUserId[MAXPLAYERS + 1] = { -1, ... } , AutoAdd = 0 , curLoading , serverID = -1 @@ -419,7 +420,7 @@ public Action ChatHook(int client, int args) } // ban him! - PrepareBan(client, g_BanTarget[client], g_BanTime[client], reason); + PrepareBan(client, g_BanTarget[client], g_BanTime[client], reason, g_BanTargetUserId[client]); // block the reason to be sent in chat return Plugin_Handled; @@ -504,6 +505,7 @@ public Action CommandBan(int client, int args) g_BanTarget[client] = target; g_BanTime[client] = time; + g_BanTargetUserId[client] = GetClientUserId(target); CreateBan(client, target, time, reason); return Plugin_Handled; @@ -581,9 +583,13 @@ public Action CommandBanIp(int client, int args) return Plugin_Handled; } - // Pack everything into a data pack so we can retain it + // Pack everything into a data pack so we can retain it. Store the admin's + // userid rather than the raw client index: SourceMod reuses freed client + // slots immediately, and this pack survives an async DB round trip, so a + // bare index could end up pointing at an unrelated player who connected + // into the same slot while the query was in flight. DataPack dataPack = new DataPack(); - dataPack.WriteCell(client); + dataPack.WriteCell(client == 0 ? 0 : GetClientUserId(client)); dataPack.WriteCell(minutes); dataPack.WriteString(Arguments[len]); dataPack.WriteString(g_sPlayerIP[target]); @@ -647,9 +653,10 @@ public Action CommandUnban(int client, int args) } } - // Pack everything into a data pack so we can retain it + // Pack everything into a data pack so we can retain it. Store the admin's + // userid, not the raw client index — see CommandBanIp for why. DataPack dataPack = new DataPack(); - dataPack.WriteCell(client); + dataPack.WriteCell(client == 0 ? 0 : GetClientUserId(client)); dataPack.WriteString(Arguments[len]); // Reason dataPack.WriteString(arg); // Steamid - IP dataPack.WriteString(adminAuth); // Admin SteamID @@ -732,9 +739,10 @@ public Action CommandAddBan(int client, int args) return Plugin_Handled; } - // Pack everything into a data pack so we can retain it + // Pack everything into a data pack so we can retain it. Store the admin's + // userid, not the raw client index — see CommandBanIp for why. DataPack dataPack = new DataPack(); - dataPack.WriteCell(client); + dataPack.WriteCell(client == 0 ? 0 : GetClientUserId(client)); dataPack.WriteCell(minutes); dataPack.WriteString(arg_string[total_len]); dataPack.WriteString(authid); @@ -863,7 +871,7 @@ public int ReasonSelected(Menu menu, MenuAction action, int param1, int param2) } else if (g_BanTarget[param1] != -1 && g_BanTime[param1] != -1) - PrepareBan(param1, g_BanTarget[param1], g_BanTime[param1], info); + PrepareBan(param1, g_BanTarget[param1], g_BanTime[param1], info, g_BanTargetUserId[param1]); } case MenuAction_Cancel: @@ -897,7 +905,7 @@ public int HackingSelected(Menu menu, MenuAction action, int param1, int param2) menu.GetItem(param2, key, sizeof(key), _, info, sizeof(info)); if (g_BanTarget[param1] != -1 && g_BanTime[param1] != -1) - PrepareBan(param1, g_BanTarget[param1], g_BanTime[param1], info); + PrepareBan(param1, g_BanTarget[param1], g_BanTime[param1], info, g_BanTargetUserId[param1]); } case MenuAction_Cancel: @@ -975,6 +983,7 @@ public int MenuHandler_BanPlayerList(Menu menu, MenuAction action, int param1, i else { g_BanTarget[param1] = target; + g_BanTargetUserId[param1] = userid; DisplayBanTimeMenu(param1); } } @@ -1186,7 +1195,7 @@ public void VerifyInsert(Database db, DBResultSet results, const char[] error, D int admin = dataPack.ReadCell(); dataPack.ReadCell(); // target - dataPack.ReadCell(); // admin userid + int adminUserId = dataPack.ReadCell(); dataPack.ReadCell(); // target userid int time = dataPack.ReadCell(); @@ -1205,6 +1214,11 @@ public void VerifyInsert(Database db, DBResultSet results, const char[] error, D dataPack.Reset(); reasonPack.Reset(); + // Re-resolve the admin from their userid: this branch only runs after + // the primary-DB INSERT failed asynchronously, so the admin's client + // slot may have been recycled by a different player in the meantime. + admin = adminUserId == 0 ? 0 : GetClientOfUserId(adminUserId); + if (PlayerDataPack[admin] != null) { CleanupBanDataPack(PlayerDataPack[admin]); @@ -1223,7 +1237,13 @@ public void VerifyInsert(Database db, DBResultSet results, const char[] error, D return; } - dataPack.ReadCell(); // admin userid + // Re-resolve the admin from their userid: the ban INSERT this callback + // reports on ran asynchronously, so the admin's client slot may have been + // recycled by a different player while it was in flight. Without this, + // ShowActivity2()/LogAction() below and the PlayerDataPack[admin] cleanup + // would act on (or clobber) whoever now happens to occupy that slot. + int adminUserId = dataPack.ReadCell(); + admin = adminUserId == 0 ? 0 : GetClientOfUserId(adminUserId); int UserId = dataPack.ReadCell(); int time = dataPack.ReadCell(); @@ -1284,7 +1304,8 @@ public void SelectBanIpCallback(Database db, DBResultSet results, const char[] e char targetName[MAX_NAME_LENGTH], targetAuth[MAX_AUTHID_LENGTH]; dataPack.Reset(); - admin = dataPack.ReadCell(); + int adminUserId = dataPack.ReadCell(); + admin = adminUserId == 0 ? 0 : GetClientOfUserId(adminUserId); minutes = dataPack.ReadCell(); dataPack.ReadString(reason, sizeof(reason)); dataPack.ReadString(ip, sizeof(ip)); @@ -1353,11 +1374,12 @@ public void InsertBanIpCallback(Database db, DBResultSet results, const char[] e if (dataPack != null) { dataPack.Reset(); - admin = dataPack.ReadCell(); + int adminUserId = dataPack.ReadCell(); + admin = adminUserId == 0 ? 0 : GetClientOfUserId(adminUserId); minutes = dataPack.ReadCell(); dataPack.ReadString(reason, sizeof(reason)); dataPack.ReadString(targetIP, sizeof(targetIP)); - + for(int i = 1; i <= MaxClients; i++) { if(!IsClientInGame(i) || IsFakeClient(i)) @@ -1413,7 +1435,8 @@ public void SelectUnbanCallback(Database db, DBResultSet results, const char[] e char reason[128]; dataPack.Reset(); - admin = dataPack.ReadCell(); + int adminUserId = dataPack.ReadCell(); + admin = adminUserId == 0 ? 0 : GetClientOfUserId(adminUserId); dataPack.ReadString(reason, sizeof(reason)); // Reason dataPack.ReadString(arg, sizeof(arg)); // SteamID - IP dataPack.ReadString(adminAuth, sizeof(adminAuth)); // Admin SteamID @@ -1477,7 +1500,8 @@ public void InsertUnbanCallback(Database db, DBResultSet results, const char[] e if (dataPack != null) { dataPack.Reset(); - admin = dataPack.ReadCell(); + int adminUserId = dataPack.ReadCell(); + admin = adminUserId == 0 ? 0 : GetClientOfUserId(adminUserId); dataPack.ReadString(reason, sizeof(reason)); // Reason dataPack.ReadString(arg, sizeof(arg)); // SteamID - IP delete dataPack; @@ -1512,7 +1536,8 @@ public void SelectAddbanCallback(Database db, DBResultSet results, const char[] char reason[128]; dataPack.Reset(); - admin = dataPack.ReadCell(); + int adminUserId = dataPack.ReadCell(); + admin = adminUserId == 0 ? 0 : GetClientOfUserId(adminUserId); minutes = dataPack.ReadCell(); dataPack.ReadString(reason, sizeof(reason)); dataPack.ReadString(authid, sizeof(authid)); @@ -1576,7 +1601,8 @@ public void InsertAddbanCallback(Database db, DBResultSet results, const char[] char reason[128]; dataPack.Reset(); - admin = dataPack.ReadCell(); + int adminUserId = dataPack.ReadCell(); + admin = adminUserId == 0 ? 0 : GetClientOfUserId(adminUserId); minutes = dataPack.ReadCell(); dataPack.ReadString(reason, sizeof(reason)); dataPack.ReadString(authid, sizeof(authid)); @@ -2502,7 +2528,7 @@ public int Native_SBBanPlayer(Handle plugin, int numParams) } } - PrepareBan(client, target, time, reason); + PrepareBan(client, target, time, reason, GetClientUserId(target)); return true; } @@ -2946,13 +2972,20 @@ stock void UTIL_InsertBan(int time, const char[] Name, const char[] Authid, cons stock void UTIL_InsertTempBan(int time, const char[] name, const char[] auth, const char[] ip, const char[] reason, const char[] adminAuth, const char[] adminIp, DataPack dataPack) { - int admin = dataPack.ReadCell(); // admin index + dataPack.ReadCell(); // admin index (unused; admin is resolved below via userid) + dataPack.ReadCell(); // target index (unused; client is resolved below via userid) - int client = dataPack.ReadCell(); + int adminUserId = dataPack.ReadCell(); + int targetUserId = dataPack.ReadCell(); + dataPack.ReadCell(); // time (already provided as a parameter) - dataPack.ReadCell(); // admin userid - dataPack.ReadCell(); // target userid - dataPack.ReadCell(); // time + // This can be reached asynchronously (VerifyInsert's primary-DB-failure + // branch), so re-resolve both parties from their userid rather than the + // raw client index captured when the ban was first issued: SourceMod can + // reuse a freed slot before this fallback fires, and IsClientInGame() + // alone can't tell "still the same player" from "someone else now". + int admin = adminUserId == 0 ? 0 : GetClientOfUserId(adminUserId); + int client = targetUserId == 0 ? 0 : GetClientOfUserId(targetUserId); DataPack reasonPack = view_as(dataPack.ReadCell()); @@ -3098,13 +3131,20 @@ stock void InsertServerInfo() } } -stock void PrepareBan(int client, int target, int time, char[] reason) +stock void PrepareBan(int client, int target, int time, char[] reason, int targetUserId) { #if defined DEBUG LogToFile(logFile, "PrepareBan()"); #endif - if (!target || !IsClientInGame(target)) + // target is a client index cached across an admin's menu navigation + // (target list -> time -> reason), which can span several seconds of + // real time. If the original target disconnected in that window, + // SourceMod may have already reassigned their slot to a newly + // connecting player; IsClientInGame() alone can't tell the two apart. + // Re-check the userid captured at selection time to make sure we are + // still about to ban the player the admin actually picked. + if (!target || !IsClientInGame(target) || GetClientUserId(target) != targetUserId) return; char bannedSite[512]; From b137200ca149c046696996dbd0c99b52df238e47 Mon Sep 17 00:00:00 2001 From: Rushaway Date: Tue, 22 Sep 2026 12:01:34 +0200 Subject: [PATCH 5/7] fix(game): fix SQLite ban-queue reprocessing bugs found while porting #1571/#1572 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs surfaced while comparing this fork's Synergy/queue-related plugin fixes against upstream sbpp/sourcebans-pp#1572 ("use native auth ids for engine bans") — upstream's diff touches the same functions and happens to carry the fix for the first one as a side effect. 1. ProcessQueueCallback re-INSERTs a matured queue row into the main `%!s_bans` table, but was building and executing that query against `db` — the local SQLite queue connection this callback fires on (bound via SQLiteDB.Query(ProcessQueueCallback, ...) in ProcessQueue()) — instead of `DB`, the main panel database the `%!s_bans` table actually lives on. SQLite has no such table, so the INSERT would fail every time; AddedFromSQLiteCallback's `results == null` branch just re-arms the temporary in-game hold and leaves the row in the queue, so the ban would keep getting re-enforced locally but NEVER actually land in the permanent database once it came back up after an outage — silently defeating the entire point of the offline queue. Switched both the `.Format()` call (MySQL vs SQLite escaping rules differ) and the final `.Query()` to `DB`. Paired with a `DB == INVALID_HANDLE` guard at the top of the callback (matching upstream) so this doesn't trade "silently fails forever" for "throws a native error" on a cycle where the main DB is still down. 2. AddedFromSQLiteCallback's success path calls `RemoveBan(auth, BANFLAG_AUTHID)` with the raw Steam2 identity to clear the temporary in-game hold once the real ban lands in the main database. But the temp hold was created by SBPP_BanIdentity(), which converts to SteamID3 whenever it can (some engines, e.g. Synergy, reject the STEAM_ format for the "banid" console command, so SBPP_BanIdentity works around it) — a Steam2-only RemoveBan() call silently misses a SteamID3-keyed entry, leaving a stale temporary ban sitting in SourceMod's cache until it expires on its own. Try the SteamID3 form too; removing an identity with no matching ban is a harmless no-op. Co-Authored-By: Claude Sonnet 5 --- game/addons/sourcemod/scripting/sbpp_main.sp | 44 +++++++++++++++++--- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/game/addons/sourcemod/scripting/sbpp_main.sp b/game/addons/sourcemod/scripting/sbpp_main.sp index 1d86bbc28..47c653cb8 100644 --- a/game/addons/sourcemod/scripting/sbpp_main.sp +++ b/game/addons/sourcemod/scripting/sbpp_main.sp @@ -1636,6 +1636,18 @@ public void ProcessQueueCallback(Database db, DBResultSet results, const char[] return; } + // Every row in the queue needs to be re-INSERTed against the main game + // panel database (DB), not the local SQLite queue DB (db, the handle + // this callback fired on) — the whole point of this timer is to catch + // up once DB is reachable again. If it still isn't, bail and retry on + // the next cycle instead of running DB.Format()/DB.Query() against an + // invalid handle, which would throw a native error. + if (DB == INVALID_HANDLE) + { + CreateTimer(float(ProcessQueueTime * 60), ProcessQueue); + return; + } + char auth[MAX_AUTHID_LENGTH]; int time; int startTime; @@ -1662,10 +1674,13 @@ public void ProcessQueueCallback(Database db, DBResultSet results, const char[] results.FetchString(7, adminIp, sizeof(adminIp)); if (startTime + time * 60 > GetTime() || time == 0) { - // This ban is still valid and should be entered into the db + // This ban is still valid and should be entered into the db. + // DB.Format(), not db.Format(): db is the local SQLite queue + // handle this callback fired on, but %!s_bans lives on the main + // panel database and MySQL/SQLite escaping rules differ. if (serverID == -1) { - if (db.Format(query, sizeof(query), + if (DB.Format(query, sizeof(query), "INSERT INTO %!s_bans (ip, authid, name, created, ends, length, reason, aid, adminIp, admin_name, sid) VALUES \ ('%s', '%s', '%s', %d, %d, %d, '%s', (SELECT aid FROM %!s_admins WHERE authid = '%s' OR authid REGEXP '^STEAM_[0-9]:%s$'), '%s', \ IFNULL((SELECT user FROM %!s_admins WHERE authid = '%s' OR authid REGEXP '^STEAM_[0-9]:%s$'), ''), \ @@ -1678,7 +1693,7 @@ public void ProcessQueueCallback(Database db, DBResultSet results, const char[] } else { - if (db.Format(query, sizeof(query), + if (DB.Format(query, sizeof(query), "INSERT INTO %!s_bans (ip, authid, name, created, ends, length, reason, aid, adminIp, admin_name, sid) VALUES \ ('%s', '%s', '%s', %d, %d, %d, '%s', (SELECT aid FROM %!s_admins WHERE authid = '%s' OR authid REGEXP '^STEAM_[0-9]:%s$'), '%s', \ IFNULL((SELECT user FROM %!s_admins WHERE authid = '%s' OR authid REGEXP '^STEAM_[0-9]:%s$'), ''), \ @@ -1692,7 +1707,15 @@ public void ProcessQueueCallback(Database db, DBResultSet results, const char[] DataPack authPack = new DataPack(); authPack.WriteString(auth); authPack.Reset(); - db.Query(AddedFromSQLiteCallback, query, authPack); + // Same reason: this INSERT has to run against DB, the connection + // the query text was built for. Pre-fix this ran against `db` + // (SQLite), which has no %!s_bans table — every re-queued ban + // would fail this INSERT forever (AddedFromSQLiteCallback's + // `results == null` branch just re-arms the temp ban and leaves + // the row in the queue), so a ban recorded during a DB outage + // would never actually land in the permanent table once the + // database came back up. + DB.Query(AddedFromSQLiteCallback, query, authPack); } else { // The ban is no longer valid and should be deleted from the queue if (db.Format(query, sizeof(query), "DELETE FROM queue WHERE steam_id = '%s'", auth) >= sizeof(query) - 1) @@ -1724,8 +1747,19 @@ public void AddedFromSQLiteCallback(Database db, DBResultSet results, const char } SQLiteDB.Query(ErrorCheckCallback, buffer); - // They are added to main banlist, so remove the temp ban + // They are added to main banlist, so remove the temp ban. + // SBPP_BanIdentity() (called from UTIL_InsertTempBan / this + // callback's own failure branch below) may have created that temp + // ban under the engine-native / SteamID3 form instead of Steam2 — + // some engines (Synergy) reject "banid" with a STEAM_ string, so + // SBPP_BanIdentity converts to SteamID3 whenever it can. A + // Steam2-only RemoveBan() call would silently miss that entry, so + // try the SteamID3 form too; removing an identity SourceMod has no + // matching ban for is a harmless no-op. RemoveBan(auth, BANFLAG_AUTHID); + char removeSteam3[MAX_AUTHID_LENGTH]; + if (SBPP_Steam2ToSteam3(auth, removeSteam3, sizeof(removeSteam3))) + RemoveBan(removeSteam3, BANFLAG_AUTHID); } else { // the insert failed so we leave the record in the queue and increase our temporary ban From 929ef650036e014a882dbabf10625c9075fa6619 Mon Sep 17 00:00:00 2001 From: Cedric Mercier Date: Sat, 26 Sep 2026 14:27:48 +0200 Subject: [PATCH 6/7] fix(game): don't abort temp-ban queueing when the target already left UTIL_InsertTempBan now resolves the target via GetClientOfUserId(), which returns 0 once the target is gone -- the normal case on the primary-DB failure path, since PrepareBan() kicks right after CreateBan(). IsClientInGame(0) raises a native error, aborting before SBPP_BanIdentity() and the SQLite queue INSERT, so the ban was lost. Guard client > 0. Also stop Native_SBBanPlayer from calling GetClientUserId() on an invalid or disconnected target (throws in the caller plugin); pass 0 so PrepareBan()'s own guard keeps the previous silent no-op. --- game/addons/sourcemod/scripting/sbpp_main.sp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/game/addons/sourcemod/scripting/sbpp_main.sp b/game/addons/sourcemod/scripting/sbpp_main.sp index 47c653cb8..9e76f6f0d 100644 --- a/game/addons/sourcemod/scripting/sbpp_main.sp +++ b/game/addons/sourcemod/scripting/sbpp_main.sp @@ -2562,7 +2562,11 @@ public int Native_SBBanPlayer(Handle plugin, int numParams) } } - PrepareBan(client, target, time, reason, GetClientUserId(target)); + // GetClientUserId() throws on an invalid/disconnected index; pass 0 so + // PrepareBan()'s own target guard turns it into a silent no-op, as the + // native did before userids were threaded through. + int targetUserId = (target > 0 && target <= MaxClients && IsClientConnected(target)) ? GetClientUserId(target) : 0; + PrepareBan(client, target, time, reason, targetUserId); return true; } @@ -3030,7 +3034,10 @@ stock void UTIL_InsertTempBan(int time, const char[] name, const char[] auth, co // we add a temporary ban and then add the record into the queue to be processed when the database is available char kickMessage[512] = ""; - if (IsClientInGame(client)) + // client is 0 when the target already left (the usual case here: the + // target is kicked right after CreateBan()), and IsClientInGame(0) + // raises a native error that would abort before the queue INSERT. + if (client > 0 && IsClientInGame(client)) { char length[32]; if(time == 0) From 1747b6e6af56e6788165c734e785b66e30801c04 Mon Sep 17 00:00:00 2001 From: Cedric Mercier Date: Sat, 26 Sep 2026 14:39:54 +0200 Subject: [PATCH 7/7] fix(game): keep retrying the SQLite ban queue after a failed SELECT ProcessQueueCallback returned without re-arming the ProcessQueue timer when the SQLite SELECT failed, so one transient error stopped queued bans from being replayed until the next plugin load. --- game/addons/sourcemod/scripting/sbpp_main.sp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/game/addons/sourcemod/scripting/sbpp_main.sp b/game/addons/sourcemod/scripting/sbpp_main.sp index 9e76f6f0d..3f23dcb32 100644 --- a/game/addons/sourcemod/scripting/sbpp_main.sp +++ b/game/addons/sourcemod/scripting/sbpp_main.sp @@ -1633,6 +1633,9 @@ public void ProcessQueueCallback(Database db, DBResultSet results, const char[] if (results == null) { LogToFile(logFile, "Failed to retrieve queued bans from sqlite database, %s", error); + // Re-arm like every other exit path, otherwise a single failed + // SELECT stops queue processing until the next plugin load. + CreateTimer(float(ProcessQueueTime * 60), ProcessQueue); return; }