fix(game): close DataPack ownership leaks and stale client-index reuse in the ban flow

[Rushaway]

Summary

Port of upstream sbpp/sourcebans-pp#1571 ("close DataPack ownership leaks"), covering both halves of that fix independently discovered/ported here.

Part 1 — handle leaks (DataPack / Timer / KeyValues)

Follow-up to #21 (port of upstream sbpp#1492), prompted by sbpp/sourcebans-pp#1570 (comment), which flagged 4 remaining DataPack leaks in this fork's sbpp_main.sp. That led to a full sweep of every plugin under scripting/*.sp for the same class of bug — resources allocated (DataPack, Timer, KeyValues, Menu, StringMap, ArrayList, SMCParser) and not freed on every code path, or a stored handle overwritten/reset without freeing what was there before. sbpp_checker.sp, sbpp_sleuth.sp, sbpp_report.sp, sbpp_admcfg.sp (and its sbpp_admin_groups.sp/sbpp_admin_users.sp) came back clean on two independent passes.

sbpp_main.sp — 4 leaks, 1 double-free (nested reasonPack / stale PlayerDataPack / stale PlayerRecheck), sbpp_comms.sp — 1 double-free + 1 orphaned-timer bug. See the individual commits for the full per-site breakdown.

Part 2 — stale client-index reuse (admin/target identity across an async gap)

Auditing the same functions against the other half of upstream sbpp#1571 turned up a second, distinct bug class the handle-leak sweep didn't touch: 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 — 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. IsClientInGame()/IsClientConnected() alone 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()); the last commit 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 — a no-op validation there since that call is synchronous, kept for signature consistency and because the native is public API third-party plugins call.

Test plan

• Read through each changed call site to confirm DataPack/handle ownership and read-cursor layout — verified by inspection, no cursor-order changes were made beyond what's documented above.
• Compile sbpp_main.sp and sbpp_comms.sp with spcomp and confirm no errors/warnings.
• On a live/dev server: issue a ban via command with a reason, via the reason menu, and cancel the reason menu after the target disconnects; confirm no DataPack/handle count growth via sm_handles across repeated bans and map changes.
• Issue two no-reason bans from the same admin back-to-back (second one before picking a reason for the first) and confirm no handle growth.
• Trigger an unsilence on a target that also has an active temp mute or gag and confirm no invalid-handle error/crash.
• Mute/gag a target twice in a row (e.g. via on-connect DB restore with more than one active row) and confirm only one expire timer survives per punishment type.
• Start a sm_ban menu flow (target list → time → reason), have the target disconnect and a different player reconnect into the same slot before picking a reason, and confirm the ban is silently skipped rather than applied to the new occupant.
• Issue sm_addban/sm_banip/sm_unban from an admin who disconnects before the async SELECT/INSERT round trip resolves (and a different player takes their slot); confirm the success/failure chat message is not misattributed to the new occupant.

🤖 Generated with Claude Code

[Rushaway]

Added a third commit while investigating upstream sbpp#1572 ("use native auth ids for engine bans") for a separate audit — turned up two bugs in the SQLite ban-queue reprocessing path that are worth fixing alongside the DataPack/userid work already here:

1. ProcessQueueCallback re-INSERTs a matured queue row against the wrong database connection. It built and executed the %!s_bans INSERT against db (the local SQLite queue handle this callback fires on) instead of DB (the main panel database %!s_bans actually lives on). SQLite has no such table, so the INSERT fails every time — AddedFromSQLiteCallback's results == null branch just re-arms the temporary in-game hold and leaves the row queued forever. A ban recorded during a brief main-DB outage would never actually land in the permanent table once the database came back up, silently defeating the entire point of the offline queue. Fixed both the .Format() call and the final .Query() to target DB, paired with a DB == INVALID_HANDLE guard at the top of the callback (matching upstream's own fix in the same area) so a still-down DB doesn't trade "fails silently" for "throws a native error".

2. AddedFromSQLiteCallback's RemoveBan(auth, BANFLAG_AUTHID) can miss the temp hold it's trying to clear. The temporary in-game ban was created via SBPP_BanIdentity(), which converts to SteamID3 whenever it can (this is the Synergy workaround this branch's earlier commits are about) — removing by the raw Steam2 string alone silently misses a SteamID3-keyed entry, leaving a stale temp ban sitting in SourceMod's cache until it expires on its own (~ProcessQueueTime minutes, so low severity, but still wrong). Now tries both identity forms.

Neither bug is new in this PR — both predate it — but they surfaced from reading the same functions upstream's sbpp#1572 touches, so bundling them here made more sense than a separate PR against the same file.