Skip to content

fix(sql): stop the group color list from growing on every reload - #42

Merged
Rushaway merged 1 commit into
masterfrom
fix/sql-audit
Oct 29, 2026
Merged

Rushaway merged 1 commit into
masterfrom
fix/sql-audit

Conversation

@Rushaway

@Rushaway Rushaway commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

Summary

  • The color list grew on every reload until it overflowed. OnSQLSelect_Color appended the sb_srvgroups rows to g_sColorList without resetting g_iColorListSize and without checking COLOR_LIST_MAX_LENGTH (255). Each sm_admins_config_mod change to 0 or 1 calls SQLInitialize() and adds the whole table again. That duplicates the entries scanned for every sm_admins, and after enough changes (or with more than 255 groups) it raises an "Array index out-of-bounds" error in the callback, so the colors stop loading. Now the callback resets the size first and stops at 255 rows. (AdvancedAdminList.sp)
  • Connection handling. SQLInitialize() deleted and reopened the connection on every cvar change. A change made while a connect was still in flight produced two callbacks, and both assigned g_hDatabase, so the first handle leaked. Now the open connection is reused (only the colors are reloaded), and a second call while a connect is pending does nothing. When the driver is not MySQL the handle is now closed instead of being kept, unused, as g_hDatabase. A failed connect is now logged and retried after 30 s; before, colors never loaded until the plugin was reloaded. (AdvancedAdminList.sp)
  • Version 2.1.9 -> 2.1.10.

Not included

  • A missing sourcebans entry in databases.cfg still calls SetFailState, as before (it is a config error, not a failed connect).

Test plan

  • Compiles with spcomp 1.12.0.7259 (same deps as CI), no new warnings
  • CI build passes
  • Toggle sm_admins_config_mod 1 -> 2 -> 1 several times: sm_admins shows the same colors each time, and no array-index errors appear in the logs
  • Start with MySQL down: a retry is logged every 30 s, and the colors load once MySQL is back

- OnSQLSelect_Color reset nothing and had no bound: every
  sm_admins_config_mode change appended the sb_srvgroups rows again
  until the 255-entry array overflowed. Reset it and stop at the max.
- Reuse the open connection instead of reconnecting on each cvar
  change, ignore a second connect while one is in flight, close a
  non-MySQL handle, retry a failed connect after 30 seconds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 28, 2026 14:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@WareZ57 WareZ57 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this against master (merges cleanly, CI green) and compiled with spcomp 1.12.0.7259 + MultiColors, no warnings.

  • OnSQLSelect_Color now resets g_iColorListSize and stops at COLOR_LIST_MAX_LENGTH (AdvancedAdminList.sp:229-235). That fixes both the duplicate entries on every sm_admins_config_mod change and the out-of-bounds write with more than 255 groups. Callbacks run one at a time on the main thread, so two overlapping SELECTs (toggling the cvar quickly) just rebuild the same list twice.
  • Connection handling: an existing handle is reused (:152-156), a second call while a connect is in flight is a no-op (:159-160), and the non-MySQL handle is now deleted instead of kept (:186-191). I didn't find any path that can assign g_hDatabase twice or leak a handle now.
  • The retry timer is guarded by g_hSQLRetryTimer so it can't stack. It has no TIMER_FLAG_NO_MAPCHANGE, so it survives a map change, and it re-checks the config mode before reconnecting (:198-206). Looks right.

Unrelated to this PR and optional: switching sm_admins_config_mod to 2 (".cfg only") doesn't clear g_iColorListSize, so the SQL colors from an earlier mode 0/1 are still used as the fallback in the lookup at :534-544. Resetting g_iColorListSize = 0 in OnCvarChanged when the mode becomes 2 would make it match the cvar description. That can be a separate change.

LGTM.

@Rushaway
Rushaway merged commit 762d1b3 into master Oct 29, 2026
7 checks passed
@Rushaway
Rushaway deleted the fix/sql-audit branch October 29, 2026 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants