fix(game): Harmonize SQL query formatting, truncation logging, and DataPack lifecycle

[Rushaway]

Description

Port of upstream sbpp/sourcebans-pp#1492 to the srcdslab fork.

Updated files:

• sbpp_main.sp
• sbpp_comms.sp

Key changes:

• Replaced manual Escape + Format patterns with DB.Format / SQLiteDB.Format.
• Standardized placeholder usage: %!s for internal SQL identifiers/fragments, %s for user/runtime input.
• Added defensive query buffer truncation checks before every DB.Query, all reported via LogError.
• DataPack objects are now allocated only after successful query formatting; added CleanupBanDataPack helper.

Fork-specific adaptation

The srcdslab fork carries an extra admin_name column (and IFNULL((SELECT user FROM %!s_admins ...)) subquery) in the _bans and _comms INSERT statements. These were preserved through the merge and converted to the new DB.Format / %!s contract, including new truncation guards on the added admin-name subqueries.

Notes

• Upstream PR is intentionally left open.
• Not compiled locally (no spcomp available); relies on the plugin-build CI workflow.

🤖 Generated with Claude Code

[Rushaway]

Review: approve after 2 fixes (pushed as a6205b1)

Verified the port against gh pr diff 1492 --repo sbpp/sourcebans-pp and the surrounding code in both plugins. The fork-specific admin_name adaptation is intact and the placeholder policy is applied correctly, but two defects needed fixing.

Fixed

1. Every truncation guard was dead code. Database.Format (SQL_FormatQuery) routes through InternalFormat → atcprintf, which initialises llen = maxlen - 1 and returns maxlen - llen - 1. The return value is therefore capped at maxlength - 1 and can never satisfy >= sizeof(buffer). All 34 guards — including the three inverted < sizeof(...) success-checks in VerifyBan and the >= iQueryLen check in Native_SBReportPlayer — could never fire, so the PR's headline feature was a no-op. Changed to >= sizeof(buf) - 1 / < sizeof(buf) - 1 / >= iQueryLen - 1. This is conservative (a query that exactly fills the buffer is now treated as truncated), which is the right side to err on.

Note: this also affects upstream sbpp#1492 and is worth reporting there.

2. sbpp_comms.sp InsertTempBlock escaped its own SQL. The queue2 INSERT kept VALUES %s%s%s while passing the prebuilt sQueryMute / sQueryGag value tuples. %s escapes, so SQLiteDB.Format doubled the single quotes inside those fragments and produced an invalid statement — temp comm blocks queued while the main DB is down would have silently failed to insert. Changed to %!s%!s%!s, matching the equivalent statement in SavePunishment (which upstream did convert). Upstream sbpp#1492 has the same miss.

Verified clean

• Placeholders. All 44 %!s in sbpp_main.sp are table-prefix positions (%!s_bans, %!s_admins, %!s_servers, %!s_banlog, %!s_mods, %!s_submissions) fed DatabasePrefix. In sbpp_comms.sp the 11 bare %!s are typeWHERE, sQueryAdm, sQueryAdmName, sQueryMute/sQueryGag and the ", " separator — all internally constructed. Every runtime/user value (auth ids, names, reasons, IPs, rcon, game folder) uses %s.
• Arg count/order. Hand-checked all 17/15/18/16-arg INSERTs in SelectBanIpCallback, SelectAddbanCallback, ProcessQueueCallback, UTIL_InsertBan, Query_ProcessQueue, SavePunishment, plus Native_SBReportPlayer, ServerInfoCallback and both VerifyBan banlog variants. All match, including the column order in _submissions.
• admin_name fork divergence. admin_name occurrence count (8 in main, 4 in comms) and SELECT user FROM %!s_admins count (8 / 2) are identical to origin/main. The IFNULL((SELECT user FROM %!s_admins WHERE authid = '%s' OR authid REGEXP '^STEAM_[0-9]:%s$'), '') subquery is preserved in all six INSERTs with matching arg lists.
• DataPack lifecycle. CleanupBanDataPack reads exactly what CreateBan writes, in order (admin, target, admin userid, target userid, time, reasonPack). UTIL_InsertTempBan still frees the pack before formatting. SelectBanIpCallback / SelectAddbanCallback / SelectUnbanCallback free on every new bail-out path; Query_UnBlockSelect frees newDataPack before continue. The new delete dataPack in VerifyInsert is safe — the pack reaching that callback is never the one parked in PlayerDataPack[admin].
• Buffer growth (Query 1536→2048, queue insert 512→1024, report query +768) is sound; no leftover *Escaped / banName / banReason locals, no unused vars, no merge residue.

Pre-existing, not touched (out of scope, but flagging)

• ProcessQueueCallback (main) and Query_ProcessQueue (comms) are invoked as SQLiteDB.Query(...) callbacks, so their db parameter is the SQLite handle — yet both run db.Query(...) on a %s_bans / %s_comms INSERT destined for MySQL. This predates the PR (the old code already used db.Escape + db.Query there). The PR's SQLiteDB.Query → db.Query swap for the DELETE FROM queue is a genuine no-op because db == SQLiteDB here, but the surrounding path looks broken and deserves its own issue.
• VerifyInsert leaks the reasonPack belonging to the stale PlayerDataPack[admin] when the reason-menu flow is used, and returns early without freeing on !IsClientConnected(client).

CI (Compile SourceMod plugins) is green on a6205b1. LGTM with the two fixes applied.

🤖 Generated with Claude Code