fix(game): Harmonize SQL query formatting, truncation logging, and DataPack lifecycle by Rushaway · Pull Request #1492 · sbpp/sourcebans-pp

Rushaway · 2026-06-01T06:56:25Z

Description

This PR updates both SourceBans++ scripting plugins to apply a consistent SQL construction and error-handling pattern.

Updated files:

sbpp_main.sp
sbpp_comms.sp

Key changes:

Replaced manual escaping patterns with DB.Format/SQLiteDB.Format where applicable.
Standardized placeholder usage:
- %!s for internal SQL values (DatabasePrefix, table/fragment internals).
- %s for user/runtime inputs.
Added/kept defensive query buffer truncation checks before DB.Query calls.
Harmonized truncation diagnostics to use LogError consistently across both plugins.
Removed early DataPack creation on code paths where query formatting can fail:
- DataPack is now created after successful query formatting in affected command/query paths.
- This removes unnecessary rollback-style delete usage introduced only for failed pre-query guards.

Motivation and Context

This change improves safety and consistency in SQL handling across plugins:

Prevents double-escaping mistakes from mixed Escape + Format patterns.
Enforces a clear placeholder contract for internal vs user-provided values.
Makes truncation failures visible in the same way across both plugins (LogError).
Simplifies ownership/lifecycle of DataPack objects by allocating them only when needed.

How Has This Been Tested?

Editor diagnostics check:
- No errors reported for both updated files.
Pattern verification checks:
- No remaining .Escape(...) usages.
- Truncation guard logs use LogError consistently in both plugins.
Lifecycle verification:
- DataPack allocation moved after successful query formatting in modified guard paths.
- Remaining delete dataPack calls are in normal callback/cleanup flows.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

Note: If your PR touches web/** and this is your first
contribution to the web panel, the CLA bot will comment within
~30 seconds with one-line sign instructions. You only need to sign
once per repo. See CLA.md for the full text and
CONTRIBUTING.md for the rationale.

Copilot

Pull request overview

This PR refactors the two SourceBans++ SourceMod plugins to standardize SQL query construction and error handling, primarily by replacing manual escaping + Format/FormatEx patterns with Database.Format/SQLiteDB.Format and adding consistent truncation guards.

Changes:

Migrated query building to DB.Format / db.Format / SQLiteDB.Format with consistent placeholder usage and truncation checks.
Standardized truncation diagnostics to LogError and increased some query buffer sizes.
Adjusted DataPack allocation timing in some paths to avoid unnecessary allocation when query formatting fails.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 35 comments.

File	Description
game/addons/sourcemod/scripting/sbpp_main.sp	Reworked multiple ban/unban and queue queries to use `DB.Format`, added truncation guards, and adjusted some DataPack lifecycle decisions.
game/addons/sourcemod/scripting/sbpp_comms.sp	Reworked comms queries to use `Database.Format`, added truncation guards, and moved DataPack allocation in unblock paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+		{
+			LogError("SelectAddbanCallback insert query truncated");
+			return;
+		}


Rushaway · 2026-06-30T07:42:03Z

Untested yet, what's your pov on this @rumblefrog ?

… DataPack lifecycle - replace Escape + Format patterns with DB.Format/SQLiteDB.Format in main and comms - enforce placeholder policy: %!s for internal SQL identifiers/fragments, %s for runtime/user input - add/keep query buffer truncation guards before query execution - standardize truncation diagnostics on LogError across both plugins

Added deletion of newDataPack on query truncation.

Removed unnecessary delete statements for dataPack. Added cleanup function for DataPack.

…taPack lifecycle Port of upstream sbpp#1492, adapted to keep the fork's admin_name column/subquery in the bans and comms INSERT statements. - replace Escape + Format patterns with DB.Format/SQLiteDB.Format in main and comms - enforce placeholder policy: %!s for internal SQL identifiers/fragments, %s for runtime/user input - add query buffer truncation guards before query execution, standardized on LogError - allocate DataPack only after successful query formatting; add CleanupBanDataPack helper Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Rushaway <rushaway@hotmail.fr>

…prebuilt SQL fragments Two defects found while reviewing the port of sbpp#1492. 1. Truncation guards were dead code. Database.Format / SQL_FormatQuery goes through InternalFormat -> atcprintf, which starts with llen = maxlen - 1 and returns (maxlen - llen - 1). The return value is therefore capped at maxlength - 1 and can never be >= sizeof(buffer), so every "if (db.Format(...) >= sizeof(buf))" check (and the inverted "< sizeof(buf)" variants in VerifyBan) could never trigger. All 34 guards in sbpp_main.sp and sbpp_comms.sp now compare against sizeof(buf) - 1 (and iQueryLen - 1 for the dynamically sized report query), which is the standard truncation test. 2. sbpp_comms.sp InsertTempBlock escaped its own SQL. The queue2 INSERT passed the prebuilt sQueryMute/sQueryGag value tuples through %s, so SQLiteDB.Format escaped the single quotes inside them and produced a syntactically invalid statement, silently dropping temp comm blocks queued while the main database was down. Switched to %!s%!s%!s to match the equivalent statement in SavePunishment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Rushaway <rushaway@hotmail.fr>

…taPack lifecycle Port of upstream sbpp#1492, adapted to keep the fork's admin_name column/subquery in the bans and comms INSERT statements. - replace Escape + Format patterns with DB.Format/SQLiteDB.Format in main and comms - enforce placeholder policy: %!s for internal SQL identifiers/fragments, %s for runtime/user input - add query buffer truncation guards before query execution, standardized on LogError - allocate DataPack only after successful query formatting; add CleanupBanDataPack helper Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Rushaway <rushaway@hotmail.fr>

…prebuilt SQL fragments Two defects found while reviewing the port of sbpp#1492. 1. Truncation guards were dead code. Database.Format / SQL_FormatQuery goes through InternalFormat -> atcprintf, which starts with llen = maxlen - 1 and returns (maxlen - llen - 1). The return value is therefore capped at maxlength - 1 and can never be >= sizeof(buffer), so every "if (db.Format(...) >= sizeof(buf))" check (and the inverted "< sizeof(buf)" variants in VerifyBan) could never trigger. All 34 guards in sbpp_main.sp and sbpp_comms.sp now compare against sizeof(buf) - 1 (and iQueryLen - 1 for the dynamically sized report query), which is the standard truncation test. 2. sbpp_comms.sp InsertTempBlock escaped its own SQL. The queue2 INSERT passed the prebuilt sQueryMute/sQueryGag value tuples through %s, so SQLiteDB.Format escaped the single quotes inside them and produced a syntactically invalid statement, silently dropping temp comm blocks queued while the main database was down. Switched to %!s%!s%!s to match the equivalent statement in SavePunishment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Rushaway <rushaway@hotmail.fr>

* fix(game): harmonize SQL query formatting, truncation logging, and DataPack lifecycle Port of upstream sbpp#1492, adapted to keep the fork's admin_name column/subquery in the bans and comms INSERT statements. - replace Escape + Format patterns with DB.Format/SQLiteDB.Format in main and comms - enforce placeholder policy: %!s for internal SQL identifiers/fragments, %s for runtime/user input - add query buffer truncation guards before query execution, standardized on LogError - allocate DataPack only after successful query formatting; add CleanupBanDataPack helper * review fixes: make truncation guards actually fire and stop escaping prebuilt SQL fragments Two defects found while reviewing the port of sbpp#1492. 1. Truncation guards were dead code. Database.Format / SQL_FormatQuery goes through InternalFormat -> atcprintf, which starts with llen = maxlen - 1 and returns (maxlen - llen - 1). The return value is therefore capped at maxlength - 1 and can never be >= sizeof(buffer), so every "if (db.Format(...) >= sizeof(buf))" check (and the inverted "< sizeof(buf)" variants in VerifyBan) could never trigger. All 34 guards in sbpp_main.sp and sbpp_comms.sp now compare against sizeof(buf) - 1 (and iQueryLen - 1 for the dynamically sized report query), which is the standard truncation test. 2. sbpp_comms.sp InsertTempBlock escaped its own SQL. The queue2 INSERT passed the prebuilt sQueryMute/sQueryGag value tuples through %s, so SQLiteDB.Format escaped the single quotes inside them and produced a syntactically invalid statement, silently dropping temp comm blocks queued while the main database was down. Switched to %!s%!s%!s to match the equivalent statement in SavePunishment.

rumblefrog reviewed Jun 1, 2026

View reviewed changes

Comment thread game/addons/sourcemod/scripting/sbpp_comms.sp

rumblefrog reviewed Jun 1, 2026

View reviewed changes

Comment thread game/addons/sourcemod/scripting/sbpp_comms.sp

rumblefrog requested a review from Copilot June 1, 2026 20:42

Copilot started reviewing on behalf of rumblefrog June 1, 2026 20:42

View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

Rushaway added 4 commits August 12, 2026 15:56

Update sbpp_main.sp

f467c3a

Handle query truncation by deleting newDataPack

40cf767

Added deletion of newDataPack on query truncation.

Refactor ban handling and cleanup DataPack usage

14a7338

Removed unnecessary delete statements for dataPack. Added cleanup function for DataPack.

Rushaway force-pushed the game-sql branch from adddbfb to 14a7338 Compare August 12, 2026 13:56

Rushaway requested a review from a team August 12, 2026 13:56

Rushaway mentioned this pull request Sep 3, 2026

fix(game): Harmonize SQL query formatting, truncation logging, and DataPack lifecycle srcdslab/sourcebans-pp#21

Merged

Rushaway mentioned this pull request Sep 18, 2026

fix(game): close DataPack ownership leaks and stale client-index reuse in the ban flow srcdslab/sourcebans-pp#40

Merged

8 tasks

Navigation Menu

Repository navigation

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

fix(game): Harmonize SQL query formatting, truncation logging, and DataPack lifecycle#1492
Rushaway wants to merge 4 commits into
sbpp:mainfrom
Rushaway:game-sql

Rushaway commented Jun 1, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rushaway commented Jun 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Rushaway commented Jun 1, 2026

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rushaway commented Jun 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants