Repository navigation
fix(db): chunk large IN-list placeholders (MariaDB error 1390 on big installs) - #43
Merged
Merged
Conversation
…ceiling Port of upstream sbpp#1577, part 1 of 4: the core Database::resultsetInList() / executeInList() helpers plus the convention writeup in AGENTS.md. A generated `WHERE x IN (?,?,?,...)` clause built from a row-derived array grows one placeholder per value. MySQL/MariaDB caps a prepared statement at 65,535 placeholders (error 1390); on an install with enough bans/admins/protests, `array_fill(0, count($rows), '?')` was one bulk operation away from failing PDO::prepare() outright before any values were bound. resultsetInList()/executeInList() slice the value list into IN_LIST_CHUNK_SIZE=10,000-item statements, run one query per chunk, and merge the results (SELECT) or accumulate the affected-row count (write). executeInList(atomic: true) wraps every chunk in a single transaction for callers that need the split to stay all-or-nothing. Both methods reject keyed PDO fetch modes (PDO::FETCH_ASSOC / PDO::FETCH_COLUMN only) since a keyed result can't be merged safely across chunk boundaries, and de-duplicate the input values before chunking so a caller-side duplicate doesn't waste a placeholder slot. New DatabaseInListChunkTest.php pins the 65,535-boundary behavior with a real 75,000-value round trip (SELECT and UPDATE), the empty-input no-op path, the keyed-fetch-mode rejection, and atomic rollback across chunks — plus a regression test for the PruneBans() query rewrite landing in part 3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ssions, groups
Part 2 of 4. Mechanical swap of every row-derived
`implode(',', array_fill(0, count($rows), '?'))` IN-list in these page
handlers over to Database::resultsetInList() / executeInList():
- page.banlist.php: removed-by admin names, active-steam/active-ip
sibling counts, per-bid banlog rows, per-bid comments.
- page.commslist.php: removed-by admin names, active-sibling counts,
per-bid comments.
- admin.bans.php: protests + protests-archive ban details, protest
comments, the protest-archive UPDATE; submissions + submissions-
archive demo filenames, mod names, comments.
- admin.groups.php: web-group members, server-admin-group members,
server-group overrides, per-group server rows.
Each of these lists is normally small (one page's worth of rows), but
on a large install with enough active bans/protests/submissions the
unbounded placeholder count was one bulk operation away from MariaDB
error 1390. No behavior change on a typical install; existing
PublicBanListRegressionTest / WebGroupsCatalogTest coverage stays
green unmodified.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ased JOIN Part 3 of 4. PruneBans() (the cron-driven sweep that archives submissions once a matching active ban exists for their Steam ID or IP) used to SELECT every active ban's authid and every active ban's IP into PHP, build two IN-list clauses from them, and run that against :prefix_submissions. On an install with enough active bans this hit the same 65,535-placeholder ceiling the sibling commit chunks around — but chunking wasn't the right fix here: materialising every active ban identifier into PHP memory just to build a WHERE clause is wasted work when the database can compute the same set intersection directly. Replaced with two INNER JOIN arms (Steam ID against `:prefix_bans` FORCE INDEX (type_authid), IP against FORCE INDEX (type_ip) — both composite indexes already exist in struc.sql) combined with UNION DISTINCT, so MariaDB probes each submission's identifier directly instead of materialising the full active-ban set first. The final archive UPDATE still goes through executeInList(atomic: true) since $subIds is still a row-derived list that can exceed the placeholder ceiling on its own. New DatabaseInListChunkTest::testPruneBansArchivesOnlySubmissionsMatchingActiveBans (landed in part 1) pins the query rewrite: a submission matching an active ban by Steam ID or IP gets archived (archiv=3), one matching only an expired ban or no ban at all does not. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…predicate Part 4 of 4. The web-permission-flag and SourceMod-flag search filters on the admin list (?admwebflag[]=…, ?admsrvflag[]=…) used to SELECT every admin's aid, loop over all of them in PHP calling $userbank->HasAccess($flag, $aid) per admin (a second implicit query / cache lookup per admin per requested flag), collect the matching aids, then build an IN-list from the result. On an install with many admins this was a full-table materialization plus an O(N) permission-check loop on every search — independent of the placeholder-ceiling issue the sibling commits fix, though a large enough match set would also have hit it. Replaced both filters with a single SQL predicate computed inline, mirroring what UserManager::HasAccess() already checks: - Web flags: `(ADM.extraflags | COALESCE(WG.flags, 0)) & ?) <> 0` via a new LEFT JOIN `:prefix_groups` AS WG ON WG.gid = ADM.gid — direct extraflags OR inherited web-group flags, any requested bit matching. - Server flags: `INSTR(BINARY CONCAT(COALESCE(ADM.srv_flags,''), COALESCE(SAG.flags,'')), BINARY ?) > 0` per requested flag character (OR'd together) via a new LEFT JOIN `:prefix_srvgroups` AS SAG ON SAG.name = ADM.srv_group — direct + inherited server flags. SM_ROOT is added to the requested-flag set unconditionally so a root holder still matches any reserved-slot-style filter. BINARY guards both the CONCAT and the comparison since SourceMod flags are case-sensitive in HasAccess() but the table's default collation is not. The server-group join (`:prefix_admins_servers_groups` / `:prefix_servers_groups`) can already produce duplicate ADM.aid rows when an admin reaches the same server through more than one path; the new web/SourceMod-group joins can do the same when a group grants more than one of the requested flags. The list query and its COUNT sibling both switched from a PHP dedupe-after-the-fact loop (which only ran for the `server` filter, and desynced the LIMIT window when it removed a row) to `SELECT DISTINCT` / `COUNT(DISTINCT ADM.aid)`. Extends AdminAdminsSearchTest.php with 4 new cases (ported from upstream, using this fork's existing alice/bob/charlie fixture admins): multi-flag OR-combination, SourceMod-flag case sensitivity, duplicate-membership-row dedup via the server filter, and inherited SourceMod-group flags. The existing web/server-flag filter tests (testWebFlagMultiFilterArrayShape, testServerFlagFilterMatchesSrvFlagsAndDoesNotCrash, testServerFlagFilterIncludesSmRootHolders, testServerCustomFlagFilterRoundTripsAndMatches) pass unmodified against the new SQL predicate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…es them - PruneBans: remove FORCE INDEX (type_authid / type_ip). The equality join already picks those indexes, and the hint turned an install missing either one into error 1176 on every banlist render, ban add/edit and GET /api/v1/bans. - inListChunks(): dedupe int 5 and string '5' as the same value so they can't land in different chunks and return a row twice. - admin.admins.php: document that flag filters are scoped by ?view= rather than enabled, and pin it with a test (?view=inactive + a flag used to always be empty because HasAccess() rejects disabled admins).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Port of upstream
sbpp/sourcebans-pp#1577. Found while doing a follow-up upstream-parity pass — this landed on upstreammainafter the audit that produced PRs #40/#41/#42, and this fork inherits the exact same vulnerable patterns.The bug: every row-derived
WHERE x IN (?,?,?,...)clause in this codebase was built withimplode(',', array_fill(0, count($rows), '?'))— one placeholder per value, unbounded. MySQL/MariaDB hard-caps a prepared statement at 65,535 placeholders (error 1390). On an install with enough active bans/admins/protests/submissions, several of these were one bulk operation away from failingPDO::prepare()outright — most seriously inPruneBans(), where a large active-ban count would break the cron-driven submission-archival sweep entirely.What changed (4 commits)
Database::resultsetInList()/executeInList()— new helpers that slice a value list into 10,000-item chunks, run one query per chunk, and merge results (or accumulate affected-row counts).executeInList(atomic: true)wraps a split write in one transaction. Documented in AGENTS.md's Database conventions.page.banlist.php,page.commslist.php,admin.bans.php(protests + submissions),admin.groups.php— every unbounded IN-list now goes through the new helpers. No behavior change.PruneBans()rewrite — replaced "materialize every active ban identifier into PHP, build an IN-list" with twoINNER JOIN … FORCE INDEXarms (type_authid/type_ip, both already instruc.sql) combined withUNION DISTINCT, so MariaDB computes the set intersection directly instead of round-tripping through PHP.admin.admins.phppermission-flag filters — the biggest behavioral change. The?admwebflag[]=…/?admsrvflag[]=…search filters used toSELECTevery admin, loop over all of them in PHP calling$userbank->HasAccess($flag, $aid)per admin, then build an IN-list from the matches. Replaced with a single SQL predicate ((ADM.extraflags | COALESCE(WG.flags,0)) & ? <> 0for web flags;INSTR(BINARY CONCAT(...), BINARY ?) > 0per SourceMod flag) via two new LEFT JOINs, mirroring whatUserManager::HasAccess()already checks (direct + inherited group flags). Also fixes a pre-existing bug: the old per-admin dedup-after-the-fact loop only ran for theserverfilter and desynced theLIMITwindow whenever it removed a duplicate row; the list + count queries now useSELECT DISTINCT/COUNT(DISTINCT ADM.aid)uniformly.Verification
install/pages/page.6.phpbaseline artifact.DatabaseInListChunkTest(new, 6 tests): a real 75,000-value round trip through bothresultsetInList/executeInList, the empty-input no-op, keyed-fetch-mode rejection, atomic rollback across chunks, and thePruneBans()rewrite. All green.AdminAdminsSearchTest(22 tests, 4 new): existing web/server-flag filter tests pass unmodified against the new SQL predicate; new tests cover multi-flag OR, SourceMod-flag case sensitivity, duplicate-membership dedup, and inherited-group-flag matching. All green.PublicBanListRegressionTest,WebGroupsCatalogTest,CommsTest,BansTest,RestBans,RestComms, etc.): all green except a pre-existing, already-documented environmental artifact (CRLF-vs-LF snapshot diffs inSbpp\Tests\Api\*classes, unrelated to any file this PR touches).AdminsService.php,ServersService.php), which carry the sameimplode/array_fillIN-list shape but weren't part of upstream's fix (upstream has no REST API) — confirmed not vulnerable: both are hard-capped atmin(100, $perPage)before the list is ever built, well under the placeholder ceiling. No change needed there.Test plan
./sbpp.sh db-seed --scale=large, 2000 bans), confirm the admin list, banlist, commslist, and protests/submissions queues still render.🤖 Generated with Claude Code